Skip to content

test: integrate fork choice test vectors#1314

Open
unnawut wants to merge 7 commits intoReamLabs:masterfrom
unnawut:fork-choice-test-vectors
Open

test: integrate fork choice test vectors#1314
unnawut wants to merge 7 commits intoReamLabs:masterfrom
unnawut:fork-choice-test-vectors

Conversation

@unnawut
Copy link
Contributor

@unnawut unnawut commented Mar 18, 2026

What was wrong? & How was it fixed?

I just spotted that we have the fork choice test vectors implemented but they're not being run. After enabling them there were 3 main groups of failures:

  1. Fork choice tie-breaking should be based on voting weight then the lexical order of the hash. But the code uses (vote_weight, slot, *child_hash) so it's ordered by weight, then slot number first before the hash. In test cases where it intentionally puts 2 blocks of equal votes, but the block at lower slot has a higher lexical block, the test failed. --> This is fixed by removing slot from the tuple.
  2. When a new block is processed by on_block(), the fork choice store collects the attestation data from block.body.attestations into attestation_data_by_root_provider, but it doesn't collect from BlockWithAttestation::proposer_attestation so in test vectors where it's quite normal to have a block with only the proposer's attestation and no non-proposer attestation, the fork choice store won't have the attestation data, and so won't aggregate the proposer's attestation and so the weight is never updated. --> Fixed by adding proposer's attestation data to attestation_data_by_root_provider
  3. The test vector setup uses Signature::blank() while the test vectors are using real signatures now. -> Fixed by downloading the keys from https://github.com/leanEthereum/leansig-test-keys and use them in lean-spec-tests

Note that there are 2 changes to the fork choice store logic, but I think at most it would make the weights stronger so hopefully this doesn't break our devnet participation.

To-Do

@unnawut unnawut self-assigned this Mar 18, 2026
Comment on lines 126 to 159
Copy link
Contributor Author

Choose a reason for hiding this comment

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

triggers an actual tick so attestations get aggregated

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