eth/sequencer: judge served preconf commitments the audit watermark already passed - #2434
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
The sweep is skipped when no audit range exists, and retention can trigger redundant scans.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
What changed in this PR
Adds reconciliation for served preconfirmation commitments below the audit watermark.
Changes:
- Sweeps stale commitments during audit passes.
- Adds regression tests for delayed and below-watermark reconciliation.
| File | Summary |
|---|---|
eth/sequencer/audit.go |
Adds the served-commitment sweep. |
eth/sequencer/audit_sweep_test.go |
Tests sweep and retry behavior. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v2.10.2-candidate #2434 +/- ##
=====================================================
- Coverage 57.93% 57.59% -0.34%
=====================================================
Files 957 959 +2
Lines 177763 175949 -1814
=====================================================
- Hits 102984 101340 -1644
+ Misses 69118 68948 -170
Partials 5661 5661
... and 56 files with indirect coverage changes
🚀 New features to boost your workflow:
|
25dc9e0 to
d4f6fab
Compare
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The audit-range decision can persist the watermark before the first pass, changing first-run and finality semantics.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Resolved since last review (1)
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>
…assed
The audit never revisits a height below its watermark, so a served preconf
commitment still sitting there is never judged again. Two ways one gets there:
judgeServed leaves a commitment in place when the canonical body is not local
yet ("so a later pass can judge it"), but the pass then persists the mark past
it and no later pass ever looks; and after a rewind moves the head below the
mark, the node re-serves those heights and persists fresh commitments that
the live path never clears (markCanonicalHeadAudited returns early at or
below the mark). A contradicted promise in either case went unrecorded.
Sweep the served commitments below the range at the start of every pass and
judge them with the existing reconcileSkippedServed. One iterator seek per
pass; only the entries present are visited, and there are normally none.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
d4f6fab to
a9003e1
Compare
The sweep below the watermark judged [0, watermark] on every pass. The watermark is seeded at the head on a first run, so that included heights above finality, which the walk deliberately never judges. Bound the sweep by the same ceiling. Check the context before and during the sweep so a cancelled pass returns instead of judging every commitment first, and decide the store dial from a side-effect-free read so runAuditPass no longer seeds the watermark on the way to that decision. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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>
Fold rangeToAudit into run: read the head, finality and watermark once, seed on a first run, sweep the served commitments up to min(watermark, finality), then walk. The consumer always builds the lazy store client instead of predicting whether the walk will need it, which removes the read-only twin of rangeToAudit and the nil-fetch special case. Log when the audit writes an invalidation record; a sweep-only pass has no walk report, so this was otherwise silent. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The sweep has a critical commitment-race issue, with additional audit-path gaps requiring fixes.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved audit synchronization, cancellation/memory, and evidence-retention issues remain.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Resolved since last review (2)
Summary
Judge outstanding served preconfirmation commitments even after the audit watermark has passed them. A missing canonical body, or serving a height again after a rewind, can leave durable evidence below the watermark that the forward walk will never revisit. The audit now sweeps those entries against the canonical chain, bounded by finality; canonical-head handling at or below the watermark queues the sweep without requiring a session restart.
This branch now includes #2433, so a matching store seal also checks the commitment this node actually served. A current store generation matching canonical does not prove that an earlier served generation matched. Commitment reads, comparisons, deletion, and live writes share a mutex; network reads remain outside it. Unreadable commitments and commitments whose mismatch record could not be written are retained. The local sweep runs even when gRPC client construction fails, observes cancellation between entries, and never extends above finality when store retention has moved further ahead.
Executed tests
go test -race ./eth/sequencer -count=1passed on rerun (58.680s). The first attempt failed in the unchanged publisher testTestBuildStartDiscardsSupersededBuffer; targeted audit regressions passed.No live compound producer-failure/consumer-crash or deep-rewind devnet reproduction was performed for this change. Fresh PR CI remains required.
Rollout notes
No consensus, public API, configuration, or database-format change. This changes local preconfirmation audit behavior. #2433 can merge first; its commits are included here to make the integration explicit.
This does not recover commitments already deleted before a deeper rewind. Retention beyond the currently trusted finality boundary remains a separate design decision.