Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(gas): amortize evm transaction fixed cost over bundle #442

Merged
merged 3 commits into from
Oct 5, 2023
Merged

fix(gas): amortize evm transaction fixed cost over bundle #442

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d1bfebb
fix(gas): amortize evm transaction fixed cost over bundle
alex-miao Oct 3, 2023
10c13ed
fix(gas): refactor bundle gas calculation
alex-miao Oct 3, 2023
7bb3809
fix(gas): refactor gas overheads
alex-miao Oct 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions crates/builder/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ mockall = {workspace = true, optional = true }
mockall.workspace = true
rundler-pool = { path = "../pool", features = ["test-utils"] }
rundler-provider = { path = "../provider", features = ["test-utils"] }
rundler-sim = { path = "../sim", features = ["test-utils"] }

[build-dependencies]
tonic-build.workspace = true
Expand Down
28 changes: 12 additions & 16 deletions crates/builder/src/bundle_proposer.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,6 @@ use crate::emit::{BuilderEvent, OpRejectionReason, SkipReason};

/// A user op must be valid for at least this long into the future to be included.
const TIME_RANGE_BUFFER: Duration = Duration::from_secs(60);
/// Entrypoint requires a buffer over the user operation gas limits in the bundle transaction
const BUNDLE_TRANSACTION_GAS_OVERHEAD_BUFFER: u64 = 5000;
/// Extra buffer percent to add on the bundle transaction gas estimate to be sure it will be enough
const BUNDLE_TRANSACTION_GAS_OVERHEAD_PERCENT: u64 = 5;

