Skip to content

test: data column sidecar gossip validation spec tests - #9430

Open
lodekeeper wants to merge 3 commits into
ChainSafe:nflaig/deneb-gossip-spec-testsfrom
lodekeeper:feat/data-column-sidecar-gossip-spec-harness
Open

test: data column sidecar gossip validation spec tests#9430
lodekeeper wants to merge 3 commits into
ChainSafe:nflaig/deneb-gossip-spec-testsfrom
lodekeeper:feat/data-column-sidecar-gossip-spec-harness

Conversation

@lodekeeper

Copy link
Copy Markdown
Contributor

Motivation

Stacks on top of #9372 to extend Lodestar's gossip-validation spec-test coverage from data_column_sidecar (PeerDAS / Fulu+).

Description

Two follow-up commits on nflaig/deneb-gossip-spec-tests:

  1. feat(test): wire gossip_data_column_sidecar into spec test harness — adds harness support for the data_column_sidecar topic in the gossip validation spec tests (consensus-specs PR Add executable gossip validation functions for deneb ethereum/consensus-specs#5146 / Add LC header validations #5246).
  2. feat(beacon-node): validate finalized-ancestor and seen-tuple for data column sidecars — fills in the source-side gaps the harness surfaced: finalized-ancestor check on the sidecar's block root and the (slot, proposer_index, column_index) seen-tuple check, matching the spec's validation conditions.

Spec test results

Full networking suite passes on both presets against nflaig/deneb-gossip-spec-tests base:

  • spec-minimal: 920 passed in 20.17s (logs/gossip-spec-rerun-minimal-2026-05-30-v2.log)
  • spec-mainnet: 932 passed in 300.06s (logs/gossip-spec-rerun-mainnet-2026-05-30.log)

Notes


🤖 Generated with AI assistance

Adds runner support for the fulu `gossip_data_column_sidecar` networking
spec tests from consensus-specs PR ChainSafe#5246. Mirrors the existing
`gossip_blob_sidecar` arm: SSZ-decodes `fulu.DataColumnSidecar` and
invokes `validateGossipFuluDataColumnSidecar` with the fixture's
`subnet_id`.

Builds on top of ChainSafe#9372. Does not address `gossip_partial_data_column_sidecar`
(optional feature) nor the open `gossip_beacon_block__valid_at_blob_parameters_limit`
zero-parent harness gap.

🤖 Generated with AI assistance
…a column sidecars

Implements spec checks 9 and 12 from `validate_data_column_sidecar_gossip`
that the harness previously hit as gaps:

- 9) REJECT a sidecar whose parent block does not descend from the current
  finalized checkpoint. Mirrors the blob sidecar pattern using
  `forkChoice.getAncestor(parentRoot, finalizedSlot)`.
- 12) IGNORE a subsequent sidecar matching an already-seen
  `(slot, proposer_index, column_index)` tuple, with `mark` after all
  signature, inclusion-proof, and KZG checks pass. Tracking lives on
  `SeenBlockInput` alongside the existing blob-tuple cache so the dedup
  applies at the validator entry point (not just in `gossipHandlers`).

Adds `FINALIZED_NOT_ANCESTOR` and `ALREADY_SEEN_TUPLE` error codes.
Removes a stale "Handled by 7)" placeholder comment.

spec-minimal `gossip_data_column_sidecar`: 17/17 pass.
spec-mainnet `gossip_data_column_sidecar`: 17/17 pass.
spec-minimal `gossip_blob_sidecar`: 32/32 still pass (no regression).
beacon-node unit `dataColumnSidecar.test.ts`: 19/19 pass.

🤖 Generated with AI assistance
@lodekeeper
lodekeeper requested a review from a team as a code owner May 30, 2026 20:27

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request implements gossip validation rules 9 and 12 for Fulu data column sidecars, including tracking seen sidecar tuples and verifying that the finalized checkpoint is an ancestor of the sidecar's block. It also integrates these validations into the spec tests. Feedback points out a critical issue in the ancestor check where calling chain.forkChoice.getAncestor with a slot higher than the parent block's slot throws an error, causing valid sidecars to be incorrectly rejected. A fix is suggested to handle cases where the parent block's slot is less than or equal to the finalized ancestor slot.

