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.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (86.45%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #2355 +/- ##
===========================================
+ Coverage 55.56% 57.17% +1.61%
===========================================
Files 918 953 +35
Lines 167125 174875 +7750
===========================================
+ Hits 92864 99990 +7126
- Misses 68786 69219 +433
- Partials 5475 5666 +191
... and 31 files with indirect coverage changes
🚀 New features to boost your workflow:
|
Contributor
Author
|
@claude review |
Contributor
Author
|
@claude review |
There was a problem hiding this comment.
Beyond the new RATE_LIMITED finding in the inline comments, I also checked whether payload-building/pending-snapshot paths (generateWork, RPC eth_call-style snapshots) could leak into the sequence store: commitTransaction gates publishing on w.sequencingActive(env.header.Number), which is only true for actual block production (the production flag is set solely by commitWork), so non-production commits never call PublishTx. That path is fine.
Extended reasoning...
This run's automated review found one new issue (RATE_LIMITED handling in the seal gate transport, eth/sequencer/stream.go) which is already attached as an inline comment. I additionally verified a candidate concern raised by a finder agent — that generateWork/payload-building paths might publish transactions to the sequence store even outside real block production — and confirmed it is not an issue: commitTransaction only calls sequencer.PublishTx when w.sequencingActive(env.header.Number) is true, and that flag is derived from the production bool which commitWork alone sets (payload-building/pending snapshot paths never set it). This is purely a record of what else was examined; it is not a substitute for addressing the RATE_LIMITED finding.
cffls
force-pushed
the
cffls/sequence-publisher
branch
from
August 24, 2026 18:51
51bd895 to
9967e98
Compare
cffls
added a commit
to 0xPolygon/pos-workflows
that referenced
this pull request
Aug 25, 2026
The first run built bor from sequencing, which does not carry the publisher yet (0xPolygon/bor#2355 is unmerged) — no sequencer metrics exist on such a build and test 1 reads 0/4 forever. Build from the PR branch until it merges (then sequencing, then develop), and on a test-1 failure print each validator's sequencer series count and publish state, telling a sequencer-less image apart from a wrong-state publisher. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Member
|
Local codex reviews left the following points. Let me know if you need the expanded version
|
Member
|
Cross verified |
Contributor
Author
|
Thanks for the thorough review, @pratikspatil024 — these were high-signal. Dispositions below; the fixes are in 0b5d8c7. Fixed in this PR (0b5d8c7):
All three were validated end-to-end on a 4-validator kurtosis rig: before the fix, non-producing validators returned Deferred / not blocking:
|
… and consumer Block producers publish each block's lifecycle (open context, per-tx records, sealed header) to the sequence store as it happens; RPC nodes follow the stream, re-execute it deterministically, and hold preconfirmation receipts for the block being built. Design doc: docs/sequencer-bor.md. Producer (eth/sequencer.Publisher, miner hooks): read-before-write follow model — a foreign unsealed window on our tip is followed, not superseded; the only supersede is the seal flush that makes the store match sealed truth. Pre-seal barrier awaits sequencing; the post-seal gate turns store acks into broadcast verdicts (foreign seal refuses, budget expiry broadcasts for liveness after a recheck). Recovery is the reconcile position ladder (anchor, block-anchor probe, floor read) with delta-only re-anchors; producer rotation adopts the dangling window instead of revoking it. Transport hardening: bounded in-flight sends with ack refill, ack-stall watchdog, and a self-heal redial after prolonged channel silence. Consensus-side: a signer outside the active producer set no longer builds at all, so its sequence can never reach the store. Consumer (eth/sequencer.Consumer): follows the gateway stream with warm/cold resume, verifies the commitment chain per entry, re-executes on canonical or parked speculative state (author-nil EVM context, speculative BLOCKHASH, EIP-2935), cross-checks seals (context, gas, receipts root, state root), voids-and-skips on divergence, and fills a capped receipt index evicted on canonical import. The RPC read path that serves these receipts ships separately. Everything is gated behind the [sequencer] config section; the role derives from the sealer flag (mining node publishes, non-mining node consumes). With the section unset there is no behavior change. Validated on kurtosis devnets: a 12-phase chaos campaign (store component restarts, pauses, 200s outages, flapping, partitions, producer and heimdall kills) ended with zero store gaps, zero revoked or reordered preconfirmations, and zero absent heights across 859k entries; preconfirmation receipts measured at p50 ~100ms against ~2.5-2.9s canonical inclusion at 4s blocks, byte-consistent with canonical receipts after import. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ed functions Raise the patch coverage of the sequence-store integration from ~81% to ~91% (local measurement). The consumer gains an execution-level harness: a real imported chain re-executes speculative blocks through the session (canonical and parked parents, every void-and-skip path, the checkSeal divergence matrix, speculative BLOCKHASH resolution), and a stream-level suite follows a live devstore end to end — receipts served pre-seal, evicted on canonical import, and an exact warm resume across a store restart. The worker's barrier-refusal and fill-halt cycles and the backend's role dispatch are covered directly. Decompose the functions the complexity gate flagged — ConfirmSeal, backfillLocked, reader.walk, restoreAbandonedDebtLocked, floorRead, runStream — and split AdoptWindow and SealBlock into their orchestration and resolution halves. The backfill debt machinery moves to debt.go, the build-start read and sealed-height recovery to buildstart.go, and the worker's sequencer integration to miner/sequencer.go, bringing classify.go, adoption.go, and worker.go back under the size gates. No behavior changes. Mutation tiers hold at T1 84.9%, T2 61.7%. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… edge rows Drive the build-start read through every store shape it classifies — unreadable, empty, behind (live window above, seal edge below), sealed with the chain's block, sealed inside and past the recovery grace — and pin the recovered window's exact content. Cover the gate's last-look recheck against sealed generations and live windows, the refusal streak cap and flush unwind, the backfill drain's pruned-jump, byte-budget, and undrainable-debt rows, prime-and-merge debt bounds, the reoffer and regrown-window paths, walk absorb hooks, probe edges, and the worker's store-owned-height discard mid-fill. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Re-arm the finality grace when sync completes: commitWork returns on syncing before it consults the gate, so a resync outliving the grace consumed it silently and opened the gate the moment sync ended — the restart window the gate exists to cover. The anchor is now re-armed in the miner's DoneEvent/FailedEvent handling, before builds unblock. Give the consumer's cold resume ladder distinct rungs (block anchor, then earliest) instead of retrying the identical block anchor before falling back. Fail the build-start read loudly when probeDown violates its contract instead of misclassifying the height as sealed past. Register sequencer flags against defaults when an HCL/JSON config has no sequencer block, instead of dereferencing nil at startup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Heights at or below a canonical whitelisted milestone are final: immutable and permanently served by the canonical chain, so their store copies serve no consumer. The backfill drain now starts past the milestone — a 1-hour store outage owes seconds of blocks, not the hour — with the skipped prefix crossed by the jump open as a counted forward jump. Without a usable milestone (Heimdall down alongside the store, or a milestone naming a chain we don't hold) a 40-block depth cap below the tip bounds the drain instead. The floor is a fact about our own chain, never a belief about the store: storeSealedTip remains untrusted, so the devnet failure class that removed the earlier freshness bound (skipping heights the store had actually shed) does not reopen — below finality the hole is now intentional. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…arm wiring The drain with a floor inside the pending range (finalized prefix dropped, remainder rebuilt) and the miner's sync-completion re-arm of the finality grace were the two untested branches of the previous commits; codecov's patch gate flagged both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The two adoption-floor tests swapped a live worker's chainConfig to vary the bor config, racing the worker's newWorkLoop, which re-reads chainConfig.Bor on every veblop tick — CI segfaulted in CalculatePeriod when a tick landed inside the nil-Bor swap window. The floor computation moves to a pure helper (adoptionMinTime) the tests exercise directly; the end-to-end applyAdoption coverage keeps the worker's real config. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…nd pending state Addresses three review findings on the sequence-store publisher: - eth/sequencer: a live store window on a displaced parent (canonical reorg) now mutes the build instead of arming a sticky hold that could refuse to seal forever. The pre-seal barrier lets a muted build seal without a store mirror, and the seal flush supersedes the dead-parent window with sealed truth. - eth/sequencer: a partially adopted window whose transaction proves unexecutable on canonical state now seals on the executable prefix and lets the seal flush supersede the remainder, instead of the barrier refusing the block and re-adopting the same window forever. Adopted window timestamps are also bounded above (mirroring the consensus future-block limit) so a far-future timestamp cannot stall the build. - consensus/bor, miner: a signer outside the active producer set keeps its pending snapshot fresh again, so eth_call/eth_estimateGas against the pending block work on non-producing nodes. The build is instead kept out of the sequence store at the miner via a muted-build flag gated on IsAuthorizedSigner, so it publishes nothing and never contends the store's per-height election. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
cffls
force-pushed
the
cffls/sequence-publisher
branch
from
August 27, 2026 18:29
0b5d8c7 to
4f2727b
Compare
The test drove a store-owned height with only the fill-loop resync abort (resyncN) guarding the seal, while AwaitSequenced still reported the height uncontested — so under constrained CI scheduling the build could seal before the abort fired (or the started worker's background loop could seal on the shared recorder), failing "must never reach the seal hook". Marking the height contested makes the seal barrier refuse unconditionally, which is what a store-owned height means, so the invariant no longer races the scheduler. Reproduced the old flake at GOMAXPROCS=1 (120/120 fail); green 200/200 after. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…used tasks Two fixes from a devnet load-test freeze: a producer rotated out mid-span kept sealing, its network-rejected block was flushed to the store as sealed truth, and the rotated-in producer then discarded its own valid block against that seal while the chain sat frozen a height below. - eth/sequencer: the broadcast gate now consensus-verifies a foreign store seal before honoring it as ownership of a height, on all three refusal paths (the timeout recheck, a tail-read loss carrying the decoded header, and a header-less STALE loss, which fetches the standing seal once). A seal whose signer the engine rejects can never become canonical, so it is noise, not ownership — the block broadcasts. A consensus-valid seal (an in-flight winner, a twin) keeps its refusal, as does one that cannot be inspected, bounded by the existing refusal cap. The verifier is the engine's VerifySeal, wired through Publisher.SetSealVerifier; a node without one behaves exactly as before. - miner: a gate-refused block now clears its pendingTasks entry. Nothing else ever could — clearPending needs chain progress a refused height never makes — and the leaked entry read as sealing-in-flight to the veblop stall fallback (decideVeblopFallback), disabling the only recovery path while the chain was stalled at that exact height. One refusal became a permanent production stop. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A producer taking over a slot wedged for seventeen seconds on a devnet: its publisher's anchor was stale from idling muted through the previous producer's stint, and the pre-seal barrier read that staleness as "the store holds content this block does not cover", refusing every rebuild. Each refusal stacked another held open in the journal, which is exactly what forbids the anchor rebase (the boundary read only rebases a clean tail), and the transport's classifier was meanwhile holding those entries awaiting the very seal flush the barrier was refusing — a deadlock broken only by heimdall rotating production away as ineffective. The stall also tripped the milestone-lag rotation threshold, and the rotation's floored backfill jumps are what left the dangling windows and store gaps the auditor has been flagging as dropped transactions. The store itself can arbitrate the shape: one walk anchored at the parent proves whether anything stands at or past the new height. Nothing there means a seal can revoke nothing — the barrier passes (both in the coverage fall-through and under a sticky hold, where a takeover's own stale-prefix STALEs look identical to a competitor), the seal flush's STALE rebases the anchor right after, and the store converges. Any entry the walk returns, and anything unreadable, keeps the refusal: a live window at the height still mirrors, a competitor still refuses, and the contested-unreadable protection is untouched. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two liveness guards from an adversarial audit of "the store must never block production", both bounding paths that relied on the store or on heimdall behaving: - The pre-seal barrier had no analog of the gate's refusal cap. Every refusal shape converges when the store is consistent (adopt, mute, backfill, the clean-boundary proof), but a store answering inconsistently across reads — Byzantine, or a load balancer over replicas at different lags — can refuse every rebuild with no benign state ever showing, halting production at that height until heimdall rotates the producer away as ineffective. Funnel every pass/refuse through a per-height streak: past the cap, liveness wins and the seal proceeds, sized past legitimate convergence and below the rotation threshold so the node heals itself before consensus gives up on it. - The gate's consensus check on a foreign store seal runs on the miner's result loop, and the engine's span resolution behind it waits on heimdall availability with no deadline of its own. Heimdall being down already halts production by design in Prepare and Seal, so this adds no new halt — but the result loop freezing over a verdict is needless coupling. Bound the verification; a timeout is unverifiable and keeps whatever verdict stood without it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A loaded producer's pre-seal tail read can land while acks lag the store's committed tail: the anchor sits inside the open window, so the walk returns bare records without the window's open entry. The summary then carries no window and a head past the anchor, and sealMirror's last branch read that as foreign content — refusing the producer's own block, adopting its own window minus the in-flight tail, and costing the height one or two rebuild cycles (2-3s blocks under sustained load, each shaving the last in-flight transaction). The anchor rung's matcher already proves the walked suffix identical to the unacked journal in lockstep; carry that verdict out of the walk (tailInfo.suffixOurs) and let sealMirror compare the block against the journal when it holds — store tail = acked prefix + verified journal suffix means the store's window at this height is the journal's. Any mismatch or extension turns the matcher off and keeps the old adopt path; the mirror rule itself is unchanged. Verified on a 7-validator devnet under 60 tx/s of 32KB-calldata load: 58 refusals across prior runs at this shape, zero after; the new middrainmirror counter caught eight organic occurrences, none costing block time. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Extract sealMirror's journal comparison — at-head or verified mid-drain suffix — into journalMirror, mirroring how sealedThroughParent names its rung. sealMirror had grown past the size gate; the rung reads better with its own contract stated once. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…g on idle Two root causes behind the store-level reorg John's devnet reported at block 51933 (auditor reorg-class supersession with no chain reorg): a publisher stream that breaks on a healthy low-load stream, then resends and STALEs, then backs off long enough for finality to strand the flush behind a milestone-floor jump — leaving a store hole the auditor reads as a reorg. Ack-stall watchdog: stallTracker.sent() incremented inflight but never touched lastAck, so the deadline measured time since the last ack ever, not how long the current entry had been outstanding. The first send after an idle stretch longer than the deadline read as stalled the instant it went out, and the watchdog reset a healthy stream. sent() now restarts the clock when it opens a fresh wait (inflight 0->1); pipelined sends leave it, so a genuinely hung store still trips. Contention streak: the streak was only ever cleared inside the stale path, so a healthy session ending any other way left it intact — it survived across quiet stretches and each later blip paid the whole 8s ladder. Consolidated into a pure advanceContention that clears the streak whenever the session made progress, whatever ended it. Reproduced on a kurtosis devnet (8s blocks for the idle gap, 1s netem on the redpanda brokers for the slow ack): 43 false ack-stalls in four minutes on the old code, zero after. Both paths keep genuine-stall detection. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…ync waits (#2385) The call did real work for microseconds - decode, submit to the txpool, submit for preconf - and then held its rpc.SafePool slot while idling in the receipt wait for up to rpc.txsync.defaulttimeout. Since every RPC call is bounded to ep-size (40 per transport, with ep-requesttimeout at 0 disabling the unbounded fallback), that capped concurrent sync calls at the pool size and let a few long-held calls monopolise a transport for a whole wait window. rpc.Slot is a releasable hold on the pool, handed to a task by SafePool.SubmitWithSlot and published on the call context. awaitReceipt releases it on entry and reacquires on exit, so decode/submit and the response tail stay bounded and only the idling is exempt. Reacquisition is best effort: parking here would stop the caller draining the channels it waits on. Waiters are bounded instead by rpc.txsync.maxconcurrent (default 4096), checked before submission so a refusal means the transaction was never accepted. This also caps live chain-event subscriptions. The 100ms tick called the full GetTransactionReceipt, two DB point-misses before reaching the preconf index. Canonical arrival already comes in as a chain event, so the tick now reads only the pending view, with a 1s canonical backstop for an event that carried no matching receipts. The backstop also closes a gap: with preconfs off there was no poll at all, so a missed or receipt-less event meant waiting out the whole timeout. The duplicated blob-sidecar upgrade in SendRawTransaction and SendRawTransactionSync is factored into decodeRawTransaction; behaviour is unchanged. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
The endpoint took a single `limit` count and returned the most recent N invalidations, so callers could not ask which blocks in a specific window were invalidated. It now takes (from, to) block numbers and returns the invalidations in that range, newest first. Adds rawdb.ReadInvalidPreconfsInRange, which seeks on the complemented key (stored as ^number) instead of scanning the whole prefix, and caps the response at InvalidPreconfQueryLimit. Tags (latest/pending/finalized/safe) resolve to the current head; explicit heights pass through so records for non-canonical numbers remain queryable. from > to is rejected. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
eth, core: add sequence store preconfirmation pipeline and RPC support
eth, eth/sequencer: serve head-state pending in the preconf import gap
…oducers A consumer (non-mining RPC) sequencer node never uses the publisher endpoint -- NewConsumerWithTransactionLookup takes only the consumer endpoint -- yet sequencerSettings required both whenever the sequencer was enabled, forcing an RPC node to carry a publisher-endpoint it never touches just to pass startup validation. Require publisher-endpoint only for the producer role; consumer-endpoint stays required for both. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…imit (#2400) The send path coalesces up to 64 transactions into one published record (commit 89143d7). It bounded a record only by that count and the 32 MB pending-input limit, never by the store's Redpanda max.message.bytes, so a run of large transactions (e.g. 64 x 32 KiB) produced a >1 MiB record. That draws a permanent MESSAGE_TOO_LARGE on produce; the ingress treats a produce failure as a fence and takeover replays the same record, so one oversized record wedges the writer and nothing behind it is preconfirmed. Cap a coalesced record at maxRecordBytes (max.message.bytes less a 4 KiB reserve for per-transaction protobuf framing, the prefix commitment, and the ingress's own recordFraming allowance), so bor never builds a record the ingress would reject. A lone transaction over the cap is still emitted and left for the store to judge, not dropped. Also give the terminal MALFORMED ack path a clearer message: with the cap in place a MALFORMED means the store's max.message.bytes is below this build's record cap, not an oversized record. Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
* sequencer: stop paying for a store that is not answering The producer waited on the store on every block even when the answer it was waiting for could not arrive. A fault-injection campaign measured the cost: a frozen gateway took block production from 1.00 to 0.47 block/s and a frozen ingress to 0.75, against a design brief that says nothing in the store can affect block production. One flag caused both. unreachable was set only by publish-transport failure and cleared by, among other things, every successful read, so it was false during an ingress outage (the gateway kept answering reads) and false during a gateway outage (nothing but the transport ever set it). Each outage went on paying the other path's waits. Split it. writeDown covers the publish path and is cleared only by an arriving ack, which is the only evidence an ack can arrive: a stream session ending is not, because the held-build watchdog ends one every 400ms from a local timer. A breaker on the reader covers the read path, refusing reads once the path has gone quiet and letting one probe per interval look for it again, on a deadline sized for the question it asks rather than for returning a usable tail. Giving up on the ack resolves what the wait would still have resolved — the store's own record, then the chain, where a rival's block at our height is the rejection notice — so a height nobody contested is not refused for want of an ack. Measured on a kurtosis devnet at 1s blocks, pausing each service in turn: ingress frozen goes 0.75 -> 0.97 block/s with seal elapsed 503ms -> 53ms, and gateway frozen 0.48 -> 0.78. The read path keeps a residual: with no read there is no anchor, so no ack arrives and the seal gate still spends its budget while writeDown is legitimately false. Left alone rather than skipping the gate while unanchored, since a STALE unanchors and a STALE means contention, which is when the gate matters most. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * sequencer: give up the seal gate when the read path is down too The gate waits for the store's verdict on a sealed block: an ack, a STALE, or the deadline. It already gave up when the publish path was down, because no ack could be delivered. It kept waiting when the read path was down, where no verdict can arrive either: reconcile reads the tail to anchor, so a silent gateway leaves the transport unable to publish anything to be acked, and the recheck at the deadline is a read the breaker refuses. On a devnet with the gateway frozen, 21 of 47 blocks paid the full budget and settled Unknown, the contested ones paying four seconds to reach the same place. The breaker is the signal, not staleness: only silent reads trip it, so a contended store that still answers - NOT_FOUND included - leaves the gate fully armed. Skipping it during an outage degrades to what a node with no store at all already does, which is a possible fork that consensus resolves; the store's coordination is gone in that state regardless of what the gate does. An ack already delivered still wins, since storedVerdict runs first; what is given up is one in flight. A withheld seal keeps its full wait. refuseOnTimeout is set from a read that already found the height closed in the store with content this block does not carry, so its refusal rests on evidence in hand rather than an answer still owed, and broadcasting over it is the displacement the gate exists to prevent. Read-path outage on a 4-validator devnet: 0.78 -> 0.98 block/s, seal mean 297ms -> 2ms, max 4002ms -> 11ms, against a 1.00 block/s baseline sealing in 4ms. The unexplained 4s outlier in the previous measurement was this gate going contested. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ot watch (#2388) * sequencer, rawdb, ethapi: audit the store for the window a node did not watch bor_getInvalidPreconfBlocks reported a node's downtime as clean no matter what the store held during it. Invalidation records come from one place - the in-memory PendingStore reconciled against canonical blocks - and a node that was down holds no entries for the heights it missed, so nothing is compared and nothing is recorded. Silence was indistinguishable from every preconfirmation in the window having held. Nothing recorded where the consumer last watched either. On a cold start resumeRequest anchors at the local canonical head, so the window between the last watched height and the current head was never requested, and the store's caught-up-to-tip frame was received and discarded rather than ending a catch-up phase. An audit pass now walks the unaudited window and compares the store's final sealed header at each height against the canonical hash, recording a mismatch as unobserved_mismatch: the store sealed something that never became canonical, and this node served nothing from it. The other reasons all mean a preconfirmation reached callers and was then invalidated, which is a stronger claim, so it gets its own reason rather than sharing theirs. The comparison needs no execution and no historic state, which is what makes it viable on a pruned node, and it runs on its own connection off the session loop so closing a gap never delays a reconnect. It is triggered on consumer start, on every session loss, and when a stream reaches the tip. Session loss gets a pass because a stream that drops while the node stays up leaves the same kind of hole as a restart. PreconfAuditedThrough carries the position across restarts, and it may only step one height at a time from the live path: a jump would carry it over the catch-up backlog the session dropped without comparing, so a gap asks for a pass instead. PreconfUnauditedThrough records a window the depth bound skipped, so an empty invalidation range there reads as unknown rather than clean. A node with no watermark seeds at the current head and audits nothing - auditing backwards from an arbitrary point on first enable would produce records with no operational meaning. GetBlock returns a height's whole generation, transaction records included, so a wide walk pays for payloads the seal comparison never reads. That is why sequencer.audit-window bounds one pass, defaulting to roughly an hour of blocks. The canonical-head reconciliation and the backlog helpers move into their own files to keep consumer.go and consumer_session.go inside the repository size checks, matching how exec.go was kept separate on this branch. Separately, an open more than backlogOpenDepth below the canonical head is now dropped without the parent-state lookup that fails on a pruned node, and without skip's reset of the speculative tip - nothing was ever published for a height the chain already holds, so there is nothing to invalidate. A producer rebuilding the tip after a rotation lands within a block or two of the head and still executes; its generation can yet win the height. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * sequencer: cover the audit pass end to end and keep the diff off moved code The audit pass was only reachable through its injected fetch hook, so runAuditPass itself - the dial, the per-height GetBlock, the guarded watermark write - was almost entirely untested. It now runs against a real ConsumerService server on a local listener, with the unreachable-endpoint and nothing-to-audit cases alongside it; the latter asserts no connection is opened at all, since a trigger fires on every session retry and must not each cost a dial. Filling the remaining branches turned up what the mutation run had been pointing at: nothing asserted the pass's own counters, the window depth boundary, the checkpoint write, recordVerdict's four verdicts, or what happens when the database refuses a write. The canonical-head handler and the live marker were covered only through their helpers, so removing either call site went unnoticed. behindHead is split out of behindCanonicalHead so the depth boundary is testable without a chain deep enough to reach it. Reverts the canonical_head.go extraction from the previous commit. Moving evictLoop and the canonical-head reconciliation into a new file made every one of those pre-existing lines count as added, which pulled code this change does not touch into both the patch-coverage and the mutation denominators. The backlog helpers stay in backlog.go - that file holds only new code. consumer.go returns to 544 lines, inside the 800-line check CI applies. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * rawdb, sequencer: tell an unreadable audit watermark apart from an absent one readPreconfHeight returned the same (0, false) for a missing key, a failed read, and a value that was not eight bytes. windowToAudit treats that sentinel as "this node has never audited" and seeds the watermark at the current head, so a read-only failure marked every height in the unaudited window as compared - and because both write paths are monotonic, the mark could never be walked back. bor_getPreconfAuditStatus then reported auditedThrough=head with no gap: the false-clean answer this feature exists to prevent. The read now probes with Has, then Get, then the length, and returns an error for anything that is not a clean present-or-absent answer. Absence still seeds the watermark; a failure aborts the pass, and persist and advanceAudited hold rather than write over a value they could not compare against. The RPC returns the error instead of an absent mark. WritePreconfUnauditedThrough treats an unreadable current mark as absent and writes anyway, since recording a known gap beats leaving the window unrecorded. behindHead compares by subtraction. The height arrives from the store, so number+backlogOpenDepth could wrap for a value near the top of the range and read as backlog. TestAuditPassSkipsTheDialWithNothingToAudit could not fail on the behaviour it was named for: with the watermark at head, deleting the pre-dial short-circuit leaves run to recheck the window and return, so the watermark stays put either way. Counting accepted connections does not fix it either - grpc.NewClient is lazy, so no connection is attempted when run returns first. The short-circuit saves a client and a per-retry warning on a bad endpoint, not a connection; the comment now says so and says there is no test for it, and the test asserts what is true: no reads and no connections when there is nothing to audit. The same trap caught the first version of the read-failure test, which asserted that no window resolved - true whether the read failed or reported absence. It now asserts that nothing was written, and was confirmed to fail against the old code by finding the watermark seeded at the head. The audit-window flag's help text says that zero uses the built-in window; docs regenerated. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * sequencer: audit once per session, report what it compared, drop the backlog change Three findings from a kurtosis devnet with the store live, none of which the unit tests could see. The audit ran every two seconds. Pre-Rio, deterministic() fails, so run() retries every consumerRetryDelay, and requestAudit() sat at the top of that loop - so every retry built a gRPC client, walked the store, and tore the client down, indefinitely, for any node sitting pre-Rio or with a persistently failing stream. The trigger moves into runSession, which asks for a pass only after a session that actually ran. Measured on the devnet: 66 pre-Rio session retries now produce 0 audit passes, against roughly one per retry before. The summary could not distinguish a height the store held nothing for from one it compared. "walked=129 mismatched=0" read as a clean audit of 129 heights when the first post-Rio pass had actually compared 2 - publishing starts at Rio activation, so the rest were NotFound. walked and compared are now separate, and the log carries both. The backlog-open drop is removed. It never fired: it needs p2p import to run 64+ blocks ahead of the store stream, and across a 330-block downtime the stream kept pace, so behindCanonicalHead never returned true. The pruned-state warning storm that motivated it did not reproduce on either build. An earlier comparison suggested a 20x reduction in skipped opens, but that control was built from feat/pbc-rpc-endpoints before #2373's later parent-resolution work - the code being counted - and against the PR's real base the difference disappears. That leaves a behaviour change in the preconf path, while preconfirmation coverage is being measured, with nothing exercising it. consumer_session.go is back to base as a result. Devnet evidence for what remains: a consumer stopped at watermark 272 under sustained load and restarted walked heights 278-451 in three passes with compared == walked throughout and no mismatches; with the gateway stopped, the head advanced to 1361 while the watermark held at 1299, and the gap closed on reconnect. Preconfirmations are unaffected - 50/50 preconfirmed, p50 117ms. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * sequencer: cover the watching reset and the audit loop's wiring Removing backlog.go took its exhaustive depth-predicate table with it, which had been killing a large share of the tier-1 mutants, so the quality gate fell to 77.3% on a pool that no longer contained them. Two of the survivors were real gaps rather than an artifact of the smaller pool. Nothing asserted that a returned session stops the consumer watching the tip. If it did not, the next canonical head would advance the audit watermark across a window nobody compared - the invariant a gateway outage exercised on the devnet, where the head ran from 1295 to 1361 while the mark held at 1299. The reset moves from run() into runSession() as a defer, which is where it belongs and makes it reachable from a test. Nothing asserted that Start wires the audit loop, so a restart would never close its window. The seeding pass reaches no further than the local chain, so the test drives Start against an unreachable store. Both tests were checked against the code they guard. The remaining survivors are log statements, the receive-buffer arithmetic in the client options, and one equivalent mutant: relaxing `number <= watermark` to `<` in markCanonicalHeadAudited falls through to advanceAudited, which is monotonic and writes nothing at that height, so no test can separate them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * sequencer, rawdb, ethapi: keep a backfill from overwriting a live verdict Three findings from re-reading the audit against questions its tests were not asking. The devnet campaign had already passed and the quality gate was green, so none of these were visible from either side. The audit could overwrite an invalidation the live path had already recorded. invalidPreconfKey is one key per height and the write was a bare Put, so a pass judging a height the live path had judged replaced it. The reasons correlate: a height whose served preconfirmation missed canonical is where the store's final seal probably missed it too, so this was reachable whenever a session dropped between the live record and the watermark advancing past that height. It ran in the worst direction - unobserved_mismatch asserts nothing was served from the height, so the ledger would have reported no user-visible impact where a preconfirmation had in fact been served to callers and then invalidated. The audit now writes only where a height carries no record, and counts what it left alone. The check and the write are not atomic; the accessor documents that rather than implying it away. A window the store held nothing for read as clean. NOT_FOUND cannot tell "the producer never published here" from "retention aged this height out", and the pass advanced the watermark across either - so a node down longer than retention walked its window, compared nothing, recorded nothing, and reported auditedThrough across the whole range. That is the same silence-as-clean this work exists to remove, arriving at the retention boundary instead of the downtime one. A run of NOT_FOUND at the oldest end of a walked window now raises PreconfUnauditedThrough, which already means not-compared; when the whole window is unheld the two marks meet. The rule stays narrow on purpose - a hole in the middle is the store having been down for those heights, not a floor - and a test pins that narrowness so it cannot widen by accident. getPreconfAuditStatus had no read-failure coverage, and an unreadable mark returned a nil status with no error, which serializes as null and reads as never-audited from the one method built to avoid a clean-looking answer. Now covered per mark, because corrupting both lets the first read short-circuit and leaves the second branch unexercised. One case of the same shape is left as it is. A height the store held but that could not be decided still advances the watermark unrecorded, and it cannot use these marks: unauditedThrough is a prefix, so raising it for one scattered height would declare the entire history below it uncompared. Recording it needs per-height state, which is a storage and API decision rather than a fix, so it stays counted and logged alongside the reorg and repair-path limits - all three documented for review rather than changed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * sequencer, rawdb, ethapi, cli: bound the store audit by retention Fold the audit's coverage of a range into bor_getInvalidPreconfBlocks as pendingFrom, so a caller needs no second call to tell an unaudited range from a clean one, and cap the range at 1024 heights instead of truncating the response: one record per height means bounding the request bounds the answer. bor_getPreconfAuditStatus goes with it. Replace the audit's block-count depth bound with the store's retention floor, read from a Range with after unset. A second bound on this side is one operators have to keep aligned with the store's, so the sequencer.audit-window flag and the PreconfUnauditedThrough mark go too. Heights below the floor, and any NOT_FOUND mid-walk, are logged and counted under sequencer/audit/ rather than marked. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * sequencer, eth: bound the audit watermark at finality Review asked for the walk to stop at the finalized head rather than the chain head. A verdict is only as good as the canonical chain it was reached against, and the mark never rewinds, so a height judged before a reorg replaced it would keep a verdict about a block that no longer exists. At or below a milestone that cannot happen, and heights above it report as pendingFrom, which is what they are. The canonical-head path needs the same ceiling. The review took it to be safe already, on the grounds that a reorg there produces a reorged record — but reconcileCanonicalLocked removes the pending entry once the height is reconciled, whether it matched or was invalidated, so a reorg arriving after that writes no record anywhere and the mark has already passed the height. Both paths now stop at finality. A node with no milestone source falls back to the head: bounding at a finality it cannot see would freeze the watermark forever. That is not the same as a source reporting nothing final yet, which judges nothing, so the two are distinguished rather than collapsed into one absent value. WhitelistedMilestone grows a nil guard. The consumer holds it and calls it from the audit loop, which Start launches during construction, so it cannot assume the handler is up; without the guard the wiring tests nil-dereference it, which showed up as one flaky package failure before it showed up as a test. Also per review: the floor read is one page and one rule, since the gateway's served window always begins at an open, and ReadInvalidPreconfs keeps its caller-supplied limit with no ceiling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Block producers publish each block's lifecycle (open context, per-tx records, sealed header) to the sequence store as it happens; RPC nodes follow the stream, re-execute it deterministically, and hold preconfirmation receipts for the block being built. Design doc: docs/sequencer-bor.md.
Producer (eth/sequencer.Publisher, miner hooks): read-before-write follow model — a foreign unsealed window on our tip is followed, not superseded; the only supersede is the seal flush that makes the store match sealed truth. Pre-seal barrier awaits sequencing; the post-seal gate turns store acks into broadcast verdicts (foreign seal refuses, budget expiry broadcasts for liveness after a recheck). Recovery is the reconcile position ladder (anchor, block-anchor probe, floor read) with delta-only re-anchors; producer rotation adopts the dangling window instead of revoking it. Transport hardening: bounded in-flight sends with ack refill, ack-stall watchdog, and a self-heal redial after prolonged channel silence. Consensus-side: a signer outside the active producer set no longer builds at all, so its sequence can never reach the store.
Consumer (eth/sequencer.Consumer): follows the gateway stream with warm/cold resume, verifies the commitment chain per entry, re-executes on canonical or parked speculative state (author-nil EVM context, speculative BLOCKHASH, EIP-2935), cross-checks seals (context, gas, receipts root, state root), voids-and-skips on divergence, and fills a capped receipt index evicted on canonical import. The RPC read path that serves these receipts ships separately.
Everything is gated behind the [sequencer] config section; the role derives from the sealer flag (mining node publishes, non-mining node consumes). With the section unset there is no behavior change.
Validated on kurtosis devnets: a 12-phase chaos campaign (store component restarts, pauses, 200s outages, flapping, partitions, producer and heimdall kills) ended with zero store gaps, zero revoked or reordered preconfirmations, and zero absent heights across 859k entries; preconfirmation receipts measured at p50 ~100ms against ~2.5-2.9s canonical inclusion at 4s blocks, byte-consistent with canonical receipts after import.