eth/sequencer: judge the served preconf commitment on a matching store seal - #2433
kamuikatsurgi wants to merge 4 commits into
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
An unjudgeable canonical body can leave a commitment behind while the audit watermark advances past it, preventing later reconciliation.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Updates sequencer auditing so matching store seals also reconcile durable served preconfirmation commitments.
Changes:
- Judges commitments for matching seals.
- Adds regression coverage for served-content divergence.
| File | Description |
|---|---|
eth/sequencer/audit.go |
Extends commitment reconciliation to matching seals. |
eth/sequencer/audit_served_test.go |
Tests divergence despite a matching store seal. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v2.10.2-candidate #2433 +/- ##
=====================================================
- Coverage 57.93% 57.57% -0.36%
=====================================================
Files 957 959 +2
Lines 177763 175928 -1835
=====================================================
- Hits 102984 101298 -1686
+ Misses 69118 68967 -151
- Partials 5661 5663 +2
... and 55 files with indirect coverage changes
🚀 New features to boost your workflow:
|
8fead4b to
61fd0b2
Compare
The audit only reconciled the served preconf commitment when the store could not confirm a height (NOT_FOUND, unsealed, unusable seal); on a plain match it cleared the commitment unjudged. A matching seal only says what the store holds now. If the generation this node followed was displaced — a backup producer republished the height and its block became canonical — and the node crashed before the live path judged it, the store's current seal matches canonical while what this node served does not. That broken promise was dropped, unrecorded. Judge the commitment on every verdict except a mismatch, which already recorded the height. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
61fd0b2 to
d0cc446
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The new test fixture does not keep the canonical block, seal, and canonical hash consistent, so it does not accurately exercise the intended scenario.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
The fixture sealed one header at 7 and made canonical another, so the seal did not actually match. Derive the seal and canonical hash from the reordered block. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unreadable durable commitments can still be cleared without judgment, losing evidence.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
Resolved since last review (1)
Problem
The audit only reconciled the served preconf commitment (#2427) when the store could not confirm a height — NOT_FOUND, unsealed, or an unusable seal. On a plain matching seal it cleared the commitment without judging it.
A matching seal only says what the store holds now. If the generation this node followed was displaced — a backup producer republished the height and its block became canonical — and the node crashed before the live path judged it (the pending entry is memory-only; the commitment is the only durable evidence), the store's current seal matches canonical while what this node served does not. That contradicted preconf was dropped, unrecorded.
Fix
Judge the served commitment on every verdict except a mismatch, which already recorded the height. Same code path the
auditUnknownbranch already used; no new inputs.Evidence
TestAuditJudgesServedCommitmentDespiteMatchingSealreproduces the mechanism (store seal == canonical, served digest ≠ canonical): before the fixmismatch = 0, no ledger record, commitment cleared; after it, oneserved_mismatchat the height and the commitment cleared. Fulleth/sequencersuite green under-race.Cost: one
ReadPreconfServedper audited height and, when a commitment exists, a body read + hash fold — per-block cadence, bounded.Not included: a live devnet reproduction of the compound scenario (primary stall → backup wins → consumer crash before import); the unit test is the precise mechanism.
🤖 Generated with Claude Code