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

consensus: implement validate_against_parent_4844 for Header #1572

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

tcoratger
Copy link
Contributor

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 dedicated ConsensusError type in alloy.

/// 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> {
Copy link
Contributor Author

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

Comment on lines +33 to +36
/// Expected excess blob gas.
expected: u64,
/// Actual excess blob gas.
got: u64,
Copy link
Contributor Author

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

https://github.com/paradigmxyz/reth/blob/ab07fcfb113cb0b579e1f9f55a5dd9511b576687/crates/primitives-traits/src/error.rs#L9

into alloy-primitives to be able to use it everywhere

@@ -1 +1 @@
msrv = "1.79"
msrv = "1.81"
Copy link
Contributor Author

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

@mattsse mattsse requested a review from klkvr as a code owner December 27, 2024 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant