Skip to content

Commit

Permalink
feat(types): improve handling of bundle size in pvg gas limits
Browse files Browse the repository at this point in the history
  • Loading branch information
dancoombs committed Oct 7, 2024
1 parent 1058542 commit f3954e2
Show file tree
Hide file tree
Showing 6 changed files with 106 additions and 47 deletions.
30 changes: 16 additions & 14 deletions crates/builder/src/bundle_proposer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -470,7 +470,7 @@ where
let mut context = ProposalContext::<UO>::new();
let mut paymasters_to_reject = Vec::<EntityInfo>::new();

let mut gas_spent = self.settings.chain_spec.transaction_intrinsic_gas();
let mut gas_spent = rundler_types::bundle_shared_gas(&self.settings.chain_spec);
let mut constructed_bundle_size = BUNDLE_BYTE_OVERHEAD;
for (po, simulation) in ops_with_simulations {
let op = po.clone().uo;
Expand Down Expand Up @@ -525,7 +525,7 @@ 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, false);
let required_gas = gas_spent + op.execution_gas_limit(&self.settings.chain_spec, None);
if required_gas > self.settings.max_bundle_gas {
continue;
}
Expand Down Expand Up @@ -561,7 +561,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, false);
gas_spent += op.execution_gas_limit(&self.settings.chain_spec, None);

