Skip to content

fix: add proposer index bounds check before signature verification - #9194

Merged
nflaig merged 2 commits into
ChainSafe:nflaig/bellatrix-capella-gossip-tests-2from
lodekeeper:fix/gossip-block-proposer-index-bounds
Apr 5, 2026
Merged

fix: add proposer index bounds check before signature verification#9194
nflaig merged 2 commits into
ChainSafe:nflaig/bellatrix-capella-gossip-tests-2from
lodekeeper:fix/gossip-block-proposer-index-bounds

Conversation

@lodekeeper

Copy link
Copy Markdown
Contributor

Summary

  • Add explicit proposer_index < len(state.validators) bounds check before signature verification in gossip block validation
  • Without this, an out-of-range proposer index causes Missing pubkey for validator index crash in the pubkey cache instead of a clean REJECT
  • Uses existing BlockErrorCode.UNKNOWN_PROPOSER error code

Spec reference: [REJECT] The proposer_index is valid -- i.e. block.proposer_index < len(state.validators)

Test plan

  • gossip_beacon_block__reject_invalid_proposer_index now passes for both phase0 and bellatrix (was crashing before)
  • All 306 networking spec tests pass (minimal preset)
  • Build passes with no type errors

🤖 Generated with AI assistance

Without this check, a block with proposer_index >= len(state.validators)
causes a raw Error ("Missing pubkey for validator index N") in the pubkey
cache during signature verification, instead of a clean REJECT with
UNKNOWN_PROPOSER error code.

Spec: [REJECT] The proposer_index is valid -- i.e.
block.proposer_index < len(state.validators)

🤖 Generated with AI assistance

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@lodekeeper
lodekeeper requested a review from a team as a code owner April 5, 2026 23:17
}
}

// [REJECT] The proposer_index is valid -- i.e. block.proposer_index < len(state.validators)

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.

that condition doesn't exist in the spec??

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.

It's in the executable spec from PR #5047 (bellatrix gossip validation):

# [REJECT] The proposer index is a valid validator index
if block.proposer_index >= len(state.validators):
    raise GossipReject("proposer index out of range")

The prose spec doesn't have it explicitly — it's implied by the signature check (no valid pubkey for an out-of-range index). But without this guard, the pubkey cache throws a raw Error before the signature verification runs, which bypasses the GossipActionError path entirely.

The test vectors also expect REJECT for reject_invalid_proposer_index. If you'd prefer a different approach (e.g., wrapping the signature path to catch the pubkey lookup error), happy to change it.

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.

ok please align the comment with whats' in 5047

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.

Updated the comment to match the spec wording from #5047.

@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 introduces a validation check in validateGossipBlock to ensure the proposer_index is within the valid range of the state's validator count. If the index is invalid, the block is rejected with an UNKNOWN_PROPOSER error. I have no feedback to provide.

🤖 Generated with AI assistance

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@nflaig
nflaig merged commit 06a0c33 into ChainSafe:nflaig/bellatrix-capella-gossip-tests-2 Apr 5, 2026
1 check 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