Comment on lines +111 to +116
let ancestorRoot: RootHex | undefined;
try {
ancestorRoot = chain.forkChoice.getAncestor(parentRoot, finalizedAncestorSlot).blockRoot;
} catch {
// parent not in fork-choice or finalized slot before parent's chain start
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

If parentBlock.slot is less than or equal to finalizedAncestorSlot (which can happen when there are skipped slots or when the parent block is before the finalized slot), calling chain.forkChoice.getAncestor with a slot higher than the block's slot will throw an error. This causes ancestorRoot to remain undefined, leading to an incorrect FINALIZED_NOT_ANCESTOR rejection for a valid sidecar.

According to the consensus spec's get_ancestor logic, if the block's slot is less than or equal to the requested ancestor slot, the ancestor is the block itself (parentRoot). We should handle this case explicitly to avoid throwing an error and incorrectly rejecting valid gossip messages.

  let ancestorRoot: RootHex | undefined;
  if (parentBlock.slot <= finalizedAncestorSlot) {
    ancestorRoot = parentRoot;
  } else {
    try {
      ancestorRoot = chain.forkChoice.getAncestor(parentRoot, finalizedAncestorSlot).blockRoot;
    } catch {
      // parent not in fork-choice or finalized slot before parent's chain start
    }
  }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the careful read, but I think the premise here is incorrect.

getAncestor in packages/fork-choice/src/protoArray/protoArray.ts:1523-1541 follows the consensus-specs get_ancestor algorithm:

if (block.slot <= ancestorSlot) {
  return block;
}

It only throws in two cases:

  1. MISSING_PROTO_ARRAY_BLOCK if blockRoot isn't in the proto-array. We already confirmed parentBlock exists via chain.forkChoice.getBlockHexDefaultStatus(parentRoot) just above (line 79-96), so this can't fire.
  2. UNKNOWN_ANCESTOR if walking up hits a missing parent — but that requires block.slot > ancestorSlot, i.e. it's not the boundary case being described.

For parentBlock.slot <= finalizedAncestorSlot, getAncestor returns parentBlock directly, so ancestorRoot = parentRoot. The subsequent ancestorRoot !== finalizedCheckpoint.rootHex check then resolves correctly:

  • Skipped slots at the checkpoint boundary (finalized root anchored at slot < finalizedAncestorSlot, parentRoot equals that root): ancestorRoot = parentRoot = finalizedRoot → PASS ✅
  • Parent on a different fork at/before the finalized slot: ancestorRoot = parentRoot ≠ finalizedRoot → REJECT ✅ (correct rejection — that parent isn't on the finalized fork)

This pattern directly mirrors blobSidecar.ts:144-159, which has been live for some time without producing false rejections. The proposed fix is functionally equivalent (it just inlines the early-return that getAncestor already does internally).

Spec reference: https://github.com/ethereum/consensus-specs/blob/dev/specs/phase0/fork-choice.md#get_ancestor

No change needed here.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3435d9b7a6

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

}

case GossipType.data_column_sidecar: {
const dataColumnSidecar = rejectOnInvalidSerializedBytes(() => ssz.fulu.DataColumnSidecar.deserialize(bytes));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Use the fork SSZ type for data-column gossip tests

When these networking tests run for gloas/gossip_data_column_sidecar, this always deserializes the message as the Fulu container, which still expects signedBlockHeader, kzgCommitments, and an inclusion proof. Gloas data-column sidecars instead contain slot and beaconBlockRoot, so valid Gloas fixtures will be mapped to SPEC_INVALID_SERIALIZED_BYTES and rejected before the Gloas validation path can be exercised; the harness should deserialize with sszTypesFor(fork).DataColumnSidecar and dispatch to the appropriate validator.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

You're right that the Gloas DataColumnSidecar differs from Fulu — packages/types/src/gloas/sszTypes.ts:301-313 drops signedBlockHeader / kzgCommitments / kzgCommitmentsInclusionProof and adds slot + beaconBlockRoot. Deserializing Gloas bytes through the Fulu container would indeed reject as SPEC_INVALID_SERIALIZED_BYTES.

Scope-deferring this for two reasons:

  1. No Gloas DCS gossip fixtures yet. Both consensus-specs PRs (Track sync committee metrics #5146, Add LC header validations #5246) and the current test corpus only include gossip_data_column_sidecar under Fulu:

    packages/beacon-node/spec-tests/tests/{minimal,mainnet}/fulu/networking/gossip_data_column_sidecar
    

    The Gloas networking/ folder only has slashings, BLS-to-execution, sync committee — no DCS fixtures. Adding the dispatch now would be untested code that the runner never reaches.

  2. Different validator signature. validateGossipGloasDataColumnSidecar(chain, payloadInput, sidecar, …) takes a PayloadEnvelopeInput, which isn't present in the standalone-DCS gossip fixture format. Once upstream defines the Gloas DCS fixture layout (likely coupled with the gossip_execution_payload_envelope flow), the harness can construct the right input shape and dispatch via sszTypesFor(fork).DataColumnSidecar.

Tracking as a follow-up — I'll wire the per-fork dispatch when Gloas DCS gossip fixtures land in consensus-specs.

// 12) [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
// -- Handled in seenGossipBlockInput
if (chain.seenBlockInputCache.isSeenDataColumnSidecar(blockHeader.slot, proposerIndex, dataColumnSidecar.index)) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a cheap check that should come first or at least before the expensive checks...

Specs show this as first https://github.com/ethereum/consensus-specs/blob/master/specs/fulu/p2p-interface.md#blob-subnets

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 589caee464: the ALREADY_SEEN_TUPLE lookup now runs right after extracting the sidecar header, before structural verification, parent-state regen, and KZG checks. The markSeenDataColumnSidecar call stays at the end after full validation passes.

// [IGNORE] The sidecar is the first sidecar for the tuple (block_header.slot, block_header.proposer_index,
// sidecar.index). Spec orders this check first, before any expensive verification.
if (
chain.seenBlockInputCache.isSeenDataColumnSidecar(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

why do we need this, doesn't the BlockInput handle seen columns already?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

These two checks have different keys and the spec mandates the tuple check.

  • BlockInput.hasColumn (gossipHandlers.ts:354-366) is keyed by (blockRoot, columnIndex) — it short-circuits re-gossip of the same column for the same block (or columns we already fetched via reqresp / getBlobsV2).
  • seenBlockInputCache.isSeenDataColumnSidecar is keyed by (slot, proposerIndex, columnIndex) and is the spec's first check. It catches equivocation: a proposer signing two different blocks at the same slot produces sidecars with the same (slot, proposer, index) but different blockRoot, so the BlockInput-keyed check would miss the second one and we'd re-run KZG + propagate. The class comment at seenGossipBlockInput.ts:440-442 calls this out.

Same shape as the blob path (isSeenBlobSidecar, blobSidecar.ts:182).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@lodekeeper are you sure about this, if that's the case this seems like a bug in our current implementation

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch — my "spec orders this first" framing was wrong. Re-reading the Deneb spec and Fulu spec, the IGNORE tuple check is listed after KZG and qualified by "with valid header signature, sidecar inclusion proof, and kzg proof" — i.e. "first validated tuple", not "first tuple seen".

So neither file has a literal spec-correctness bug:

  • Mark side (writes to cache) is what the "with valid…" qualifier constrains — and in both files markSeen* runs only after all checks pass (data column line 231, blob line 209). Good.
  • Read side placement is up to the client. Lighthouse uses a two-phase pattern: an early read-only check (verify_is_unknown_sidecar) for perf, then the late write under the spec-listed position. See consensus-specs#3261 for the related blob-spam discussion.

This PR places the data column read check early; blobSidecar.ts (on this branch) places it after KZG. Both are correctness-equivalent given the post-validation mark; the early position saves a BLS sig + inclusion proof + KZG verify on duplicate gossip.

The two-key argument still holds: BlockInput.hasColumn catches re-gossip of (blockRoot, columnIndex), seen-tuple catches equivocations on (slot, proposerIndex, columnIndex). So we still need the seen-tuple check somewhere. Happy to align both files to the early position in a follow-up if you prefer the perf path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

5 participants