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

Update compute_el_header_block_hash for EIP-7685 #3778

Merged
merged 7 commits into from
Jun 12, 2024

Conversation

jtraglia
Copy link
Member

Reference: https://eips.ethereum.org/EIPS/eip-7685

This PR does the following:

  • Consolidate deposit_receipts and withdrawal_requests into requests.
  • When encoding those two types, prefix with type (0x00 and 0x01).
  • Encode with non-null parent_beacon_root.
    • A null parent_beacon_root causes issues when requests_root is not null.
    • I believe this field was accidentally overlooked before.
  • Set payload.withdrawal_requests = [] in build_empty_execution_payload.

I noticed these issues when looking into why the block hash differed from Geth.

@jtraglia
Copy link
Member Author

@lightclient I would appreciate your review but I can't select you as a reviewer.

Copy link
Collaborator

@mkalinin mkalinin left a comment

Choose a reason for hiding this comment

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

The logic looks correct to me. One nit: get_deposit_receipt_rlp (same with withdrawal requests) returns Request encoding which is not an RLP because of the type byte at the beginning, consider renaming these methods to reflect their actual semantics.

@jtraglia
Copy link
Member Author

consider renaming these methods to reflect their actual semantics.

Ah yes, you're right. Thanks for pointing this out! I took this approach.

@hwwhww hwwhww mentioned this pull request Jun 4, 2024
10 tasks
@hwwhww
Copy link
Contributor

hwwhww commented Jun 4, 2024

Note: if #3757 gets merged, we should rename it in this PR too.

Copy link
Contributor

@hwwhww hwwhww left a comment

Choose a reason for hiding this comment

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

@jtraglia I fixed the conflicts and added lines for ConsolidationRequest. Could you take a look?

@jtraglia
Copy link
Member Author

jtraglia commented Jun 10, 2024

@jtraglia I fixed the conflicts and added lines for ConsolidationRequest. Could you take a look?

@hwwhww So get_withdrawal_request_rlp_bytes and get_consolidation_request_rlp_bytes are both prefixed with 0x01 now, these should be different. And get_deposit_request_rlp_bytes doesn't have a prefix byte anymore.

I'm guessing deposit_requests=0, withdrawal_requests=1, and consolidation_requests=2 but I'm not sure.

@hwwhww
Copy link
Contributor

hwwhww commented Jun 12, 2024

@jtraglia you're right! I forget to update the prefix when copy-pasting.

@hwwhww hwwhww merged commit a24837b into ethereum:dev Jun 12, 2024
26 checks passed
etan-status added a commit to etan-status/consensus-specs that referenced this pull request Jul 2, 2024
The parent beacon block hash was incorrectly set to zero in ethereum#3778.
Passing the state to the computation function allows correct hash
computation.
@jtraglia jtraglia deleted the electra-block-hash branch July 2, 2024 21:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants