Verify data column sidecars before the duplicate check - #5560
Open
pucedoteth wants to merge 4 commits into
Open
Conversation
`validate_blob_sidecar_gossip` verifies the KZG proof before the duplicate check, but `validate_data_column_sidecar_gossip` does the opposite in both Fulu and Gloas. For a sidecar whose tuple has already been seen and whose KZG proof is invalid, the blob path returns REJECT while the data column path returns IGNORE. REJECT descores the peer and IGNORE does not, so the same malformed message is scored differently depending on which sidecar type carries it. Fulu originally matched the blob path. The prose it was converted from in ethereum#5246 read: [REJECT] The sidecar's column data is valid as verified by verify_data_column_sidecar_kzg_proofs(sidecar). [IGNORE] The sidecar is the first sidecar for the tuple (block_header.slot, block_header.proposer_index, sidecar.index) with valid header signature, sidecar inclusion proof, and kzg proof. The duplicate check is defined over sidecars that already carry a valid signature, inclusion proof and KZG proof, so it cannot run first without changing its meaning. Moving it ahead of those checks was incidental to making the validations executable, not an intended change. Gloas's gossip functions were added later in ethereum#5294 and inherited the order. Move the duplicate check after KZG verification in both, restoring the prose order and matching `validate_blob_sidecar_gossip`. Fixes ethereum#5451 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #5451.
Problem
validate_blob_sidecar_gossipverifies the KZG proof before the duplicate check, butvalidate_data_column_sidecar_gossipdoes the opposite — in both Fulu and Gloas. For a sidecar whose tuple has already been seen and whose KZG proof is invalid:validate_blob_sidecar_gossipREJECT—invalid blob kzg proofvalidate_data_column_sidecar_gossipIGNORE—already seen sidecar ...REJECTdescores the peer,IGNOREdoes not, so the same malformed message is scored differently depending on which sidecar type carries it.Why verify-then-dedup is the correct order
Fulu originally matched the blob path. The prose that #5246 converted into executable form read:
The duplicate check is defined over sidecars that already carry a valid signature, inclusion proof and KZG proof — so it cannot run first without changing its meaning. #5246 was a bulk prose-to-Python conversion ("Add executable gossip validation functions for fulu"); moving the check to the top looks incidental rather than intended.
Gloas's gossip functions were added later, in #5294, and inherited the reordered structure — so Gloas is not independent confirmation of a design choice. That is the one point where I differ from the issue, which states Gloas is already correct; it currently dedups first, like Fulu, so this PR changes both.
Change
Move the duplicate check after KZG verification in
specs/fulu/p2p-interface.mdandspecs/gloas/p2p-interface.md. Fulu now matchesvalidate_blob_sidecar_gossipstep for step: inclusion proof → KZG → dedup → proposer → mark seen.The diff is a pure move;
seen.add(...)already happened at the end of the function in both, so the contents of theseenset are unchanged. Only the verdict for an invalid duplicate changes, fromIGNOREtoREJECT.Alternative
If maintainers prefer dedup-first for data columns (cheap check first, skipping KZG work on duplicates), then the consistent fix is the opposite one — move dedup earlier in
validate_blob_sidecar_gossip. I went with this direction because it restores the documented prose order and because rejecting invalid sidecars is itself the peer-scoring defense. Happy to flip it.Test plan
Added
test_gossip_data_column_sidecar__reject_already_seen_with_invalid_kzg_proofs, which delivers a valid sidecar and then the same tuple with corrupted KZG proofs.@with_fulu_and_latermeans the one test covers both forks.AssertionError: assert 'ignore' == 'reject'— verified by reverting the two spec files and regenerating.specs/gloas/p2p-interface.mdalso fails it, confirming the test really exercises the Gloas path.__ignore_already_seentest is unaffected: a valid duplicate still returnsIGNORE.245 passedacross the fulu, gloas, deneb and electra networking suites;make lintclean.🤖 Written with Claude Code. All results above come from a local
make lintand pytest run against the generated pyspec.