From 8ad1a9bf6c0d3640eaf39f6d135ddff9598ba38b Mon Sep 17 00:00:00 2001 From: dancoombs Date: Mon, 21 Oct 2024 15:04:52 -0500 Subject: [PATCH] chore(pool): add tests for da/gas limit in pool --- crates/pool/src/mempool/pool.rs | 169 ++++++++++++++++++- crates/pool/src/mempool/uo_pool.rs | 198 ++++++++++++++++++++--- crates/provider/src/traits/test_utils.rs | 38 ++++- 3 files changed, 380 insertions(+), 25 deletions(-) diff --git a/crates/pool/src/mempool/pool.rs b/crates/pool/src/mempool/pool.rs index e07bdf2dc..919e2b681 100644 --- a/crates/pool/src/mempool/pool.rs +++ b/crates/pool/src/mempool/pool.rs @@ -181,7 +181,7 @@ where required_pvg: u128, ) -> MempoolResult { // only eligibility criteria is required PVG which is enabled when da_gas_tracking is enabled - let is_eligible = if self.config.da_gas_tracking_enabled { + let is_eligible = if self.config.da_gas_tracking_enabled && self.da_gas_oracle.is_some() { if op.uo.pre_verification_gas() < required_pvg { self.emit(PoolEvent::UpdatedDAData { op_hash: op @@ -766,6 +766,7 @@ struct PoolMetrics { #[cfg(test)] mod tests { use alloy_primitives::U256; + use rundler_provider::MockDAGasOracleSync; use rundler_types::{ v0_6::UserOperation, EntityInfo, EntityInfos, UserOperation as UserOperationTrait, ValidTimeRange, @@ -1227,6 +1228,164 @@ mod tests { assert_eq!(None, pool.get_operation_by_hash(hash3)); } + #[test] + fn test_add_operation_ineligible_initially() { + let mut conf = conf(); + conf.da_gas_tracking_enabled = true; + let mut pool = pool_with_conf_oracle(conf.clone(), MockDAGasOracleSync::default()); + + let po1 = create_op(Address::random(), 0, 10); + + let hash = pool.add_operation(po1, 50_001).unwrap(); + + assert!(pool.get_operation_by_hash(hash).is_some()); + assert_eq!(pool.best_operations().collect::>().len(), 0); // UO is ineligible due to pvg + } + + #[test] + fn test_add_operation_ineligible_then_eligible() { + let mut conf = conf(); + conf.chain_spec.da_pre_verification_gas = true; + conf.chain_spec.include_da_gas_in_gas_limit = true; + conf.da_gas_tracking_enabled = true; + + let po1 = create_op(Address::random(), 0, 10); + let pvg = po1.uo.pre_verification_gas(); + let da_pvg = po1 + .uo + .pre_verification_da_gas_limit(&conf.chain_spec, Some(1)); + + let mut oracle = MockDAGasOracleSync::default(); + oracle + .expect_calc_da_gas_sync() + .returning(move |_, _, _| da_pvg - 1); + + let mut pool = pool_with_conf_oracle(conf.clone(), oracle); + + let hash = pool.add_operation(po1, pvg + 1).unwrap(); + + assert!(pool.get_operation_by_hash(hash).is_some()); + assert_eq!(pool.best_operations().collect::>().len(), 0); // UO is ineligible due to pvg + + pool.do_maintenance( + 0, + 0.into(), + Some(&DAGasBlockData::default()), + GasFees::default(), + 0, + ); + + assert_eq!(pool.best_operations().collect::>().len(), 1); // UO is now eligible + } + + #[test] + fn test_add_operation_eligible_then_ineligible() { + let mut conf = conf(); + conf.chain_spec.da_pre_verification_gas = true; + conf.chain_spec.include_da_gas_in_gas_limit = true; + conf.da_gas_tracking_enabled = true; + + let po1 = create_op(Address::random(), 0, 10); + let pvg = po1.uo.pre_verification_gas(); + let da_pvg = po1 + .uo + .pre_verification_da_gas_limit(&conf.chain_spec, Some(1)); + + let mut oracle = MockDAGasOracleSync::default(); + oracle + .expect_calc_da_gas_sync() + .returning(move |_, _, _| da_pvg + 1); + + let mut pool = pool_with_conf_oracle(conf.clone(), oracle); + + let hash = pool.add_operation(po1, pvg).unwrap(); + + assert!(pool.get_operation_by_hash(hash).is_some()); + assert_eq!(pool.best_operations().collect::>().len(), 1); + + pool.do_maintenance( + 0, + 0.into(), + Some(&DAGasBlockData::default()), + GasFees::default(), + 0, + ); + + assert_eq!(pool.best_operations().collect::>().len(), 0); + } + + #[test] + fn test_add_operation_eligible_then_eligible() { + let mut conf = conf(); + conf.chain_spec.da_pre_verification_gas = true; + conf.chain_spec.include_da_gas_in_gas_limit = true; + conf.da_gas_tracking_enabled = true; + + let po1 = create_op(Address::random(), 0, 10); + let pvg = po1.uo.pre_verification_gas(); + let da_pvg = po1 + .uo + .pre_verification_da_gas_limit(&conf.chain_spec, Some(1)); + + let mut oracle = MockDAGasOracleSync::default(); + oracle + .expect_calc_da_gas_sync() + .returning(move |_, _, _| da_pvg); + + let mut pool = pool_with_conf_oracle(conf.clone(), oracle); + + let hash = pool.add_operation(po1, pvg).unwrap(); + + assert!(pool.get_operation_by_hash(hash).is_some()); + assert_eq!(pool.best_operations().collect::>().len(), 1); + + pool.do_maintenance( + 0, + 0.into(), + Some(&DAGasBlockData::default()), + GasFees::default(), + 0, + ); + + assert_eq!(pool.best_operations().collect::>().len(), 1); + } + + #[test] + fn test_add_operation_ineligible_then_ineligible() { + let mut conf = conf(); + conf.chain_spec.da_pre_verification_gas = true; + conf.chain_spec.include_da_gas_in_gas_limit = true; + conf.da_gas_tracking_enabled = true; + + let po1 = create_op(Address::random(), 0, 10); + let pvg = po1.uo.pre_verification_gas(); + let da_pvg = po1 + .uo + .pre_verification_da_gas_limit(&conf.chain_spec, Some(1)); + + let mut oracle = MockDAGasOracleSync::default(); + oracle + .expect_calc_da_gas_sync() + .returning(move |_, _, _| da_pvg + 1); + + let mut pool = pool_with_conf_oracle(conf.clone(), oracle); + + let hash = pool.add_operation(po1, pvg + 1).unwrap(); + + assert!(pool.get_operation_by_hash(hash).is_some()); + assert_eq!(pool.best_operations().collect::>().len(), 0); + + pool.do_maintenance( + 0, + 0.into(), + Some(&DAGasBlockData::default()), + GasFees::default(), + 0, + ); + + assert_eq!(pool.best_operations().collect::>().len(), 0); + } + fn conf() -> PoolInnerConfig { PoolInnerConfig { chain_spec: ChainSpec::default(), @@ -1247,6 +1406,13 @@ mod tests { PoolInner::new(conf, None, broadcast::channel(100000).0) } + fn pool_with_conf_oracle( + conf: PoolInnerConfig, + oracle: MockDAGasOracleSync, + ) -> PoolInner { + PoolInner::new(conf, Some(oracle), broadcast::channel(100000).0) + } + fn mem_size_of_ordered_pool_op() -> usize { OrderedPoolOperation::new(Arc::new(create_op(Address::random(), 1, 1)), 1, true).mem_size() } @@ -1257,6 +1423,7 @@ mod tests { sender, nonce: U256::from(nonce), max_fee_per_gas, + pre_verification_gas: 50_000, ..UserOperation::default() } .into(), diff --git a/crates/pool/src/mempool/uo_pool.rs b/crates/pool/src/mempool/uo_pool.rs index d928614cb..5ce7448d1 100644 --- a/crates/pool/src/mempool/uo_pool.rs +++ b/crates/pool/src/mempool/uo_pool.rs @@ -159,8 +159,21 @@ where ) -> MempoolResult<()> { // Check call gas limit efficiency only if needed if self.config.gas_limit_efficiency_reject_threshold > 0.0 { + // Node clients set base_fee to 0 during eth_call. + // Geth: https://github.com/ethereum/go-ethereum/blob/a5fe7353cff959d6fcfcdd9593de19056edb9bdb/internal/ethapi/api.go#L1202 + // Reth: https://github.com/paradigmxyz/reth/blob/4d3b35dbd24c3a5c6b1a4f7bd86b1451e8efafcc/crates/rpc/rpc-eth-api/src/helpers/call.rs#L1098 + // Arb-geth: https://github.com/OffchainLabs/go-ethereum/blob/54adef6e3fbea263e770c578047fd38842b8e17f/internal/ethapi/api.go#L1126 let gas_price = op.gas_price(0); + + if gas_price == 0 { + // Can't calculate efficiency without gas price, fail open. + return Ok(()); + } + let call_gas_limit = op.call_gas_limit(); + if call_gas_limit == 0 { + return Ok(()); // No call gas limit, not useful, but not a failure here. + } let sim_result = self .ep_providers @@ -890,12 +903,13 @@ struct UoPoolMetrics { #[cfg(test)] mod tests { - use std::collections::HashMap; + use std::{collections::HashMap, vec}; - use alloy_primitives::Bytes; + use alloy_primitives::{uint, Bytes}; use mockall::Sequence; use rundler_provider::{ - DepositInfo, MockEntryPointV0_6, MockEvmProvider, ProvidersWithEntryPoint, + DepositInfo, ExecutionResult, MockDAGasOracleSync, MockEntryPointV0_6, MockEvmProvider, + ProvidersWithEntryPoint, }; use rundler_sim::{ MockPrechecker, MockSimulator, PrecheckError, PrecheckReturn, PrecheckSettings, @@ -1669,6 +1683,118 @@ mod tests { check_ops(pool.best_operations(3, 0).unwrap(), uos); } + #[tokio::test] + async fn test_pre_op_gas_limit_reject() { + let mut config = default_config(); + config.gas_limit_efficiency_reject_threshold = 0.25; + + let op = create_op_from_op_v0_6(UserOperation { + call_gas_limit: 10_000, + verification_gas_limit: 500_000, // used 100K of 550K + pre_verification_gas: 50_000, + max_fee_per_gas: 1, + max_priority_fee_per_gas: 1, + ..Default::default() + }); + + let mut ep = MockEntryPointV0_6::new(); + ep.expect_simulate_handle_op().returning(|_, _, _, _, _| { + Ok(Ok(ExecutionResult { + pre_op_gas: 100_000, + paid: uint!(110_000_U256), + target_success: true, + ..Default::default() + })) + }); + + let pool = create_pool_with_entry_point_config(config, vec![op.clone()], ep); + let ret = pool.add_operation(OperationOrigin::Local, op.op).await; + let actual_eff = 100_000_f32 / 550_000_f32; + + match ret.err().unwrap() { + MempoolError::PreOpGasLimitEfficiencyTooLow(eff, actual) => { + assert_eq!(eff, 0.25); + assert_eq!(actual, actual_eff); + } + _ => panic!("Expected PreOpGasLimitEfficiencyTooLow error"), + } + } + + #[tokio::test] + async fn test_call_gas_limit_reject() { + let mut config = default_config(); + config.gas_limit_efficiency_reject_threshold = 0.25; + + let op = create_op_from_op_v0_6(UserOperation { + call_gas_limit: 50_000, + max_fee_per_gas: 1, + max_priority_fee_per_gas: 1, + ..Default::default() + }); + + let mut ep = MockEntryPointV0_6::new(); + ep.expect_simulate_handle_op().returning(|_, _, _, _, _| { + Ok(Ok(ExecutionResult { + pre_op_gas: 50_000, + paid: uint!(60_000_U256), // call gas used is 10K + target_success: true, + ..Default::default() + })) + }); + + let pool = create_pool_with_entry_point_config(config, vec![op.clone()], ep); + let ret = pool.add_operation(OperationOrigin::Local, op.op).await; + let actual_eff = 10_000_f32 / 50_000_f32; + + match ret.err().unwrap() { + MempoolError::CallGasLimitEfficiencyTooLow(eff, actual) => { + assert_eq!(eff, 0.25); + assert_eq!(actual, actual_eff); + } + _ => panic!("Expected CallGasLimitEfficiencyTooLow error"), + } + } + + #[tokio::test] + async fn test_gas_price_zero_fail_open() { + let mut config = default_config(); + config.gas_limit_efficiency_reject_threshold = 0.25; + + let op = create_op_from_op_v0_6(UserOperation { + call_gas_limit: 50_000, + max_fee_per_gas: 0, + max_priority_fee_per_gas: 0, + ..Default::default() + }); + + let pool = create_pool_with_config(config, vec![op.clone()]); + pool.add_operation(OperationOrigin::Local, op.op) + .await + .unwrap(); + } + + #[tokio::test] + async fn test_da_gas_ineligible() { + let mut config = default_config(); + config.da_gas_tracking_enabled = true; + + let op = create_op_from_op_v0_6(UserOperation { + call_gas_limit: 50_000, + max_fee_per_gas: 0, + max_priority_fee_per_gas: 0, + pre_verification_gas: 50_000, // below 100K + ..Default::default() + }); + + let pool = create_pool_with_config(config, vec![op.clone()]); + pool.add_operation(OperationOrigin::Local, op.op) + .await + .unwrap(); + + let best = pool.best_operations(10000, 0).unwrap(); + assert_eq!(best.len(), 0); + } + #[derive(Clone, Debug)] struct OpWithErrors { op: UserOperationVariant, @@ -1678,19 +1804,8 @@ mod tests { staked: bool, } - fn create_pool( - ops: Vec, - ) -> UoPool { - let entrypoint = MockEntryPointV0_6::new(); - create_pool_with_entry_point(ops, entrypoint) - } - - fn create_pool_with_entry_point( - ops: Vec, - entrypoint: MockEntryPointV0_6, - ) -> UoPool { - let entrypoint = Arc::new(entrypoint); - let args = PoolConfig { + fn default_config() -> PoolConfig { + PoolConfig { chain_spec: ChainSpec::default(), entry_point: Address::random(), entry_point_version: EntryPointVersion::V0_6, @@ -1711,7 +1826,38 @@ mod tests { reputation_tracking_enabled: true, drop_min_num_blocks: 10, gas_limit_efficiency_reject_threshold: 0.0, - }; + } + } + + fn create_pool( + ops: Vec, + ) -> UoPool { + let entrypoint = MockEntryPointV0_6::new(); + create_pool_with_entry_point(ops, entrypoint) + } + + fn create_pool_with_config( + args: PoolConfig, + ops: Vec, + ) -> UoPool { + let entrypoint = MockEntryPointV0_6::new(); + create_pool_with_entry_point_config(args, ops, entrypoint) + } + + fn create_pool_with_entry_point( + ops: Vec, + entrypoint: MockEntryPointV0_6, + ) -> UoPool { + let config = default_config(); + create_pool_with_entry_point_config(config, ops, entrypoint) + } + + fn create_pool_with_entry_point_config( + args: PoolConfig, + ops: Vec, + entrypoint: MockEntryPointV0_6, + ) -> UoPool { + let entrypoint = Arc::new(entrypoint); let mut evm = MockEvmProvider::new(); evm.expect_get_latest_block_hash_and_number() @@ -1754,7 +1900,7 @@ mod tests { } else { Ok(PrecheckReturn { da_gas_data: DAGasUOData::Empty, - required_pre_verification_gas: 0, + required_pre_verification_gas: 100_000, }) } }); @@ -1777,7 +1923,7 @@ mod tests { }, ..EntityInfos::default() }, - pre_op_gas: 1000, + pre_op_gas: 100_000, ..SimulationResult::default() }) } @@ -1785,11 +1931,11 @@ mod tests { } let (event_sender, _) = broadcast::channel(4); - let none_oracle: Option> = None; + let da_oracle = Arc::new(MockDAGasOracleSync::new()); UoPool::new( args, - ProvidersWithEntryPoint::new(Arc::new(evm), entry_point, none_oracle), + ProvidersWithEntryPoint::new(Arc::new(evm), entry_point, Some(da_oracle)), UoPoolProviders::new(simulator, prechecker), event_sender, paymaster, @@ -1877,6 +2023,16 @@ mod tests { } } + fn create_op_from_op_v0_6(op: UserOperation) -> OpWithErrors { + OpWithErrors { + op: op.into(), + valid_time_range: ValidTimeRange::default(), + precheck_error: None, + simulation_error: None, + staked: false, + } + } + fn check_ops(ops: Vec>, expected: Vec) { assert_eq!(ops.len(), expected.len()); for (actual, expected) in ops.into_iter().zip(expected) { diff --git a/crates/provider/src/traits/test_utils.rs b/crates/provider/src/traits/test_utils.rs index 8beeb48fe..49ce1cf73 100644 --- a/crates/provider/src/traits/test_utils.rs +++ b/crates/provider/src/traits/test_utils.rs @@ -28,9 +28,9 @@ use rundler_types::{ use super::error::ProviderResult; use crate::{ - AggregatorOut, BlockHashOrNumber, BundleHandler, DAGasProvider, DepositInfo, EntryPoint, - EntryPointProvider, EvmCall, EvmProvider as EvmProviderTrait, ExecutionResult, HandleOpsOut, - SignatureAggregator, SimulationProvider, + AggregatorOut, BlockHashOrNumber, BundleHandler, DAGasOracle, DAGasOracleSync, DAGasProvider, + DepositInfo, EntryPoint, EntryPointProvider, EvmCall, EvmProvider as EvmProviderTrait, + ExecutionResult, HandleOpsOut, SignatureAggregator, SimulationProvider, }; mockall::mock! { @@ -290,3 +290,35 @@ mockall::mock! { impl EntryPointProvider for EntryPointV0_7 {} } + +mockall::mock! { + pub DAGasOracleSync {} + + #[async_trait::async_trait] + impl DAGasOracleSync for DAGasOracleSync { + async fn block_data(&self, block: BlockHashOrNumber) -> ProviderResult; + async fn uo_data( + &self, + uo_data: Bytes, + to: Address, + block: BlockHashOrNumber, + ) -> ProviderResult; + fn calc_da_gas_sync( + &self, + uo_data: &DAGasUOData, + block_data: &DAGasBlockData, + gas_price: u128, + ) -> u128; + } + + #[async_trait::async_trait] + impl DAGasOracle for DAGasOracleSync { + async fn estimate_da_gas( + &self, + uo_bytes: Bytes, + to: Address, + block: BlockHashOrNumber, + gas_price: u128, + ) -> ProviderResult<(u128, DAGasUOData, DAGasBlockData)>; + } +}