Expand Down Expand Up @@ -538,7 +536,7 @@ where
let mut gas_left = U256::from(self.settings.max_bundle_gas);
let mut ops_in_bundle = Vec::new();
for op in ops {
let gas = gas::user_operation_gas_limit(&op.uo, self.settings.chain_id);
let gas = gas::user_operation_gas_limit(&op.uo, self.settings.chain_id, false);
if gas_left < gas {
self.emit(BuilderEvent::skipped_op(
self.builder_index,
Expand Down Expand Up @@ -739,10 +737,7 @@ impl ProposalContext {
}

fn get_total_gas_limit(&self, chain_id: u64) -> U256 {
self.iter_ops()
.map(|op| gas::user_operation_gas_limit(op, chain_id))
.fold(U256::zero(), |acc, c| acc + c)
+ BUNDLE_TRANSACTION_GAS_OVERHEAD_BUFFER
gas::bundle_gas_limit(self.iter_ops(), chain_id)
}

fn iter_ops_with_simulations(&self) -> impl Iterator<Item = &OpWithSimulation> + '_ {
Expand Down Expand Up @@ -786,7 +781,8 @@ mod tests {
op.pre_verification_gas
+ op.verification_gas_limit * 2
+ op.call_gas_limit
+ BUNDLE_TRANSACTION_GAS_OVERHEAD_BUFFER,
+ U256::from(5_000)
+ U256::from(21_000),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to get these from some common place? Tests will break if we ever modify these values.

These are the sorts of things I want to move to the ChainSpec config class in the future.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to exposing the relevant fields of GasOverheads, it's not great but just avoiding a major refactor for now.

BUNDLE_TRANSACTION_GAS_OVERHEAD_PERCENT,
);

Expand Down Expand Up @@ -909,7 +905,7 @@ mod tests {
let max_priority_fee_per_gas = U256::from(50);
let op1 = op_with_sender_and_fees(address(1), 2054.into(), 54.into());
let op2 = op_with_sender_and_fees(address(2), 2055.into(), 55.into());
let bundle = make_bundle(
let bundle = mock_make_bundle(
vec![
MockOp {
op: op1.clone(),
Expand Down Expand Up @@ -943,7 +939,7 @@ mod tests {
let max_priority_fee_per_gas = U256::from(50);
let op1 = op_with_sender_and_fees(address(1), 1054.into(), 55.into());
let op2 = op_with_sender_and_fees(address(2), 1055.into(), 55.into());
let bundle = make_bundle(
let bundle = mock_make_bundle(
vec![
MockOp {
op: op1.clone(),
Expand Down Expand Up @@ -993,7 +989,7 @@ mod tests {
let op_b_aggregated_sig = 21;
let aggregator_a_signature = 101;
let aggregator_b_signature = 102;
let bundle = make_bundle(
let bundle = mock_make_bundle(
vec![
MockOp {
op: unaggregated_op.clone(),
Expand Down Expand Up @@ -1098,7 +1094,7 @@ mod tests {
let op6 = op_with_sender_factory(address(6), address(4));
let deposit = parse_units("1", "ether").unwrap().into();

let bundle = make_bundle(
let bundle = mock_make_bundle(
vec![
MockOp {
op: op1.clone(),
Expand Down Expand Up @@ -1159,7 +1155,7 @@ mod tests {
let op4 = op_with_sender_call_gas_limit(address(4), U256::from(10_000_000));
let deposit = parse_units("1", "ether").unwrap().into();

let bundle = make_bundle(
let bundle = mock_make_bundle(
vec![
MockOp {
op: op1.clone(),
Expand Down Expand Up @@ -1198,7 +1194,7 @@ mod tests {
assert_eq!(
bundle.gas_estimate,
U256::from(math::increase_by_percent(
10_000_000 + BUNDLE_TRANSACTION_GAS_OVERHEAD_BUFFER,
10_000_000 + 5_000 + 21_000,
BUNDLE_TRANSACTION_GAS_OVERHEAD_PERCENT
))
);
Expand All @@ -1216,7 +1212,7 @@ mod tests {
}

async fn simple_make_bundle(mock_ops: Vec<MockOp>) -> Bundle {
make_bundle(
mock_make_bundle(
mock_ops,
vec![],
vec![HandleOpsOut::Success],
Expand All @@ -1227,7 +1223,7 @@ mod tests {
.await
}

async fn make_bundle(
async fn mock_make_bundle(
mock_ops: Vec<MockOp>,
mock_aggregators: Vec<MockAggregator>,
mock_handle_ops_call_results: Vec<HandleOpsOut>,
Expand Down
49 changes: 37 additions & 12 deletions crates/sim/src/gas/gas.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,23 +33,23 @@ use super::polygon::Polygon;
// see: https://github.com/eth-infinitism/bundler/blob/main/packages/sdk/src/calcPreVerificationGas.ts
#[derive(Clone, Copy, Debug)]
struct GasOverheads {
fixed: U256,
bundle_transaction_gas_buffer: U256,
transaction_gas_overhead: U256,
per_user_op: U256,
per_user_op_word: U256,
zero_byte: U256,
non_zero_byte: U256,
bundle_size: U256,
dancoombs marked this conversation as resolved.
Show resolved Hide resolved
}

impl Default for GasOverheads {
fn default() -> Self {
Self {
fixed: 21000.into(),
per_user_op: 18300.into(),
bundle_transaction_gas_buffer: 5_000.into(), // Entrypoint requires a buffer over the user operation gas limits in the bundle transaction
transaction_gas_overhead: 21_000.into(), // The fixed gas overhead for any EVM transaction
per_user_op: 18_300.into(),
per_user_op_word: 4.into(),
zero_byte: 4.into(),
non_zero_byte: 16.into(),
bundle_size: 1.into(),
}
}
}
Expand All @@ -74,7 +74,7 @@ pub async fn calc_pre_verification_gas<P: Provider>(
provider: Arc<P>,
chain_id: u64,
) -> anyhow::Result<U256> {
let static_gas = calc_static_pre_verification_gas(full_op);
let static_gas = calc_static_pre_verification_gas(full_op, GasOverheads::default(), true);
let dynamic_gas = match chain_id {
_ if ARBITRUM_CHAIN_IDS.contains(&chain_id) => {
provider
Expand All @@ -94,13 +94,31 @@ pub async fn calc_pre_verification_gas<P: Provider>(
Ok(static_gas + dynamic_gas)
}

/// Compute the gas limit for the bundle composed of the given user operations
pub fn bundle_gas_limit<'a, I>(iter_ops: I, chain_id: u64) -> U256
where
I: Iterator<Item = &'a UserOperation>,
{
let ov = GasOverheads::default();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

somewhat unnecessary duplication since we are creating the GasOverheads object both here and within user_operation_gas_limit, but favoring this over needing to make GasOverheads public.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're doing this here, we might as well do the same thing in calc_static_pre_verification_gas?

Either we make it part of both the interfaces, or we instantiate in each.

Again I eventually want these to look like bundle_gas_limit<C: ChainSpec> and then you can get these constants off of the chain spec.

iter_ops
.map(|op| user_operation_gas_limit(op, chain_id, false))
.fold(
ov.bundle_transaction_gas_buffer + ov.transaction_gas_overhead,
|acc, c| acc + c,
)
}

/// Returns the gas limit for the user operation that applies to bundle transaction's limit
pub fn user_operation_gas_limit(uo: &UserOperation, chain_id: u64) -> U256 {
pub fn user_operation_gas_limit(
uo: &UserOperation,
chain_id: u64,
include_fixed_gas_overhead: bool,
) -> U256 {
// On some chains (OP bedrock, Arbitrum) the L1 gas fee is charged via pre_verification_gas
// but this not part of the execution gas limit of the transaction.
// In such cases we only consider the static portion of the pre_verification_gas in the gas limit.
let pvg = if OP_BEDROCK_CHAIN_IDS.contains(&chain_id) | ARBITRUM_CHAIN_IDS.contains(&chain_id) {
calc_static_pre_verification_gas(uo)
calc_static_pre_verification_gas(uo, GasOverheads::default(), include_fixed_gas_overhead)
} else {
uo.pre_verification_gas
};
Expand All @@ -115,8 +133,11 @@ pub fn user_operation_max_gas_cost(uo: &UserOperation) -> U256 {
* (uo.pre_verification_gas + uo.call_gas_limit + uo.verification_gas_limit * mul)
}

fn calc_static_pre_verification_gas(op: &UserOperation) -> U256 {
let ov = GasOverheads::default();
fn calc_static_pre_verification_gas(
op: &UserOperation,
ov: GasOverheads,
include_fixed_gas_overhead: bool,
) -> U256 {
let encoded_op = op.clone().encode();
let length_in_words = encoded_op.len() / 32; // size of packed user op is always a multiple of 32 bytes
let call_data_cost: U256 = encoded_op
Expand All @@ -131,10 +152,14 @@ fn calc_static_pre_verification_gas(op: &UserOperation) -> U256 {
.reduce(|a, b| a + b)
.unwrap_or_default();

ov.fixed / ov.bundle_size
+ call_data_cost
call_data_cost
+ ov.per_user_op
+ ov.per_user_op_word * length_in_words
+ (if include_fixed_gas_overhead {
ov.transaction_gas_overhead
} else {
0.into()
})
}

fn verification_gas_limit_multiplier(uo: &UserOperation) -> u64 {
Expand Down
2 changes: 1 addition & 1 deletion crates/sim/src/precheck.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,7 @@ impl<P: Provider, E: EntryPoint> PrecheckerImpl<P, E> {
max_verification_gas,
));
}
let total_gas_limit = gas::user_operation_gas_limit(op, chain_id);
let total_gas_limit = gas::user_operation_gas_limit(op, chain_id, true);
if total_gas_limit > max_total_execution_gas {
violations.push(PrecheckViolation::TotalGasLimitTooHigh(
total_gas_limit,
Expand Down
Loading