From e94908d8c9d256b9d0b4c072ad096be4c423a5f1 Mon Sep 17 00:00:00 2001 From: dancoombs Date: Tue, 31 Dec 2024 14:34:31 -0600 Subject: [PATCH] fix: fix multiple paymaster post op bugs - Don't zero out paymaster post op gas limit during gas estimation - Encorporate paymaster post op gas limit during efficiency check --- crates/builder/src/bundle_proposer.rs | 7 ++-- crates/pool/proto/op_pool/op_pool.proto | 4 +- crates/pool/src/mempool/mod.rs | 4 +- crates/pool/src/mempool/uo_pool.rs | 27 +++++++------ crates/pool/src/server/remote/error.rs | 24 ++++++------ crates/rpc/src/eth/error.rs | 2 +- crates/sim/src/estimation/v0_6.rs | 2 +- crates/sim/src/estimation/v0_7.rs | 6 +-- crates/sim/src/gas/oracle.rs | 2 +- crates/sim/src/precheck.rs | 2 +- crates/types/src/pool/error.rs | 6 +-- crates/types/src/user_operation/mod.rs | 52 ++++++++++++++++--------- crates/types/src/user_operation/v0_6.rs | 8 ++++ crates/types/src/user_operation/v0_7.rs | 4 ++ 14 files changed, 92 insertions(+), 58 deletions(-) diff --git a/crates/builder/src/bundle_proposer.rs b/crates/builder/src/bundle_proposer.rs index 47c09f33e..d5fd3defc 100644 --- a/crates/builder/src/bundle_proposer.rs +++ b/crates/builder/src/bundle_proposer.rs @@ -566,7 +566,8 @@ where } // Skip this op if the bundle does not have enough remaining gas to execute it. - let required_gas = gas_spent + op.execution_gas_limit(&self.settings.chain_spec, None); + let required_gas = + gas_spent + op.computation_gas_limit(&self.settings.chain_spec, None); if required_gas > self.settings.max_bundle_gas { continue; } @@ -602,7 +603,7 @@ where } // Update the running gas that would need to be be spent to execute the bundle so far. - gas_spent += op.execution_gas_limit(&self.settings.chain_spec, None); + gas_spent += op.computation_gas_limit(&self.settings.chain_spec, None); constructed_bundle_size = constructed_bundle_size.saturating_add(op_size_with_offset_word); @@ -1069,7 +1070,7 @@ where for op in ops { // Here we use optimistic gas limits for the UOs by assuming none of the paymaster UOs use postOp calls. // This way after simulation once we have determined if each UO actually uses a postOp call or not we can still pack a full bundle - let gas = op.uo.execution_gas_limit(&self.settings.chain_spec, None); + let gas = op.uo.computation_gas_limit(&self.settings.chain_spec, None); if gas_left < gas { self.emit(BuilderEvent::skipped_op( self.builder_index, diff --git a/crates/pool/proto/op_pool/op_pool.proto b/crates/pool/proto/op_pool/op_pool.proto index 3c07ef621..25c5e1e56 100644 --- a/crates/pool/proto/op_pool/op_pool.proto +++ b/crates/pool/proto/op_pool/op_pool.proto @@ -512,7 +512,7 @@ message MempoolError { PaymasterBalanceTooLow paymaster_balance_too_low = 15; OperationDropTooSoon operation_drop_too_soon = 16; PreOpGasLimitEfficiencyTooLow pre_op_gas_limit_efficiency_too_low = 17; - CallGasLimitEfficiencyTooLow call_gas_limit_efficiency_too_low = 18; + ExecutionGasLimitEfficiencyTooLow execution_gas_limit_efficiency_too_low = 18; } } @@ -570,7 +570,7 @@ message PreOpGasLimitEfficiencyTooLow { float actual = 2; } -message CallGasLimitEfficiencyTooLow { +message ExecutionGasLimitEfficiencyTooLow { float required = 1; float actual = 2; } diff --git a/crates/pool/src/mempool/mod.rs b/crates/pool/src/mempool/mod.rs index 71b40a919..13bd31702 100644 --- a/crates/pool/src/mempool/mod.rs +++ b/crates/pool/src/mempool/mod.rs @@ -95,8 +95,8 @@ pub trait Mempool: Send + Sync { /// Looks up a user operation by hash, returns None if not found fn get_user_operation_by_hash(&self, hash: B256) -> Option>; - /// Debug methods - + // Debug methods + /// Clears the mempool of UOs or reputation of all addresses fn clear_state(&self, clear_mempool: bool, clear_paymaster: bool, clear_reputation: bool); diff --git a/crates/pool/src/mempool/uo_pool.rs b/crates/pool/src/mempool/uo_pool.rs index 26b440ca3..77b3e4eb9 100644 --- a/crates/pool/src/mempool/uo_pool.rs +++ b/crates/pool/src/mempool/uo_pool.rs @@ -150,7 +150,7 @@ where self.ep_specific_metrics.removed_entities.increment(1); } - async fn check_call_gas_limit_efficiency( + async fn check_execution_gas_limit_efficiency( &self, op: UserOperationVariant, block_hash: B256, @@ -168,8 +168,8 @@ where return Ok(()); } - let call_gas_limit = op.call_gas_limit(); - if call_gas_limit == 0 { + let execution_gas_limit = op.execution_gas_limit(); + if execution_gas_limit == 0 { return Ok(()); // No call gas limit, not useful, but not a failure here. } @@ -199,13 +199,15 @@ where .try_into() .context("total gas used should fit in u128")?; - let call_gas_used = total_gas_used - execution_res.pre_op_gas; + let execution_gas_used = total_gas_used - execution_res.pre_op_gas; - let call_gas_efficiency = call_gas_used as f32 / call_gas_limit as f32; - if call_gas_efficiency < self.config.gas_limit_efficiency_reject_threshold { - return Err(MempoolError::CallGasLimitEfficiencyTooLow( + let execution_gas_efficiency = + execution_gas_used as f32 / execution_gas_limit as f32; + if execution_gas_efficiency < self.config.gas_limit_efficiency_reject_threshold + { + return Err(MempoolError::ExecutionGasLimitEfficiencyTooLow( self.config.gas_limit_efficiency_reject_threshold, - call_gas_efficiency, + execution_gas_efficiency, )); } } @@ -551,8 +553,9 @@ where .simulator() .simulate_validation(versioned_op, block_hash, None) .map_err(Into::into); - let call_gas_check_future = self.check_call_gas_limit_efficiency(op.clone(), block_hash); - let (sim_result, _) = tokio::try_join!(sim_fut, call_gas_check_future)?; + let execution_gas_check_future = + self.check_execution_gas_limit_efficiency(op.clone(), block_hash); + let (sim_result, _) = tokio::try_join!(sim_fut, execution_gas_check_future)?; // No aggregators supported for now if let Some(agg) = &sim_result.aggregator { @@ -1809,11 +1812,11 @@ mod tests { let actual_eff = 10_000_f32 / 50_000_f32; match ret.err().unwrap() { - MempoolError::CallGasLimitEfficiencyTooLow(eff, actual) => { + MempoolError::ExecutionGasLimitEfficiencyTooLow(eff, actual) => { assert_eq!(eff, 0.25); assert_eq!(actual, actual_eff); } - _ => panic!("Expected CallGasLimitEfficiencyTooLow error"), + _ => panic!("Expected ExecutionGasLimitEfficiencyTooLow error"), } } diff --git a/crates/pool/src/server/remote/error.rs b/crates/pool/src/server/remote/error.rs index 1b6cad5f4..99b86cc29 100644 --- a/crates/pool/src/server/remote/error.rs +++ b/crates/pool/src/server/remote/error.rs @@ -24,10 +24,10 @@ use rundler_types::{ use super::protos::{ mempool_error, precheck_violation_error, simulation_violation_error, validation_revert, AccessedUndeployedContract, AccessedUnsupportedContractType, AggregatorValidationFailed, - AssociatedStorageDuringDeploy, AssociatedStorageIsAlternateSender, - CallGasLimitEfficiencyTooLow, CallGasLimitTooLow, CallHadValue, CalledBannedEntryPointMethod, - CodeHashChanged, DidNotRevert, DiscardedOnInsertError, Entity, EntityThrottledError, - EntityType, EntryPointRevert, ExistingSenderWithInitCode, FactoryCalledCreate2Twice, + AssociatedStorageDuringDeploy, AssociatedStorageIsAlternateSender, CallGasLimitTooLow, + CallHadValue, CalledBannedEntryPointMethod, CodeHashChanged, DidNotRevert, + DiscardedOnInsertError, Entity, EntityThrottledError, EntityType, EntryPointRevert, + ExecutionGasLimitEfficiencyTooLow, ExistingSenderWithInitCode, FactoryCalledCreate2Twice, FactoryIsNotContract, InvalidAccountSignature, InvalidPaymasterSignature, InvalidSignature, InvalidStorageAccess, InvalidTimeRange, MaxFeePerGasTooLow, MaxOperationsReachedError, MaxPriorityFeePerGasTooLow, MempoolError as ProtoMempoolError, MultipleRolesViolation, @@ -130,8 +130,8 @@ impl TryFrom for MempoolError { Some(mempool_error::Error::PreOpGasLimitEfficiencyTooLow(e)) => { MempoolError::PreOpGasLimitEfficiencyTooLow(e.required, e.actual) } - Some(mempool_error::Error::CallGasLimitEfficiencyTooLow(e)) => { - MempoolError::CallGasLimitEfficiencyTooLow(e.required, e.actual) + Some(mempool_error::Error::ExecutionGasLimitEfficiencyTooLow(e)) => { + MempoolError::ExecutionGasLimitEfficiencyTooLow(e.required, e.actual) } None => bail!("unknown proto mempool error"), }) @@ -242,11 +242,13 @@ impl From for ProtoMempoolError { PreOpGasLimitEfficiencyTooLow { required, actual }, )), }, - MempoolError::CallGasLimitEfficiencyTooLow(required, actual) => ProtoMempoolError { - error: Some(mempool_error::Error::CallGasLimitEfficiencyTooLow( - CallGasLimitEfficiencyTooLow { required, actual }, - )), - }, + MempoolError::ExecutionGasLimitEfficiencyTooLow(required, actual) => { + ProtoMempoolError { + error: Some(mempool_error::Error::ExecutionGasLimitEfficiencyTooLow( + ExecutionGasLimitEfficiencyTooLow { required, actual }, + )), + } + } } } } diff --git a/crates/rpc/src/eth/error.rs b/crates/rpc/src/eth/error.rs index f80baa3af..28060a9f1 100644 --- a/crates/rpc/src/eth/error.rs +++ b/crates/rpc/src/eth/error.rs @@ -314,7 +314,7 @@ impl From for EthRpcError { MempoolError::PreOpGasLimitEfficiencyTooLow(_, _) => { Self::InvalidParams(value.to_string()) } - MempoolError::CallGasLimitEfficiencyTooLow(_, _) => { + MempoolError::ExecutionGasLimitEfficiencyTooLow(_, _) => { Self::InvalidParams(value.to_string()) } } diff --git a/crates/sim/src/estimation/v0_6.rs b/crates/sim/src/estimation/v0_6.rs index 4e8398477..87b9f8a85 100644 --- a/crates/sim/src/estimation/v0_6.rs +++ b/crates/sim/src/estimation/v0_6.rs @@ -108,7 +108,7 @@ where op_with_gas.verification_gas_limit = verification_gas_limit; op_with_gas.call_gas_limit = call_gas_limit; // require that this can fit in a bundle of size 1 - let gas_limit = op_with_gas.execution_gas_limit(&self.chain_spec, Some(1)); + let gas_limit = op_with_gas.computation_gas_limit(&self.chain_spec, Some(1)); if gas_limit > self.settings.max_total_execution_gas { return Err(GasEstimationError::GasTotalTooLarge( gas_limit, diff --git a/crates/sim/src/estimation/v0_7.rs b/crates/sim/src/estimation/v0_7.rs index 37803707a..d2bd9419a 100644 --- a/crates/sim/src/estimation/v0_7.rs +++ b/crates/sim/src/estimation/v0_7.rs @@ -124,7 +124,7 @@ where op_with_gas.verification_gas_limit = verification_gas_limit; op_with_gas.paymaster_verification_gas_limit = paymaster_verification_gas_limit; // require that this can fit in a bundle of size 1 - let gas_limit = op_with_gas.execution_gas_limit(&self.chain_spec, Some(1)); + let gas_limit = op_with_gas.computation_gas_limit(&self.chain_spec, Some(1)); if gas_limit > self.settings.max_total_execution_gas { return Err(GasEstimationError::GasTotalTooLarge( gas_limit, @@ -262,11 +262,11 @@ where let get_op_with_limit = |op: UserOperation, args: GetOpWithLimitArgs| { let GetOpWithLimitArgs { gas, fee } = args; + // set call gas to 0 to avoid simulating the call, keep paymasterPostOpGasLimit as is because it is often checked during verification UserOperationBuilder::from_uo(op, &self.chain_spec) .verification_gas_limit(gas) .max_fee_per_gas(fee) .max_priority_fee_per_gas(fee) - .paymaster_post_op_gas_limit(0) .call_gas_limit(0) .build() }; @@ -307,11 +307,11 @@ where let get_op_with_limit = |op: UserOperation, args: GetOpWithLimitArgs| { let GetOpWithLimitArgs { gas, fee } = args; + // set call gas to 0 to avoid simulating the call, keep paymasterPostOpGasLimit as is because it is often checked during verification UserOperationBuilder::from_uo(op, &self.chain_spec) .max_fee_per_gas(fee) .max_priority_fee_per_gas(fee) .paymaster_verification_gas_limit(gas) - .paymaster_post_op_gas_limit(0) .call_gas_limit(0) .build() }; diff --git a/crates/sim/src/gas/oracle.rs b/crates/sim/src/gas/oracle.rs index 0eb7456aa..b77c3f7d0 100644 --- a/crates/sim/src/gas/oracle.rs +++ b/crates/sim/src/gas/oracle.rs @@ -304,7 +304,7 @@ impl<'a> MaxOracle<'a> { } #[async_trait::async_trait] -impl<'a> FeeOracle for MaxOracle<'a> { +impl FeeOracle for MaxOracle<'_> { async fn estimate_priority_fee(&self) -> Result { let futures = self .oracles diff --git a/crates/sim/src/precheck.rs b/crates/sim/src/precheck.rs index 33972e465..f8f7fee81 100644 --- a/crates/sim/src/precheck.rs +++ b/crates/sim/src/precheck.rs @@ -278,7 +278,7 @@ where // Compute the worst case total gas limit by assuming the UO is in its own bundle. // This is conservative and potentially may invalidate some very large UOs that would otherwise be valid. - let gas_limit = op.execution_gas_limit(&self.chain_spec, Some(1)); + let gas_limit = op.computation_gas_limit(&self.chain_spec, Some(1)); if gas_limit > max_total_execution_gas { violations.push(PrecheckViolation::TotalGasLimitTooHigh( gas_limit, diff --git a/crates/types/src/pool/error.rs b/crates/types/src/pool/error.rs index dad9401ff..485fa612a 100644 --- a/crates/types/src/pool/error.rs +++ b/crates/types/src/pool/error.rs @@ -97,9 +97,9 @@ pub enum MempoolError { /// Pre-op gas limit efficiency too low #[error("Pre-op gas limit efficiency too low. Required: {0}, Actual: {1}")] PreOpGasLimitEfficiencyTooLow(f32, f32), - /// Call gas limit efficiency too low - #[error("Call gas limit efficiency too low. Required: {0}, Actual: {1}")] - CallGasLimitEfficiencyTooLow(f32, f32), + /// Execution gas limit efficiency too low + #[error("Execution gas limit efficiency too low. Required: {0}, Actual: {1}")] + ExecutionGasLimitEfficiencyTooLow(f32, f32), } /// Precheck violation enumeration diff --git a/crates/types/src/user_operation/mod.rs b/crates/types/src/user_operation/mod.rs index 9cc7b3411..dd2867437 100644 --- a/crates/types/src/user_operation/mod.rs +++ b/crates/types/src/user_operation/mod.rs @@ -129,21 +129,6 @@ pub trait UserOperation: Debug + Clone + Send + Sync + 'static { /// Returns the heap size of the user operation fn heap_size(&self) -> usize; - /// Returns the total verification gas limit - fn total_verification_gas_limit(&self) -> u128; - - /// Returns the required pre-execution buffer - /// - /// This should capture all of the gas that is needed to execute the user operation, - /// minus the call gas limit. The entry point will check for this buffer before - /// executing the user operation. - fn required_pre_execution_buffer(&self) -> u128; - - /// Returns the limit of gas that may be used used prior to the execution of the user operation - fn pre_op_gas_limit(&self) -> u128 { - self.pre_verification_gas() + self.total_verification_gas_limit() - } - /// Returns the pre-verification gas fn pre_verification_gas(&self) -> u128; @@ -180,7 +165,7 @@ pub trait UserOperation: Debug + Clone + Send + Sync + 'static { /// /// If calculating the gas limit value to put on a bundle transaction, use the gas limit functions. /// If limiting the size of a bundle transaction to adhere to block gas limit, use the execution gas limit functions. - + /// /// Returns the gas limit that applies to bundle's total gas limit /// /// On an L2 this is the total gas limit for the bundle transaction ~including~ any potential DA costs @@ -196,14 +181,14 @@ pub trait UserOperation: Debug + Clone + Send + Sync + 'static { + self.call_gas_limit() } - /// Returns the gas limit that applies to the execution portion of a bundle's gas limit + /// Returns the gas limit that applies to the computation portion of a bundle's gas limit /// /// On an L2 this is the total gas limit for the bundle transaction ~excluding~ any potential DA costs. /// /// This is needed to limit the size of the bundle transaction to adhere to the block gas limit. /// /// `bundle_size` is the size of the bundle if applying shared gas to the gas limit, otherwise `None`. - fn execution_gas_limit(&self, chain_spec: &ChainSpec, bundle_size: Option) -> u128 { + fn computation_gas_limit(&self, chain_spec: &ChainSpec, bundle_size: Option) -> u128 { self.pre_verification_execution_gas_limit(chain_spec, bundle_size) + self.total_verification_gas_limit() + self.required_pre_execution_buffer() @@ -295,6 +280,30 @@ pub trait UserOperation: Debug + Clone + Send + Sync + 'static { self.pre_verification_gas() > self.required_pre_verification_gas(chain_spec, bundle_size, da_gas) } + + /// Returns the total verification gas limit + fn total_verification_gas_limit(&self) -> u128; + + /// Returns the required pre-execution buffer + /// + /// This should capture all of the gas that is needed to execute the user operation, + /// minus the call gas limit. The entry point will check for this buffer before + /// executing the user operation. + fn required_pre_execution_buffer(&self) -> u128; + + /// Returns the limit of gas that may be used used prior to the execution of the user operation + fn pre_op_gas_limit(&self) -> u128 { + self.pre_verification_gas() + self.total_verification_gas_limit() + } + + /// Returns the limit of gas that may be used during the execution of a user operation, including + /// the paymaster post operation + fn execution_gas_limit(&self) -> u128 { + self.call_gas_limit() + self.paymaster_post_op_gas_limit() + } + + /// Returns the limit of gas that may be used during the paymaster post operation + fn paymaster_post_op_gas_limit(&self) -> u128; } /// Returns the total shared gas for a bundle @@ -428,6 +437,13 @@ impl UserOperation for UserOperationVariant { } } + fn paymaster_post_op_gas_limit(&self) -> u128 { + match self { + UserOperationVariant::V0_6(op) => op.paymaster_post_op_gas_limit(), + UserOperationVariant::V0_7(op) => op.paymaster_post_op_gas_limit(), + } + } + fn required_pre_execution_buffer(&self) -> u128 { match self { UserOperationVariant::V0_6(op) => op.required_pre_execution_buffer(), diff --git a/crates/types/src/user_operation/v0_6.rs b/crates/types/src/user_operation/v0_6.rs index bd3296900..5f897d946 100644 --- a/crates/types/src/user_operation/v0_6.rs +++ b/crates/types/src/user_operation/v0_6.rs @@ -243,6 +243,14 @@ impl UserOperationTrait for UserOperation { self.verification_gas_limit * mul } + fn paymaster_post_op_gas_limit(&self) -> u128 { + if self.paymaster().is_some() { + self.verification_gas_limit * 2 + } else { + 0 + } + } + fn required_pre_execution_buffer(&self) -> u128 { self.verification_gas_limit + ENTRY_POINT_INNER_GAS_OVERHEAD } diff --git a/crates/types/src/user_operation/v0_7.rs b/crates/types/src/user_operation/v0_7.rs index ca83c7456..91b3139fa 100644 --- a/crates/types/src/user_operation/v0_7.rs +++ b/crates/types/src/user_operation/v0_7.rs @@ -191,6 +191,10 @@ impl UserOperationTrait for UserOperation { self.verification_gas_limit + self.paymaster_verification_gas_limit } + fn paymaster_post_op_gas_limit(&self) -> u128 { + self.paymaster_post_op_gas_limit + } + fn static_pre_verification_gas(&self, chain_spec: &ChainSpec) -> u128 { self.calldata_gas_cost + chain_spec.per_user_op_v0_7_gas()