Skip to content

fix: wasn't able to decode Electra Era BeaconBlock + invalid request_hash() generated#1820

Merged
KolbyML merged 2 commits into
ethereum:masterfrom
KolbyML:fix-reading-e2store-files
May 13, 2025
Merged

fix: wasn't able to decode Electra Era BeaconBlock + invalid request_hash() generated#1820
KolbyML merged 2 commits into
ethereum:masterfrom
KolbyML:fix-reading-e2store-files

Conversation

@KolbyML
Copy link
Copy Markdown
Member

@KolbyML KolbyML commented May 13, 2025

What was wrong?

I was trying to generate E2HS files from ElectraBeaconBlock's and

  • e2store era reader was failing to decode ElectraBeaconBlock
  • We were generating the request_hash() wrong.

How was it fixed?

  • I added Electra to get_beacon_fork, in the future we should use NetworkSpec instead, but NetworkSpec currently doesn't store CL fork data (it will in the future) so we can update that code then
  • apparently for request_hash() you are supposed to convert the whole list of X objects to ssz, not do it individually

After I added these fixes I was able to generate E2HS files from ElectraBeaconBlocks.

I added a test case for requests_hash() to prevent regressions.

@KolbyML KolbyML requested a review from morph-dev May 13, 2025 03:46
@KolbyML KolbyML self-assigned this May 13, 2025
Copy link
Copy Markdown
Collaborator

@morph-dev morph-dev left a comment

Choose a reason for hiding this comment

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

... but NetworkSpec currently doesn't store CL fork data (it will in the future) so we can update that code then

This is not true. We use CL fork data in several places already. In this case, we could do something like this:

network_spec().is_ethereum_fork_active_at_timestamp(
    EthereumHardfork::Prague,
    network_spec().slot_to_timestamp(slot),
)

Probably better option would be to add is_ethereum_fork_active_at_slot directly inside NetworkSpec.

I don't think that this should block this PR, I just wanted to point out that it's possible at the moment, just not very elegant.

use crate::consensus::{pubkey::PubKey, signature::BlsSignature};

#[test]
fn test_requests_hash() {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I tried, and this specific test would pass with the old code, so it doesn't really test the regression that well :)

I think it would be better if you would use the real mainnet data (the one that it failed for). You would of course mention the slot / block number, so it doesn't appear just like random data.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't think real test data matters in this case, but sure I can use real testdata

@KolbyML
Copy link
Copy Markdown
Member Author

KolbyML commented May 13, 2025

... but NetworkSpec currently doesn't store CL fork data (it will in the future) so we can update that code then

This is not true. We use CL fork data in several places already. In this case, we could do something like this:

network_spec().is_ethereum_fork_active_at_timestamp(
    EthereumHardfork::Prague,
    network_spec().slot_to_timestamp(slot),
)

Probably better option would be to add is_ethereum_fork_active_at_slot directly inside NetworkSpec.

I don't think that this should block this PR, I just wanted to point out that it's possible at the moment, just not very elegant.

image

Not all CL forks happened at the same time as EL forks, so the idea above only works for the last 2 CL forks, but there is no guarantee it will work in the future as well. To do this properly we need to explicitly load the CL fork data into the NetworkSpec

@KolbyML KolbyML merged commit e655405 into ethereum:master May 13, 2025
14 checks passed
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.

2 participants