Skip to content

feat(validator_store): record block-QBFT-decided beacon block roots - #1224

Merged
mergify[bot] merged 6 commits into
sigp:epbsfrom
jnhsigmap:epbs
Aug 11, 2026
Merged

feat(validator_store): record block-QBFT-decided beacon block roots#1224
mergify[bot] merged 6 commits into
sigp:epbsfrom
jnhsigmap:epbs

Conversation

@jnhsigmap

@jnhsigmap jnhsigmap commented Aug 7, 2026

Copy link
Copy Markdown
Contributor

Problem, Evidence, and Context (Required)

Change Overview (Required)

  • AnchorValidatorStore gains a small handoff store: a mutex-guarded map from (validator, slot) to the decided block root, written at the success point of block consensus for Gloas-and-later duties.
  • Writes are first-write-wins. A repeat write of the same root is idempotent. A different root for the same duty is a hard error that aborts the duty before threshold signing, because two decided roots for one duty means consensus safety failed.
  • Reads reject slots older than 4 slots against the store's own clock. Eviction on insert bounds the map. A miss, a stale read, and a clock failure each return a distinct error. No path returns a default root.
  • Read the production change in validator_store first (one store field, two private helpers, one hook in the block-decide path). The two new test modules are map-level contract tests and end-to-end sign_block tests, and they share a small fixture struct.
  • Intentionally unchanged: the envelope sign stub, pre-Gloas proposer flow (creates no entries), decode_decided_block, and the store's public constructor signature.

Risks, Trade-offs, and Mitigations (Required)

  • Blast radius is one crate. The field and helpers are private, so no consumer outside validator_store changes.
  • Trade-off: a conflicting root fails the whole block duty instead of overwriting. Signing a second block for one slot is the dangerous outcome, so the duty aborts and an end-to-end test pins that no signature is attempted after a conflict.
  • Trade-off: the conflict error payload is boxed to keep the error enum under clippy's size limit while it keeps both roots for diagnosis.
  • The write path takes no clock check by design. The decided value passed slot validation upstream, and a late write for an old slot must still succeed. A test pins that such a write cannot evict a newer live root.

Validation (Required)

  • 11 map-level tests pin the store contract: exact round-trip, error on a miss (never a default), key isolation across validators and slots, idempotent rewrite, conflict keeps the first root, stale rejection while the entry is still present, the inclusive age boundary, and both eviction directions.
  • 3 end-to-end tests drive the real sign_block path: the stored root is the decoded block's canonical_root() and not the QBFT wrapper hash, a conflict aborts before any threshold signature, and an Electra duty leaves the store empty.
  • cargo test -p anchor_validator_store (79 passed), full workspace make test in release mode, make cargo-fmt-check, and make lint all pass.

Rollback (Required for behavior or runtime changes; optional otherwise)

  • Revert the two commits. The store is in-memory only, private, and has no consumer yet, so a revert has no config, data, or operational impact.

Blockers / Dependencies (Optional)

Additional Info / Next Steps (Optional)

  • Known coverage limits: the clock-failure read arm is untestable with the current harness (genesis at time zero), and the pre-Gloas test exercises the decoded-shape guard rather than the version gate, because Electra decodes to a blinded block. Both limits are recorded as comments in the test files.

@codecov-commenter

codecov-commenter commented Aug 7, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.46524% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.03%. Comparing base (09b302d) to head (f39e9a3).

Files with missing lines Patch % Lines
...idator_store/src/testing/decided_block_root_e2e.rs 98.49% 2 Missing ⚠️
.../validator_store/src/testing/decided_block_root.rs 99.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             epbs    #1224      +/-   ##
==========================================
+ Coverage   71.95%   73.03%   +1.08%     
==========================================
  Files         168      170       +2     
  Lines       33797    34356     +559     