constructed_bundle_size =
constructed_bundle_size.saturating_add(op_size_with_offset_word);
Expand Down Expand Up @@ -998,7 +998,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, false);
let gas = op.uo.execution_gas_limit(&self.settings.chain_spec, None);
if gas_left < gas {
self.emit(BuilderEvent::skipped_op(
self.builder_index,
Expand Down Expand Up @@ -1238,9 +1238,9 @@ impl<UO: UserOperation> ProposalContext<UO> {
// needed to have the buffer for each op.

self.iter_ops_with_simulations()
.map(|sim_op| sim_op.op.gas_limit(chain_spec, false))
.map(|sim_op| sim_op.op.gas_limit(chain_spec, None))
.sum::<u128>()
+ chain_spec.transaction_intrinsic_gas()
+ rundler_types::bundle_shared_gas(chain_spec)
}

fn iter_ops_with_simulations(&self) -> impl Iterator<Item = &OpWithSimulation<UO>> + '_ {
Expand Down Expand Up @@ -1441,7 +1441,7 @@ mod tests {
let cs = ChainSpec::default();

let expected_gas: u64 = math::increase_by_percent(
op.gas_limit(&cs, true),
op.gas_limit(&cs, Some(1)),
BUNDLE_TRANSACTION_GAS_OVERHEAD_PERCENT,
)
.try_into()
Expand Down Expand Up @@ -1925,9 +1925,9 @@ mod tests {
.await;

let cs = ChainSpec::default();
let expected_gas_limit: u64 = (op1.gas_limit(&cs, false)
+ op2.gas_limit(&cs, false)
+ cs.transaction_intrinsic_gas())
let expected_gas_limit: u64 = (op1.gas_limit(&cs, None)
+ op2.gas_limit(&cs, None)
+ rundler_types::bundle_shared_gas(&cs))
.try_into()
.unwrap();

Expand Down Expand Up @@ -1980,8 +1980,9 @@ mod tests {
entity_updates: BTreeMap::new(),
};

let expected_gas_limit =
op1.gas_limit(&cs, false) + op2.gas_limit(&cs, false) + cs.transaction_intrinsic_gas();
let expected_gas_limit = op1.gas_limit(&cs, None)
+ op2.gas_limit(&cs, None)
+ rundler_types::bundle_shared_gas(&cs);

assert_eq!(context.get_bundle_gas_limit(&cs), expected_gas_limit);
}
Expand Down Expand Up @@ -2021,8 +2022,9 @@ mod tests {
};
let gas_limit = context.get_bundle_gas_limit(&cs);

let expected_gas_limit =
op1.gas_limit(&cs, false) + op2.gas_limit(&cs, false) + cs.transaction_intrinsic_gas();
let expected_gas_limit = op1.gas_limit(&cs, None)
+ op2.gas_limit(&cs, None)
+ rundler_types::bundle_shared_gas(&cs);

assert_eq!(gas_limit, expected_gas_limit);
}
Expand Down
3 changes: 2 additions & 1 deletion crates/sim/src/estimation/v0_6.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,8 @@ where
let mut op_with_gas = full_op;
op_with_gas.verification_gas_limit = verification_gas_limit;
op_with_gas.call_gas_limit = call_gas_limit;
let gas_limit = op_with_gas.execution_gas_limit(&self.chain_spec, true);
// 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));
if gas_limit > self.settings.max_total_execution_gas {
return Err(GasEstimationError::GasTotalTooLarge(
gas_limit,
Expand Down
3 changes: 2 additions & 1 deletion crates/sim/src/estimation/v0_7.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,8 @@ where
op_with_gas.call_gas_limit = call_gas_limit;
op_with_gas.verification_gas_limit = verification_gas_limit;
op_with_gas.paymaster_verification_gas_limit = paymaster_verification_gas_limit;
let gas_limit = op_with_gas.execution_gas_limit(&self.chain_spec, true);
// 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));
if gas_limit > self.settings.max_total_execution_gas {
return Err(GasEstimationError::GasTotalTooLarge(
gas_limit,
Expand Down
16 changes: 4 additions & 12 deletions crates/sim/src/gas/gas.rs
Original file line number Diff line number Diff line change
Expand Up @@ -46,11 +46,6 @@ pub async fn estimate_pre_verification_gas<
random_op: &UO,
gas_price: u128,
) -> anyhow::Result<u128> {
let static_gas = full_op.static_pre_verification_gas(chain_spec);

// Currently assume 1 op bundle
let shared_gas = chain_spec.transaction_intrinsic_gas();

let da_gas = if chain_spec.da_pre_verification_gas {
entry_point
.calc_da_gas(*entry_point.address(), random_op.clone(), gas_price)
Expand All @@ -59,7 +54,8 @@ pub async fn estimate_pre_verification_gas<
0
};

Ok(static_gas.saturating_add(shared_gas).saturating_add(da_gas))
// Currently assume 1 op bundle
Ok(full_op.required_pre_verification_gas(chain_spec, 1, da_gas))
}

/// Calculate the required pre_verification_gas for the given user operation and the provided base fee.
Expand All @@ -74,11 +70,6 @@ pub async fn calc_required_pre_verification_gas<
op: &UO,
base_fee: u128,
) -> anyhow::Result<u128> {
let static_gas = op.static_pre_verification_gas(chain_spec);

// Currently assume 1 op bundle
let shared_gas = chain_spec.transaction_intrinsic_gas();

let da_gas = if chain_spec.da_pre_verification_gas {
let gas_price = cmp::min(
base_fee + op.max_priority_fee_per_gas(),
Expand All @@ -96,7 +87,8 @@ pub async fn calc_required_pre_verification_gas<
0
};

Ok(static_gas.saturating_add(shared_gas).saturating_add(da_gas))
// Currently assume 1 op bundle
Ok(op.required_pre_verification_gas(chain_spec, 1, da_gas))
}

/// Different modes for calculating the required priority fee
Expand Down
6 changes: 3 additions & 3 deletions crates/sim/src/precheck.rs
Original file line number Diff line number Diff line change
Expand Up @@ -240,9 +240,9 @@ where
));
}

// compute the worst case total gas limit by assuming the UO is in its own bundle and has a postOp call.
// 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, true);
let gas_limit = op.execution_gas_limit(&self.chain_spec, Some(1));
if gas_limit > max_total_execution_gas {
violations.push(PrecheckViolation::TotalGasLimitTooHigh(
gas_limit,
Expand Down Expand Up @@ -523,7 +523,7 @@ mod tests {

let res = prechecker.check_gas(&op, get_test_async_data());

let total_gas_limit = op.gas_limit(&cs, true);
let total_gas_limit = op.gas_limit(&cs, Some(1));

assert_eq!(
res,
Expand Down
95 changes: 79 additions & 16 deletions crates/types/src/user_operation/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -176,8 +176,10 @@ pub trait UserOperation: Debug + Clone + Send + Sync + 'static {
/// if the chain requires it.
///
/// This is needed to set the gas limit for the bundle transaction.
fn gas_limit(&self, chain_spec: &ChainSpec, include_intrinsic_gas: bool) -> u128 {
self.pre_verification_gas_limit(chain_spec, include_intrinsic_gas)
///
/// `bundle_size` is the size of the bundle if applying shared gas to the gas limit, otherwise `None`.
fn gas_limit(&self, chain_spec: &ChainSpec, bundle_size: Option<usize>) -> u128 {
self.pre_verification_gas_limit(chain_spec, bundle_size)
+ self.total_verification_gas_limit()
+ self.required_pre_execution_buffer()
+ self.call_gas_limit()
Expand All @@ -188,8 +190,10 @@ pub trait UserOperation: Debug + Clone + Send + Sync + 'static {
/// 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.
fn execution_gas_limit(&self, chain_spec: &ChainSpec, include_intrinsic_gas: bool) -> u128 {
self.pre_verification_execution_gas_limit(chain_spec, include_intrinsic_gas)
///
/// `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 {
self.pre_verification_execution_gas_limit(chain_spec, bundle_size)
+ self.total_verification_gas_limit()
+ self.required_pre_execution_buffer()
+ self.call_gas_limit()
Expand All @@ -198,15 +202,21 @@ pub trait UserOperation: Debug + Clone + Send + Sync + 'static {
/// Returns the portion of pre-verification gas the applies to the DA portion of a bundle's gas limit
///
/// On an L2 this is the portion of the pre_verification_gas that is due to DA costs
fn pre_verification_da_gas_limit(&self, chain_spec: &ChainSpec) -> u128 {
///
/// `bundle_size` is the size of the bundle if applying shared gas to the gas limit, otherwise `None`.
fn pre_verification_da_gas_limit(
&self,
chain_spec: &ChainSpec,
bundle_size: Option<usize>,
) -> u128 {
// On some chains (OP bedrock) the DA gas fee is charged via pre_verification_gas
// but this not part of the gas limit of the transaction.
//
// On other chains (Arbitrum), the DA portion IS charged in the gas limit, so calculate it here.
if chain_spec.da_pre_verification_gas && chain_spec.include_da_gas_in_gas_limit {
self.pre_verification_gas()
.saturating_sub(self.static_pre_verification_gas(chain_spec))
.saturating_sub(chain_spec.transaction_intrinsic_gas())
.saturating_sub(optional_bundle_per_uo_shared_gas(chain_spec, bundle_size))
} else {
0
}
Expand All @@ -215,35 +225,88 @@ pub trait UserOperation: Debug + Clone + Send + Sync + 'static {
/// Returns the portion of pre-verification gas the applies to the execution 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
///
/// `bundle_size` is the size of the bundle if applying shared gas to the gas limit, otherwise `None`.
fn pre_verification_execution_gas_limit(
&self,
chain_spec: &ChainSpec,
include_intrinsic_gas: bool,
bundle_size: Option<usize>,
) -> u128 {
// On some chains (OP bedrock, Arbitrum) the DA gas fee is charged via pre_verification_gas
// but this not part of the EXECUTION gas limit of the transaction.
//
// On other chains, the DA portion is zero.
//
// Thus, only consider the static portion of the pre_verification_gas in the gas limit.
let static_pvg = self.static_pre_verification_gas(chain_spec);
if include_intrinsic_gas {
static_pvg.saturating_add(chain_spec.transaction_intrinsic_gas())
} else {
static_pvg
}
self.static_pre_verification_gas(chain_spec)
.saturating_add(optional_bundle_per_uo_shared_gas(chain_spec, bundle_size))
}

/// Returns the portion of pre-verification gas that applies to a bundle's total gas limit
///
/// On an L2 this is the total gas limit for the bundle transaction ~including~ any potential DA costs
///
/// `bundle_size` is the size of the bundle if applying shared gas to the gas limit, otherwise `None`.
fn pre_verification_gas_limit(
&self,
chain_spec: &ChainSpec,
include_intrinsic_gas: bool,
bundle_size: Option<usize>,
) -> u128 {
self.pre_verification_execution_gas_limit(chain_spec, include_intrinsic_gas)
+ self.pre_verification_da_gas_limit(chain_spec)
self.pre_verification_execution_gas_limit(chain_spec, bundle_size)
+ self.pre_verification_da_gas_limit(chain_spec, bundle_size)
}

/// Returns the required pre-verification gas for the given user operation
///
/// `bundle_size` is the size of the bundle
/// `da_gas` is the DA gas cost for the user operation, calculated elsewhere
fn required_pre_verification_gas(
&self,
chain_spec: &ChainSpec,
bundle_size: usize,
da_gas: u128,
) -> u128 {
self.static_pre_verification_gas(chain_spec)
.saturating_add(bundle_per_uo_shared_gas(chain_spec, bundle_size))
.saturating_add(da_gas)
}

/// Returns true if the user operation has enough pre-verification gas to be included in a bundle
///
/// `bundle_size` is the size of the bundle
/// `da_gas` is the DA gas cost for the user operation, calculated elsewhere
fn has_required_pre_verification_gas(
&self,
chain_spec: &ChainSpec,
bundle_size: usize,
da_gas: u128,
) -> bool {
self.pre_verification_gas()
> self.required_pre_verification_gas(chain_spec, bundle_size, da_gas)
}
}

/// Returns the total shared gas for a bundle
pub fn bundle_shared_gas(chain_spec: &ChainSpec) -> u128 {
chain_spec.transaction_intrinsic_gas()
}

/// Returns the shared gas per user operation for a given bundle size
///
/// `bundle_size` is the size of the bundle if applying shared gas to the gas limit, otherwise `None`.
pub fn bundle_per_uo_shared_gas(chain_spec: &ChainSpec, bundle_size: usize) -> u128 {
if bundle_size == 0 {
0
} else {
bundle_shared_gas(chain_spec) / bundle_size as u128
}
}

fn optional_bundle_per_uo_shared_gas(chain_spec: &ChainSpec, bundle_size: Option<usize>) -> u128 {
if let Some(bundle_size) = bundle_size {
bundle_per_uo_shared_gas(chain_spec, bundle_size)
} else {
0
}
}

Expand Down

0 comments on commit f3954e2

Please sign in to comment.