eth/sequencer, rawdb: record served preconfirmations to close the open-block loss window

[kamuikatsurgi]

What

bor_getInvalidPreconfBlocks must never omit a block whose preconfirmation this node served and then broke. The store audit (#2388) closes the gap for a node's downtime by comparing the store's sealed generation at each height against canonical — so it can only judge heights the store still holds a seal for.

One window escapes it: the producer serves preconfirmations for an open block, then dies before sealing it, and its successor rebuilds the height without the store. The store holds no seal (or only an unsealed open), so the audit has nothing to compare, advances past the height, and the broken preconfirmation is never recorded — the severe case, an invalid preconf absent from the ledger.

Fix

Record the promise durably, at serve time. When receipts are about to reach callers (indexExecutedTransactions), the node persists a per-height commitment to its own chaindb: the count of transactions preconfirmed there and a keccak fold over their hashes in served order (WritePreconfServed). It lives in a keyspace off the block path, so it survives the crash that loses the store generation.

Reconcile it when the store cannot confirm a height — it holds nothing (NOT_FOUND), holds a generation it never sealed, returns a seal that is unusable (undecodable or wrong-height), or the height falls in a retention-skipped range. The audit folds the canonical block's leading transactions the same way and compares; a divergence is recorded as served_mismatch, a distinct reason from unobserved_mismatch because the node did serve this one. Commitments are cleared once judged.

Notes / boundaries

• The fallback digest is node-local, compared only against itself (serve vs audit), so it folds the already-cached tx.Hash() rather than the on-wire commitment scheme — cheaper on the serve path, no tag or chain seed.
• Only the latest generation per height is held; a superseded generation's promise is left to the live path, as before.
• Read-only monitor: it records a marker and does not touch fork choice or block acceptance.

Testing

• Unit/rawdb: round-trip and range reads for the commitment (incl. malformed value); audit records served_mismatch for NOT_FOUND, unsealed, retention-skip, and unusable-seal cases; kept promises and store-confirmed heights record nothing; commitments cleared once judged. go test -race clean on core/rawdb and eth/sequencer.

• Durability (deterministic, 5/5): TestServedCommitmentSurvivesCrashAndIsAudited — real on-disk pebble crash (close/reopen); no commitment → unrecorded (the bug), durable commitment → recorded.

• Live devnet A/B (custom bor image, deterministic fault injection): serve a divergent preconf at height H, fsync, crash (SIGKILL-equivalent), restart, store returns NOT_FOUND for H.

  height  reconcile   bor_getInvalidPreconfBlocks   servedmismatch
  171     off (pre)   []  UNRECORDED (the bug)       0
  245     on  (fix)   0xf5  RECORDED                 1
  319     on  (fix)   0x13f RECORDED                 1
  393     on  (fix)   0x189 RECORDED                 1
  

  Trace of one fixed run (RPC bor logs):

  TESTHOOK: persisted served commitment height=736 count=3   (write, pre-crash)
  Rewound to block with state number=535                     (crash + restart)
  TESTHOOK: audit read served commitment height=736 ok=true  (survived crash)
  servedmismatch=1                                           (reconciled -> recorded)
  

🤖 Generated with Claude Code

[claude]

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.}

[pratikspatil024]

Good find, and the diagnosis is right — the gap is real and it's in my code. auditHeight returns auditNoSeal when a generation has no seal, and recordVerdict groups that with auditMatch, so the height is walked, nothing is recorded, no counter moves, and the watermark advances over it. Of the three ways a height can get past that pass, retentionskipped and unheld at least increment something; auditNoSeal was the one that was completely silent, and it's the one you hit.

I'd also had the gap scoped too narrowly in my own notes — I'd written it down as "the producer died before publishing at all", where the real condition is "no seal in the store", which includes published-but-unsealed. That's the far more likely crash shape, so thanks for pinning it properly.

The approach looks right to me: it extends the pass rather than replacing it, which keeps the store-side comparison covering preconfs served by other nodes while the local commitment covers what the store never sealed. Nice handling of the cases beyond the headline one — reconciling on auditUnknown instead of clearing, so an undecodable or wrong-height seal can't bury a broken promise before the watermark passes it, and having reconcileSkippedServed scan the served prefix rather than every skipped height.

