Skip to content

Add state transition vectors for aggregation bits bounds handling#899

Merged
tcoratger merged 2 commits into
leanEthereum:mainfrom
lambdaclass:aggregation-bits-bounds-fixtures
Jun 10, 2026
Merged

Add state transition vectors for aggregation bits bounds handling#899
tcoratger merged 2 commits into
leanEthereum:mainfrom
lambdaclass:aggregation-bits-bounds-fixtures

Conversation

@MegaRedHand

@MegaRedHand MegaRedHand commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Motivation

An audit finding in ethlambda (fixed in lambdaclass/ethlambda#425) showed that clients can diverge on blocks carrying malformed aggregation_bits: ethlambda skipped an attestation whose bitfield was longer than the validator registry, while zeam and lantern reject such blocks. A skipped attestation also left a spurious all-false entry in the post-state — a cross-client state-divergence vector. The spec had no vectors pinning this behavior.

Spec change

process_attestations previously crashed with a bare IndexError when a set bit pointed past the validator registry. That exception is not an AssertionError, so rejection handlers cannot catch it, and it carries no language-neutral reason for fixtures. It now raises SpecRejectionError(VALIDATOR_INDEX_OUT_OF_RANGE), matching the equivalent check in verify_signatures. The check sits after the vote filters, so filtered attestations are still skipped without their bits being touched. The pre-existing EMPTY_AGGREGATION_BITS rejection is unchanged, now resolved once before the tally insert.

New vectors (state_transition format)

Vector Expectation
Set bit at index >= validator count Rejected: VALIDATOR_INDEX_OUT_OF_RANGE
All-false bits (registry-sized) Rejected: EMPTY_AGGREGATION_BITS
Zero-length bitfield Rejected: EMPTY_AGGREGATION_BITS
Oversized bitfield, all set bits in range Valid; pinned post-state root fails any client that skips the attestation

The last vector is the sharp one: validated against ethlambda, it passes on the fixed branch and fails on pre-fix code with a state root mismatch. The rejection vectors discriminate via the emitted rejectionReason, so client runners should assert it rather than only checking that the block was rejected.

Framework changes

  • AggregatedAttestationSpec gains a raw aggregation_bits override, honored only by the unsigned forced-attestation path of the state transition format. Deriving bits from validator indices can only produce tight bitfields, so empty or padded bitfields were previously inexpressible.
  • The placeholder zero state root of rejection tests now applies only to the final block, so earlier setup blocks process normally instead of failing with STATE_ROOT_MISMATCH. All existing rejection tests are single-block, where this changes nothing (verified: full fill passes unchanged).

Testing

  • uv run pytest: 2872 passed
  • uv run fill --fork=lstar -n auto: 530 fillers pass, determinism check passes
  • just check clean
  • New fixtures run against ethlambda: 4/4 pass on the fix branch; the oversized-bits vector fails on pre-fix code as intended

@tcoratger

Copy link
Copy Markdown
Collaborator

Thanks a lot, can you just rebase after #895 and follow the new doc writer conventions for the documentation of the test vectors just for uniformity? :)

Also I haven't checked in details but would be nice to ensure that this test doesn't already exist with the new coverage added in the PR. I imagine this test coverage was not present before since it helped you guys to spot the bug.

…itly

A set aggregation bit at an index past the validator registry previously
crashed the vote tally write with a bare IndexError. That exception is not
an AssertionError, so rejection handlers cannot catch it, and it carries no
language-neutral reason for cross-client fixtures to assert against.

Raise SpecRejectionError(VALIDATOR_INDEX_OUT_OF_RANGE) instead, matching
the equivalent check in signature verification. The check sits after the
vote filters, so filtered attestations are still skipped without touching
their bits.
…unds

Cover the malformed-bits block rejection paths and the benign padding case
(prompted by an audit finding in ethlambda, where a skipped attestation
left a divergent post-state):

- set bit past the validator registry -> VALIDATOR_INDEX_OUT_OF_RANGE
- all-false bits and zero-length bits -> EMPTY_AGGREGATION_BITS
- trailing unset padding past the registry -> processed normally; the
  pinned post-state root fails any client that skips the attestation

Two framework changes make these expressible:

- AggregatedAttestationSpec gains a raw aggregation bits override for the
  unsigned forced-attestation path, since deriving bits from validator
  indices can only produce tight bitfields.
- The placeholder zero state root of rejection tests now applies only to
  the final block, so earlier setup blocks process normally instead of
  failing with STATE_ROOT_MISMATCH. All existing rejection tests are
  single-block, where this changes nothing.
@MegaRedHand MegaRedHand force-pushed the aggregation-bits-bounds-fixtures branch from 88d9731 to 3cd715e Compare June 10, 2026 15:43

@tcoratger tcoratger left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for flagging this!

@tcoratger tcoratger marked this pull request as ready for review June 10, 2026 16:27
@tcoratger tcoratger merged commit 3e90d00 into leanEthereum:main Jun 10, 2026
13 checks passed
@MegaRedHand MegaRedHand deleted the aggregation-bits-bounds-fixtures branch June 10, 2026 16:28
tcoratger added a commit that referenced this pull request Jun 10, 2026
…ions (#919)

* docs(testing): align aggregation-bits vectors with doc-writer conventions

Follow-up to #899. Brings the aggregation-bits bounds vectors in line
with the consensus test-vector documentation standard:

- Drop all inline body comments from the test vectors; the
  Given/When/Then docstring is the single source of truth.
- Strip the "a client that..." narration from every Then section so it
  states only the asserted outcome (rejection reason or post-state).
- Move the bitfield layout and SSZ-encoding rationale into Given as
  atomic facts, and note the bitfield-limit vs registry distinction.
- Tighten the aggregation_bits field and resolver docstrings to terse
  one-sentence-per-line phrasing.
- Note in process_attestations that signature verification normally
  rejects an out-of-range bit first, so this guards the unsigned path.
- Make is_final_block keyword-only on the block builder.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* docs(testing): use bullets for the aggregation_bits field docstring

Per the doc-writer rules, render the field's three behaviors as bullet
points instead of prose, and fix the summary line so it describes the
default (derived) case rather than only the verbatim override.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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