-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat: add support for eip-7872 Max blob flag for local builders #19614
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
Conversation
mattsse
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.
cool, smol nits
crates/ethereum/payload/src/lib.rs
Outdated
| let protocol_max_blob_count = | ||
| blob_params.as_ref().map(|params| params.max_blob_count).unwrap_or_default(); |
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.
can we move this into unwrap_or_else
|
|
||
| /// Maximum number of blobs to include per block. | ||
| #[arg(long = "builder.max-blobs", value_name = "COUNT")] | ||
| pub max_blobs_per_block: Option<u8>, |
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.
this will (hopefully) eventually exceed 256, so we can just make this a u64 instead, then we can avoid type casts
crates/ethereum/payload/src/lib.rs
Outdated
| // Apply user-configured blob limit (EIP-7872) | ||
| let max_blob_count = builder_config | ||
| .max_blobs_per_block | ||
| .map(|user_limit| std::cmp::min(user_limit as u64, protocol_max_blob_count).max(1)) |
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.
is max(1) part of the eip? that this can't be lower than that?
1 doesnt really make much sense really, so imo 0 is also a valid value here
mattsse
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.
lgtm!
closes #19613