Skip to content

Commit

Permalink
fix: fix multiple paymaster post op bugs
Browse files Browse the repository at this point in the history
- Don't zero out paymaster post op gas limit during gas estimation
- Encorporate paymaster post op gas limit during efficiency check
  • Loading branch information
dancoombs authored and andysim3d committed Jan 17, 2025
1 parent 16d8dc3 commit e94908d
Show file tree
Hide file tree
Showing 14 changed files with 92 additions and 58 deletions.
7 changes: 4 additions & 3 deletions crates/builder/src/bundle_proposer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions crates/pool/proto/op_pool/op_pool.proto
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down Expand Up @@ -570,7 +570,7 @@ message PreOpGasLimitEfficiencyTooLow {
float actual = 2;
}

message CallGasLimitEfficiencyTooLow {
message ExecutionGasLimitEfficiencyTooLow {
float required = 1;
float actual = 2;
}
Expand Down
4 changes: 2 additions & 2 deletions crates/pool/src/mempool/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Arc<PoolOperation>>;

/// 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);

Expand Down
27 changes: 15 additions & 12 deletions crates/pool/src/mempool/uo_pool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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.
}

Expand Down Expand Up @@ -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,
));
}
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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"),
}
}

Expand Down
24 changes: 13 additions & 11 deletions crates/pool/src/server/remote/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -130,8 +130,8 @@ impl TryFrom<ProtoMempoolError> 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"),
})
Expand Down Expand Up @@ -242,11 +242,13 @@ impl From<MempoolError> 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 },
)),
}
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/rpc/src/eth/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ impl From<MempoolError> for EthRpcError {
MempoolError::PreOpGasLimitEfficiencyTooLow(_, _) => {
Self::InvalidParams(value.to_string())
}
MempoolError::CallGasLimitEfficiencyTooLow(_, _) => {
MempoolError::ExecutionGasLimitEfficiencyTooLow(_, _) => {
Self::InvalidParams(value.to_string())
}
}
Expand Down
2 changes: 1 addition & 1 deletion crates/sim/src/estimation/v0_6.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
6 changes: 3 additions & 3 deletions crates/sim/src/estimation/v0_7.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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()
};
Expand Down Expand Up @@ -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()
};
Expand Down
2 changes: 1 addition & 1 deletion crates/sim/src/gas/oracle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<u128> {
let futures = self
.oracles
Expand Down
2 changes: 1 addition & 1 deletion crates/sim/src/precheck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
6 changes: 3 additions & 3 deletions crates/types/src/pool/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
52 changes: 34 additions & 18 deletions crates/types/src/user_operation/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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
Expand All @@ -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<usize>) -> u128 {
fn computation_gas_limit(&self, chain_spec: &ChainSpec, bundle_size: Option<usize>) -> u128 {
self.pre_verification_execution_gas_limit(chain_spec, bundle_size)
+ self.total_verification_gas_limit()
+ self.required_pre_execution_buffer()
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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(),
Expand Down
8 changes: 8 additions & 0 deletions crates/types/src/user_operation/v0_6.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
4 changes: 4 additions & 0 deletions crates/types/src/user_operation/v0_7.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down

0 comments on commit e94908d

Please sign in to comment.