==========================================
+ Hits        24317    25091     +774     
+ Misses       9480     9265     -215     
Flag Coverage Δ
rust 73.03% <99.46%> (+1.08%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jnhsigmap

Copy link
Copy Markdown
Contributor Author

@claude review this PR. Prioritise finding potential edge cases not covered and quality of tests. Ensure solution is correctly implemented against issue spec.

@claude-code-actions-sigp

claude-code-actions-sigp Bot commented Aug 7, 2026

Copy link
Copy Markdown

Claude finished @jnhsigmap's task in 7m 51s —— View job


PR Review: record block-QBFT-decided beacon block roots

Verdict

The implementation is correct against the stated intent. The store is written at the single post-consensus point (decide_abstract_block, lib.rs:636-644), which is the only Gloas proposer path — envelope signing is stubbed and decide_abstract_block has one caller, so the hook is not bypassable. Every operator decodes the same QBFT-decided completed_data and stores the same canonical_root(), satisfying the "all operators hold the same value" requirement. First-write-wins, idempotent rewrite, hard-error-on-conflict, read-side staleness, and slot-referenced eviction all behave as documented. Test suite is well-structured (AAA, descriptive names, contract- and e2e-level split, and the negative assertions — canonical vs wrapper hash, captured-calls-empty on conflict — are genuinely load-bearing).

Edge case not covered (1 inline)

Write-side inclusive eviction boundary is unpinned. The retain keeps an entry at exactly MAX_DECIDED_ROOT_AGE_SLOTS behind the inserted slot (>=), but no test asserts this. Read-side inclusive edge and write-side MAX+1 eviction are both covered; the write-side inclusive edge is the missing corner. A >=> regression would pass silently. See inline comment on lib.rs:672.

Test-quality notes (no change required)

  • The ForkName >= Gloas version gate is untested — but it is also redundant. In the real sign_block flow the block is always projected to blinded before consensus (lib.rs:2994-3012), so a pre-Gloas duty always decodes to UnsignedBlock::Blinded. That means the let UnsignedBlock::Full(FullBlockContents::Block(_)) shape guard alone already excludes every pre-Gloas real-flow case, and pre_gloas_duty_creates_no_entry (Electra→Blinded) exercises only that shape guard. The version gate is defense-in-depth against a hypothetical future full-block fork; keep it, but be aware no test distinguishes it from the shape guard, and none can via the real flow. The author's inline comment already acknowledges this honestly.

  • Conflict-window is bounded by the eviction window (by design, worth a one-line note). After an entry is evicted (a later insert > MAX slots ahead), a subsequent conflicting re-decide at that old slot would insert fresh rather than raise DecidedRootConflict. This is not reachable in practice — a duty runs once per slot and such an old slot is already read-side stale — so it is an acceptable trade-off, not a defect. Flagging only so the "conflict = consensus-safety-failure abort" guarantee is understood to hold within the retention window.

  • conflicting_root_aborts_before_threshold_signing proves no signing was attempted (captured_calls empty) but does not assert consensus actually ran first. Minor; the hook placement makes this safe.

  • Clock-failure read arm untested — correctly acknowledged as untestable with the genesis-at-zero harness.

No blocking issues. The one inline is a small, worthwhile test addition.

Comment thread anchor/validator_store/src/lib.rs
@jnhsigmap
jnhsigmap marked this pull request as ready for review August 7, 2026 07:00
@jnhsigmap

Copy link
Copy Markdown
Contributor Author

addressed claude review in 852aedd. also my bad with the source branch name 🤦. I will fix that up on my local after this merges.

@shane-moore

Copy link
Copy Markdown
Member

lgtm breh! will approve after you knock out the small merge conflicts

@mergify

mergify Bot commented Aug 10, 2026

Copy link
Copy Markdown

Queued — the merge queue status continues in this comment ↓.

@mergify

mergify Bot commented Aug 11, 2026

Copy link
Copy Markdown

Merge Queue Status

  • Entered queue2026-08-11 02:51 UTC · Rule: default · triggered by rule Add outdated, ready-to-merge PRs to merge queue
  • Checks passed · on draft merge queue: checking epbs (09b302d) and #1224 together #1237
  • Merged2026-08-11 03:20 UTC · at f39e9a3c99b5721d66467deb7a0ce7e988fb056e · squash

This pull request spent 28 minutes 55 seconds in the queue, including 27 minutes 4 seconds running CI.

Required conditions to merge
  • check-success=cli-reference-check
  • check-success=run-local-testnet
  • check-success=test-suite-success

@mergify
mergify Bot merged commit 7ec3a06 into sigp:epbs Aug 11, 2026
23 checks passed
@jnhsigmap
jnhsigmap deleted the epbs branch August 11, 2026 03:20
@mergify mergify Bot removed the queued label Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants