-
Notifications
You must be signed in to change notification settings - Fork 8
Conversation
CodSpeed Performance Report
Merging this PR will not alter performance
Comparing feat/gas_limit (224c40a) with scroll (079764e)
Summary
✅ 77 untouched benchmarks
georgehao
commented
Jan 12, 2026
https://github.com/scroll-tech/reth/actions/runs/20906978938/job/60062238660?pr=374
This is because Bincode is unmaintained https://github.com/paradigmxyz/reth/blob/main/deny.toml#L12
I think can ignore this lint error, wait for upstream merge
@frisitano
frisitano
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! I think we should take this opportunity to do a bit of housekeeping on our block building types.
My proposal is as follows:
Update the ScrollBuilderConfig such that we remove the Option for gas_limit, such that it becomes:
pub struct ScrollBuilderConfig { /// Gas limit. pub gas_limit: u64, /// Time limit for payload building. pub time_limit: Duration, /// Maximum total data availability size for a block. pub max_da_block_size: Option<u64>, }
This removes the need for the unwrap_or_default() which looks a bit scary here:
let desired_gas_limit = self .attributes() .gas_limit .unwrap_or_else(|| builder_config.gas_limit.unwrap_or_default());
Update PayloadBuildingBreaker so the gas limit is no longer optional:
pub struct PayloadBuildingBreaker { start: Instant, time_limit: Duration, gas_limit: u64, max_da_block_size: Option<u64>, }
Introduce a method gas_limit on ScrollPayloadBuilderCtx:
/// Returns the gas limit for the new block. fn gas_limit(&self, builder_gas_limit: u64) -> u64 { let parent_gas_limit = self.parent().gas_limit(); let target_gas_limit = self.attributes().gas_limit.unwrap_or(builder_gas_limit); calculate_block_gas_limit(parent_gas_limit, target_gas_limit) }
Update the method block_builder to be block_builder_with_breaker which returns both a block builder and a breaker:
/// Prepares a [`BlockBuilder`] for the next block. pub fn block_builder_with_breaker<'a, DB: Database>( &'a self, db: &'a mut State<DB>, builder_config: &ScrollBuilderConfig, ) -> Result< (impl BlockBuilder<Primitives = Evm::Primitives> + 'a, PayloadBuildingBreaker), PayloadBuilderError, > { // get the base fee for the attributes. let base_fee_provider = ScrollBaseFeeProvider::new(self.chain_spec.clone()); let base_fee: u64 = base_fee_provider .next_block_base_fee(db, self.parent().header(), self.attributes().timestamp()) .map_err(|err| PayloadBuilderError::Other(Box::new(err)))?; let gas_limit = self.gas_limit(builder_config.gas_limit); let block_builder = self .evm_config .builder_for_next_block( db, self.parent(), ScrollNextBlockEnvAttributes { timestamp: self.attributes().timestamp(), suggested_fee_recipient: self.attributes().suggested_fee_recipient(), gas_limit, base_fee, }, ) .map_err(PayloadBuilderError::other)?; let breaker = PayloadBuildingBreaker::new( builder_config.time_limit, gas_limit, builder_config.max_da_block_size, ); Ok((block_builder, breaker)) }
In ScrollBuilder::build(..) we do:
let (mut builder, breaker) = ctx.block_builder_with_breaker(&mut db, builder_config)?;
to get both the builder and the breaker. We should clean up any unused functions after these changes.
What do you think about this? cc: @Thegaram
Uh oh!
There was an error while loading. Please reload this page.
Fixes #315.