Two inline comments, neither blocking: the block == nil path reading as a mismatch when it's really "can't judge", and the serve-path write being unmeasured. Prefix-fold semantics look correct to me — folding canonical's leading count transactions means an insertion or reorder anywhere in the served prefix trips it, which is exactly the transactionIndex 2 → 3 from run 04.

[codecov]

Codecov Report

❌ Patch coverage is 92.15686% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.46%. Comparing base (68a4609) to head (79d16fa).
⚠️ Report is 22 commits behind head on v2.10.2-candidate.

Files with missing lines	Patch %	Lines
eth/sequencer/audit.go	93.12%	6 Missing and 3 partials ⚠️
eth/sequencer/consumer.go	68.75%	4 Missing and 1 partial ⚠️
core/rawdb/accessors_preconf.go	95.55%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@                  Coverage Diff                  @@
##           v2.10.2-candidate    #2427      +/-   ##
=====================================================
- Coverage              57.93%   57.46%   -0.48%     
=====================================================
  Files                    957      957              
  Lines                 177763   175548    -2215     
=====================================================
- Hits                  102984   100875    -2109     
+ Misses                 69118    69009     -109     
- Partials                5661     5664       +3     

Files with missing lines	Coverage Δ
core/rawdb/schema.go	37.28% <ø> (ø)
eth/sequencer/exec.go	92.23% <100.00%> (+0.23%)	⬆️
core/rawdb/accessors_preconf.go	88.80% <95.55%> (+3.41%)	⬆️
eth/sequencer/consumer.go	85.75% <68.75%> (+0.07%)	⬆️
eth/sequencer/audit.go	94.16% <93.12%> (-0.77%)	⬇️

... and 42 files with indirect coverage changes

Files with missing lines	Coverage Δ
core/rawdb/schema.go	37.28% <ø> (ø)
eth/sequencer/exec.go	92.23% <100.00%> (+0.23%)	⬆️
core/rawdb/accessors_preconf.go	88.80% <95.55%> (+3.41%)	⬆️
eth/sequencer/consumer.go	85.75% <68.75%> (+0.07%)	⬆️
eth/sequencer/audit.go	94.16% <93.12%> (-0.77%)	⬇️

... and 42 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[cffls]

Confirmed the gap against the base independently of the write-up: auditHeightInto treats NOT_FOUND as "nothing was promised", recordVerdict no-ops on auditNoSeal, and the consumer's served state is purely in-memory (the only durable consumer-side writes are invalidations, written at invalidation time) — so a node that served a preconf and then lost the process leaves the broken promise unrecorded. Scenario 2 is real, and this PR targets exactly the three branches that let it through, plus the retention-skipped range that nobody would have noticed leaking. Writing the commitment before the receipts reach callers, IfAbsent so the live path's stronger record keeps precedence, treating a malformed record as an error rather than "nothing served", and refusing to let an unusable seal clear a commitment are all the right calls.

One thing I'd change before merge: the commitment is weaker than the live judgement it stands in for. foldServed commits to transaction identity and order only, while entryMatchesCanonical also checks the execution context (ParentHash, Time, GasLimit, BaseFee, Difficulty) and the receipts. That leaves a slice of scenario 2 open on exactly the handover this fix targets — details inline at audit.go:421. Cheap to close (seed the fold with the header context) and it makes the two paths agree by construction.

+1 to Pratik's block == nil point; inline at audit.go:413.

Also: codecov is at 86.5% patch vs the 90% gate, mostly consumer.go (57%) — the different-parent test below plus a test for the live-path clear in markCanonicalHeadAudited should carry it over.

[kamuikatsurgi]

Thanks both — all addressed in c92eedf:

• Execution-context fold (@cffls): foldServed is now seeded with contextSeed(header) (ParentHash, Number, Time, GasLimit, BaseFee, Difficulty — the sameExecutionContext fields), seeded identically on serve and audit, so a handover context change trips the fold even with identical leading txs. + TestAuditReconcilesServedMismatchOnDifferentContext.
• block == nil conflation (@ppatil, @cffls): judgeServedAgainstCanonical is three-way — nil/pruned body → uncomparable (counted, commitment left, no record); only a present-but-divergent block records. + TestReconcileServedLeavesUnjudgeableCommitment.
• Serve-path write (@ppatil): added a sequencer/preconf/servedpersist timer; durability rationale in-thread.
• Coverage: added the different-context, unjudgeable, live-path-clear, persistServed, and ReadPreconfServed read-failure tests.

Ready for another look.