-
Notifications
You must be signed in to change notification settings - Fork 7
feat: Implement gradual gas limit increase #374
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
base: scroll
Are you sure you want to change the base?
Conversation
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
|
|
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
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
Fixes #315.