Skip to content

Conversation

@onbjerg
Copy link
Contributor

@onbjerg onbjerg commented May 2, 2024

Adds EIP-7685 header fields to both the consensus and RPC Header types.

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

@onbjerg onbjerg added the enhancement New feature or request label May 2, 2024
@onbjerg onbjerg marked this pull request as ready for review May 2, 2024 16:27
Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all of this looks correct, this basically shifts replaces the parent beacon root as the last entry

pending @Rjected

I assume we also need additional checks in the consensus impl?

@onbjerg
Copy link
Contributor Author

onbjerg commented May 2, 2024

@mattsse yeah i'm opening PRs as I progress on paradigmxyz/reth#8053, we need to add some fields to body + calc the root itself, will keep the alloy impl on par

@prestwich
Copy link
Member

should we be managing changes in e.g. an enum over versions?

@mattsse
Copy link
Member

mattsse commented May 2, 2024

should we be managing changes in e.g. an enum over versions?

while this would have advantages, we'd still need this type for decoding any rlp blob or we need additional context for decoding, like the hardfork so we know how to decode

@prestwich
Copy link
Member

great i love it when field semantics change without any structural indication 🎉

Copy link
Contributor

@Rjected Rjected left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Comment on lines +299 to +309
// Encode requests root length.
//
// If new fields are added, the above pattern will
// need to be repeated and placeholder length added. Otherwise, it's impossible to
// tell _which_ fields are missing. This is mainly relevant for contrived cases
// where a header is created at random, for example:
// * A header is created with a withdrawals root, but no base fee. Shanghai blocks are
// post-London, so this is technically not valid. However, a tool like proptest would
// generate a block like this.
if let Some(parent_beacon_block_root) = self.parent_beacon_block_root {
length += parent_beacon_block_root.length();
if let Some(requests_root) = self.requests_root {
length += requests_root.length();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note: the hack in this comment is not needed in reth any more, the arbitrary impl was changed instead, we should copy it over so the encoding / decoding impl is more sensible

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will do

@prestwich prestwich merged commit 9922f8d into main May 4, 2024
@prestwich prestwich deleted the onbjerg/eip-7685-header branch May 4, 2024 13:32
ben186 pushed a commit to ben186/alloy that referenced this pull request Jul 27, 2024
* feat: add eip-7685 requests root to header

* feat: requests root for consensus header

* chore: fmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants