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.

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(builder): filter fee before filtering by gas limit #707

Merged
merged 1 commit into from
May 16, 2024
Merged
Changes from all commits
Commits
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
95 changes: 61 additions & 34 deletions crates/builder/src/bundle_proposer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -154,18 +154,9 @@ where
self.fee_estimator.required_bundle_fees(required_fees)
)?;

// Limit the amount of gas in the bundle
tracing::debug!("Starting bundle proposal with {} ops", ops.len(),);
tracing::info!("Starting bundle proposal with {} ops", ops.len());

// Do an initial filtering of ops that we want to simulate.
let (ops, gas_limit) = self.limit_user_operations_for_simulation(ops);
tracing::debug!(
"Bundle proposal after limit had {} ops and {:?} gas limit",
ops.len(),
gas_limit
);

// Determine fees required for ops to be included in a bundle
// (0) Determine fees required for ops to be included in a bundle
// if replacing, just require bundle fees increase chances of unsticking
let required_op_fees = if is_replacement {
bundle_fees
Expand All @@ -177,10 +168,32 @@ where
.filter_map(|op| op.uo.paymaster())
.collect::<Vec<Address>>();

// Filter ops and simulate
// (1) Filter out ops that don't pay enough to be included
let fee_futs = ops
.into_iter()
.map(|op| self.check_fees(op, base_fee, required_op_fees))
.collect::<Vec<_>>();
let ops = future::join_all(fee_futs)
.await
.into_iter()
.flatten()
.collect::<Vec<_>>();

tracing::info!("Bundle proposal after fee limit had {} ops", ops.len());

// (2) Limit the amount of operations for simulation
let (ops, gas_limit) = self.limit_user_operations_for_simulation(ops);

tracing::info!(
"Bundle proposal after gas limit had {} ops and {:?} gas limit",
ops.len(),
gas_limit
);

// (3) simulate ops
let simulation_futures = ops
.into_iter()
.map(|op| self.filter_and_simulate(op, block_hash, base_fee, required_op_fees))
.map(|op| self.simulate_op(op, block_hash))
.collect::<Vec<_>>();

let ops_with_simulations_future = future::join_all(simulation_futures);
Expand Down Expand Up @@ -266,19 +279,17 @@ where
}
}

// Filter and simulate a single op. Returns None if the op should be skipped.
// Check fees for a single user op. Returns None if the op should be skipped.
//
// Filters on:
// - gas fees
// - pre-verification gas
// - any errors
async fn filter_and_simulate(
// - insufficient gas fees
// - insufficient pre-verification gas
async fn check_fees(
&self,
op: PoolOperation,
block_hash: H256,
base_fee: U256,
required_op_fees: GasFees,
) -> Option<(PoolOperation, Result<SimulationResult, SimulationError>)> {
) -> Option<PoolOperation> {
let op_hash = self.op_hash(&op.uo);

// filter by fees
Expand All @@ -300,26 +311,29 @@ where
}

// Check if the pvg is enough
let required_pvg = gas::calc_required_pre_verification_gas(
let required_pvg = match gas::calc_required_pre_verification_gas(
&self.settings.chain_spec,
&self.entry_point,
op.uo.as_ref(),
base_fee,
)
.await
.map_err(|e| {
self.emit(BuilderEvent::skipped_op(
self.builder_index,
op_hash,
SkipReason::Other {
reason: Arc::new(format!(
"Failed to calculate required pre-verification gas for op: {e:?}, skipping"
)),
},
));
e
})
.ok()?;
{
Ok(pvg) => pvg,
Err(e) => {
error!("Failed to calculate required pre-verification gas for op: {e:?}, skipping");
self.emit(BuilderEvent::skipped_op(
self.builder_index,
op_hash,
SkipReason::Other {
reason: Arc::new(format!(
"Failed to calculate required pre-verification gas for op: {e:?}, skipping"
)),
},
));
return None;
}
};

if op.uo.pre_verification_gas() < required_pvg {
self.emit(BuilderEvent::skipped_op(
Expand All @@ -338,6 +352,19 @@ where
return None;
}

Some(op)
}

// Simulate a single op. Returns None if the op should be skipped.
//
// Filters on any errors
async fn simulate_op(
&self,
op: PoolOperation,
block_hash: H256,
) -> Option<(PoolOperation, Result<SimulationResult, SimulationError>)> {
let op_hash = self.op_hash(&op.uo);

// Simulate
let result = self
.simulator
Expand Down
Loading