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

validate EL block hash when running consensus block tests #6406

Merged
merged 20 commits into from
Jan 10, 2025

Conversation

etan-status
Copy link
Contributor

We currently don't have an easy way to test EL block hash computation. As the EL block hash in consensus-spec-tests is computed correctly, update the test runners that load block from test files to also verify the EL block hash. This increases missing test coverage.

Requires ethereum/consensus-specs#3829

We currently don't have an easy way to test EL block hash computation.
As the EL block hash in consensus-spec-tests is computed correctly,
update the test runners that load block from test files to also verify
the EL block hash. This increases missing test coverage.

Requires ethereum/consensus-specs#3829
Copy link

github-actions bot commented Jul 3, 2024

Unit Test Results

0 tests   0 ✔️  0s ⏱️
0 suites  0 💤
0 files    0

Results for commit 5acdbf8.

♻️ This comment has been updated with latest results.

@etan-status
Copy link
Contributor Author

Cross-checked against jtraglia/consensus-specs#2 (with #6718)

@etan-status
Copy link
Contributor Author

The hash is incorrect when there are empty txes, see status-im/nim-eth#744 (comment)

As such blocks are invalid anyway as of ethereum/consensus-specs#3885, have excluded that condition from block hash consistency check.

@etan-status
Copy link
Contributor Author

alpha.9 --> used to work but new tests got added after the latest fixes got merged.

@tersec
Copy link
Contributor

tersec commented Jan 3, 2025

Still in alpha.10:

EF - Electra - Finality  [Preset: minimal] ..... (0.42s)
Invalid `block_hash`: /github-runner/github-runner-node-01/workspace/nimbus-eth2/nimbus-eth2/vendor/nim-eth2-scenarios/tests-v1.5.0-alpha.10/minimal/electra/random/random/pyspec_tests/randomized_6/blocks_0.ssz_snappy
EF - Electra - Random  [Preset: minimal]  (0.00s)

@etan-status
Copy link
Contributor Author

Yes, they keep adding breakages after the fixes :)

@etan-status
Copy link
Contributor Author

@etan-status
Copy link
Contributor Author

Checked it against alpha.10 + the latest fix PR and nimbus passes with that. As long as no new incorrect block hashes get added by other PRs it should once more be alright (same as we hoped for during the earlier alphas which all worked against nimbus at a time).

@tersec tersec enabled auto-merge (squash) January 10, 2025 12:12
@tersec tersec merged commit c63862c into unstable Jan 10, 2025
11 checks passed
@tersec tersec deleted the dev/etan/ct-elblockhash branch January 10, 2025 13:34
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