-
Notifications
You must be signed in to change notification settings - Fork 249
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
consensus: implement validate_against_parent_4844
for Header
#1572
base: main
Are you sure you want to change the base?
Conversation
/// 1. The current block header contains the `blob_gas_used` and `excess_blob_gas` fields. | ||
/// 2. The `excess_blob_gas` field matches the expected value calculated from the parent header | ||
/// fields. | ||
pub fn validate_against_parent_4844(&self, parent: &Self) -> Result<(), ConsensusError> { |
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.
Wanted to create a follow up PR with unit tests to validate all the scenarios, but first I wanted to know if this proposal of migration would be accepted
/// Expected excess blob gas. | ||
expected: u64, | ||
/// Actual excess blob gas. | ||
got: u64, |
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.
I've seen several places in alloy where the got/expected error pattern is present, maybe it would be good to move the reth primitive
into alloy-primitives to be able to use it everywhere
@@ -1 +1 @@ | |||
msrv = "1.79" | |||
msrv = "1.81" |
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.
I had to bump Rust due to the derive_more error stuff. I think this goes against
Alloy will keep a rolling MSRV policy of at least two versions behind the latest stable release (so if the latest stable release is 1.58, we would support 1.56).
I think that Rust 1.81 is also needed for #1552 so maybe we should wait depending on the prio level
Right now in Reth, we have a
validate_against_parent_4844
that is implemented inside the consensus crate.https://github.com/paradigmxyz/reth/blob/1bdf429af5092a0b737e900977c1418bc070ec59/crates/consensus/common/src/validation.rs#L245-L274
Since it affects consensus and applies to the
Header
, it seems to me that it makes sense to move it here (I don't know if this proposal will be accepted). So I also created a dedicatedConsensusError
type in alloy.