Repository navigation
Conversation
… authorizes The background header check skipped every tick on which the latest milestone was at or past the head, reading it as "still syncing". A rotated-out producer can sit there for good: an exact rebuild of its block by the new producer shares the block's state root, so on a node that has pruned the fork point the side-chain import refuses the canonical chain (a side block whose root matches the canonical block at its height cannot be verified there), while milestone handling defers because the node looks out of sync. Nothing else moves it. When the milestone is at or past the head, check the chain's own trailing headers (at most 256, above the higher of the whitelisted milestone and the finalized block) against the span, and rewind to the parent of the first one sealed by an unauthorized signer. The canonical chain then imports as an ordinary extension with full execution. Only an unauthorized-signer error rewinds; any other verification failure leaves the chain alone, since a node that is merely syncing passes through here on every tick. consensus gains an ErrUnauthorizedSigner sentinel that the bor unauthorized signer and proposer errors match through Is, so core can tell a provably invalid header from an inconclusive failure without importing consensus/bor.
… takeover When a producer is cut off and another takes over, the store holds the old producer's sealed run of heights, which consumers have already served as preconfirmations. Recovery rebuilt only the first of them: a bor seal signs the header, so the rebuilt block never reproduces the original hash, every later height names a parent this chain does not have, and those heights went to the pool in a new order, changing the results of preconfirmed transactions. Follow the run across a parent that differs only in its seal. The store's sealed parent and ours must agree on transactions, receipts, gas used, gas limit, timestamp and base fee. The state root is not compared: EIP-2935 records the parent hash in state, so roots differ after the first rebuilt height even when every transaction executed identically. Supporting changes found while testing takeovers on a devnet: - A recovered sealed generation is rebuilt exactly (AdoptedWindow.Complete): pool transactions appended after it would change the state the store's next sealed height executed on. - A dead producer's live window on an equivalent parent is seeded in the store's order, and the seal flush may displace it without the chain's proof when it carries the window in full, in order. - A recovery build passes the seal barrier: it is the store's sealed generation, and the tail read could show a newer window of the dead producer as owed, looping the rebuild until the height went to the pool. - Once the probe finds our height sealed in the store, the walk and the recovery fetches get their own budget instead of what is left of the 250ms build-start deadline, which a CPU-starved takeover producer misses. - The frozen-tail read breaker is suspended while following a dead producer's run, its probe asks the store directly whether anything follows the frozen head, and a staleness hold lifts after a bound, so a sole producer holding its own publishes cannot stay blind forever. - Debug logs for every build-start classification and every reason a sealed height is not recovered.
cffls
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #2480 +/- ##
===========================================
+ Coverage 57.78% 57.90% +0.12%
===========================================
Files 966 967 +1
Lines 176613 177189 +576
===========================================
+ Hits 102048 102606 +558
- Misses 68907 68926 +19
+ Partials 5658 5657 -1
... and 24 files with indirect coverage changes
🚀 New features to boost your workflow:
|
![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.
Background header verification rewinds the chain off the first header it rejects, but reaches that verdict outside the chain lock. A valid chain imported, or a milestone whitelisted, between the verification pass and the rewind makes the verdict stale, and the rewind then drops blocks it never judged, finalized ones included: a chain finalized at 9 was rewound to 5 and lost its finalized marker. Both rewind paths, the stalled tail and the pending headers above the milestone, now go through rewindOff. It rewinds only if, once the chain mutex is held, the rejected header is still canonical and its parent is still at or above the finality floor, and it keeps the mutex until the rewind is done. setHeadBeyondRoot takes optional guards for this, checked right after it takes the lock; its existing callers pass none.
The takeover bridge followed the store's run across a parent that matched ours in transactions, receipts and execution context, without comparing state. Matching receipts do not prove matching state: a transaction can store a BLOCKHASH result, which differs under another seal, and keep its receipt identical, and a later transaction can diverge on the stored value without reading a block hash itself. The parents' states must now agree apart from the EIP-2935 history slots, the one difference a rebuild under another seal cannot avoid. Our parent's state gets the store chain's hash written into the history slot of each height the two chains do not share, and its root must then equal the store parent's exactly. The store's hashes for earlier heights come from the sealed headers the bridge read at each step, kept for up to 1024 heights; a missing or mismatched link can only fail the check. The state is opened with SealingStateAt: StateAt serves a pipelined import still committing as an overlay, whose root leaves the block's own writes out.
|
@claude review |
rewindOff's guard only checked the requested target against the finality floor, but rewindHead keeps walking back past blocks without state. On a hash-scheme node with pruned fork-point state, that is the case the stalled-tail check targets, the head could land below the whitelisted milestone or finalized block. The guard now also requires state between the floor and the target, using the same availability test rewindHead does, under the chain lock and before any marker is written. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ed it p.bridged was set when a build seeded a dead producer's live window across an equivalent parent, and never cleared. A later build at that height on a parent that doesn't bridge could then match the stale record, and its flush displace the window without proof from the chain. A new build at or below the record's height now drops it. The record still outlives the seal: the flush that relies on it reconciles after the next height's build has started. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each recovery read (the walk at our height, the sealed generation and the bridged parent) started its own 1s deadline, so a slow store could hold a takeover producer's build start for about 3s, past the block period. AdoptWindow now attaches one budget that the first recovery read starts and the rest share. It stays detached from the 250ms build-start deadline, so a CPU-starved takeover still gets its recovery reads. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The history check followed the store's run only as far as the links the bridge had read in this process. A takeover producer restarted mid-run came back with none, so the check failed at the next height and every later one was rebuilt from the pool. The check now walks height by height, and reads a missing link's sealed generation back from the store on the build start's recovery budget. The header must have the hash its child names as parent; a missing or mismatched read still ends the walk. Backfilled links keep the ones above them, unlike a fresh read of the store's tail. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
pratikspatil024
left a comment
pratikspatil024
left a comment
There was a problem hiding this comment.
Two high-priority finality findings from static analysis; reproductions have not been executed.
The finality precheck only asked whether a block between the target and the floor had state. rewindHashHead checks its search limit (the pivot, or head minus FullImmutabilityThreshold) before state, so a persisted pivot above the target sent the head to genesis, below the floor. The precheck now replays rewindHead's cutoffs in its order on each scheme. Also correct rewindOff's comment on milestones whitelisted while the rewind runs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A tail that stops moving after sealing past our own head is a producer we haven't caught up with, not a wedged store. Counting it tripped the read breaker just before a late takeover, so the new producer started its span degraded. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A producer elected again muted every build at or below its sealed tip, even when the chain had moved off the blocks it sealed there. Record the parent we sealed each height on, and when a build under the tip sits on a different parent, drop the tip to that build's parent and read the window from the store instead. A build on the parent we sealed on is our own seal racing the head update, and stays muted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A build muted because our signer couldn't seal its height publishes nothing to the sequencer. If a span update authorizes the signer before the build finishes, sealing it would put a block on chain that never read the store. Refuse it at the seal barrier so the next build-start read follows the store. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@claude review |
spanByBlockNumber falls back to the older of two overlapping spans when the lookup for the newer one fails, and the latest-span cache keeps its old value while Heimdall is unreachable. Right after a rotation, either makes the new producer's valid blocks look unauthorized, and the stalled-tail check would rewind them. The span store now reports whether the newer-span search settled: it reached a latest span fetched in the last 2s, or ran past every span that could hold the block, with no failed lookup. A Rio snapshot built from an unsettled span marks its unauthorized-signer and -proposer errors so they no longer match consensus.ErrUnauthorizedSigner. Block import and the miner see the same error types as before. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| func (p *Publisher) sealedRunLeftLocked(number uint64, parent common.Hash) bool { | ||
| ours, ok := p.ownRun.at(number) | ||
| if !ok || ours.parent == parent { | ||
| return false |
There was a problem hiding this comment.
This misses the case where we rewind the whole run and start again on the original parent. The parent still matches, but the blocks we sealed are gone.
I tried sealing heights 1 and 2, rewinding to their parent, then rebuilding height 1. The store read finds the transactions, but AdoptWindow returns nil and keeps sealedTip=2, so we rebuild from the pool and lose their order.
Can we distinguish that rewind from a seal that's still waiting to be imported? Parent equality alone doesn't tell us which happened.
There was a problem hiding this comment.
Right, parent equality can't tell those apart. Fixed in 9ceb185, which tracks the height of the latest build start and raises it on any seal above it. Sealing a height needs the chain at that height's parent, so a build below it can only follow a rewind, and it now reads the run from the store. Your case (seal 1 and 2, rewind to their parent, rebuild 1) recovers both heights in the store's order. So does sealing 1 and 2, building 3 on our 2, rewinding, then rebuilding 2.
A build at our latest seal height on the parent we sealed on still stays muted. Until a build starts on our block, that's the same thing the chain sees when our seal is still waiting to be imported, so this one height is the only case still ambiguous.
Test: TestRewoundOwnRunIsRebuiltFromTheStore.
| if !bytes.Equal(tail, b.lastTail) { | ||
| b.lastTail = append(b.lastTail[:0], tail...) | ||
| b.freeze = 0 | ||
| if b.stale { | ||
| b.open, b.stale, b.failures = false, false, 0 | ||
| readBreakerClosed.Inc(1) | ||
| log.Info("Sequencer store tail is advancing again") | ||
| if seal != 0 { | ||
| b.lastSeal = seal | ||
| } |
There was a problem hiding this comment.
Could we update lastSeal outside the tail-change check? A read starting just after the seal can see the tail without learning the seal height. If a later read finds the seal but ends at the same tail, we still keep lastSeal=0.
With the store sealed through height 2 and our head at 0, this trips the breaker after six reads, even though we're just behind the producer. Moving the seal update before this check should let the new exemption work in that case too.
There was a problem hiding this comment.
Agreed, fixed in 49fb8b4: lastSeal is now updated before the tail-change check, so a later read of the same tail can still set it. Added your case (the first read sees the tail with no seal, a later one sees it sealed above our head) to TestReadBreakerFrozenRunAboveChain; it stays closed.
0xrukimedo
left a comment
0xrukimedo
left a comment
There was a problem hiding this comment.
I’d fix the span lookup before merging, since it can make us rewind valid blocks.
| // Spans start in id order, so two starting past the block rule out every later span. | ||
| if blockNumber < span.StartBlock { | ||
| skippedSpans++ | ||
| if skippedSpans > 1 { | ||
| if candidateSpan == nil { | ||
| return nil, fmt.Errorf("span not found for block %d", blockNumber) | ||
| } | ||
| return candidateSpan, nil | ||
| return spanOrNotFound(candidateSpan, true, blockNumber) |
There was a problem hiding this comment.
I'm concerned about settled=true here. Newer spans can start earlier than older ones; Heimdall even has a test for that.
For block 400, I tried span 1 covering it, spans 2 and 3 starting at 7000 and 7100, and span 4 starting at 300. We stop at span 3 and return span 1 as settled. The actual span-4 producer then gets ErrUnauthorizedSigner, which can now trigger a rewind of valid blocks.
Can we leave this result unsettled unless we've ruled out the later spans? The old shortcut becomes risky now that a rewind depends on it.
There was a problem hiding this comment.
Agreed, thanks for the Heimdall reference. Fixed in 51ec0a7: the two-skipped-spans shortcut now always returns unsettled, so a settled verdict needs a walk all the way to a fresh latest span. The shortcut still serves the plain lookup for import as before. Your layout (span 1 holding 400, spans 2 and 3 at 7000 and 7100, span 4 at 300) is now a case in TestSettledSpanByBlockNumber. The lookup still returns span 1 there, a limit of the shortcut that predates this PR, but it's unsettled, so a signer it rejects doesn't match ErrUnauthorizedSigner and nothing rewinds.
getFutureSpan stops once two spans in a row start past the block. A newer span can still start before older ones, so a later span may hold the block, and a rewind must not rest on that shortcut. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A read can reach the tail before it learns the seal under it. Updating the seal only when the tail changed kept it at 0, so a node just behind the producer still tripped the frozen-tail rule. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A rewind back to the parent our run started from leaves every parent matching, so the sealed-run check kept the build muted and it was built from the pool. Sealing a height needs the chain at its parent, so a build below our latest build start or seal follows a rewind; it now reads the run from the store. A build at our latest seal height on the same parent is still treated as our seal racing the head update. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| p.mu.Unlock() | ||
|
|
||
| switch { | ||
| case recovering: |
There was a problem hiding this comment.
A small hardening idea on the recovery path:
When we rebuild a dead producer's sealed generation, recoverWindowLocked sets modeRecover and Complete = true. From there, nothing seems to check that the block we seal actually contains the whole window:
seedAdoptedskips any tx that fails to apply (continue), and withCompletethe worker returns right after it.- This
case recovering:returnstruebeforemirrorVerdictruns. That's the check that, for normal builds, makes sure "a dropped executable transaction must still refuse". SealBlocktakes therecoveredpath: it publishes nothing and clears the gate, so the block broadcasts.
If a tx is dropped, we'd broadcast a block at N whose content differs from what the store says is sealed at N, with no supersede to fix it. The bridge would fail closed at N+1, so it's bounded, but the seedAdopted doc relies on "the publisher's expectation matching" to catch exactly this, and on this path that matching doesn't run. (A declined adoption in applyAdoption also leaves p.mode as modeRecover while the worker pool-fills, though that one at least logs a Warn.)
I agree this should be rare, since the bridge already proved the parents equivalent. But since the safety argument now leans on the rebuild being exact, what do you think about checking it here? For example, keep the window's tx hashes on p.mode and only short-circuit when txs matches them exactly, otherwise fall through to the normal mirror/gate path. A unit test with one dropped tx in a Complete build would pin it.
There was a problem hiding this comment.
Good catch, fixed in e9237aa. One thing differs from the suggestion: falling through to the mirror wouldn't catch it. A recovery publishes nothing, so sealMirror reads the tail, sees the height already sealed in the store, and passes ("height already sealed in the store").
So the check lives in the recovering case itself. The recovered generation's tx hashes now ride on p.mode, and the barrier passes a recovery only when the block's transactions equal them exactly. Anything else (a dropped tx, a pool fill after applyAdoption declined the window, a reordering) becomes the over-sealed fallback (modeOverSealed). That's the existing path for sealing a store-closed height with content the store's seal doesn't describe, so SealBlock now publishes the block's flush and arms the gate with tolerateSealed instead of clearing it. The mismatch logs a Warn and counts sequencer/publish/recovered/inexact. This also covers your parenthetical about a declined adoption left in modeRecover.
I didn't refuse the block. The drop is deterministic on our parent, so every rebuild would recover and drop the same tx again until the refusal streak sealed it anyway. The fallback keeps every other tx in the generation's order, and the bridge still fails closed at N+1 since our N's transactions differ.
Test: TestBarrierPassesRecoveryBuild is now table-driven (exact, a tx dropped, filled from the pool, reordered) and checks the mode and the gate SealBlock arms. I also updated the seedAdopted doc.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Matches #2504, which took the same snapshot for readTail, recoverSealed and the gate: equivalentParent runs before AdoptWindow takes the lock. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A recovery build passed the seal barrier unchecked and sealed without publishing, so a block missing a transaction of the sealed generation (one failed to apply, or the worker declined the window and filled from the pool) broadcast as if it were the store's. The barrier now passes a recovery only when the block carries the generation exactly; anything else becomes the over-sealed fallback, whose seal flush and gate account for it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| return nil, false, err | ||
| } | ||
|
|
||
| fresh := time.Since(time.Unix(0, s.latestSpanAt.Load())) < latestSpanFreshness |
There was a problem hiding this comment.
A latest-span cache younger than two seconds can still be outdated immediately after an overlapping span rotation. Returning settled=true can then make a valid producer match ErrUnauthorizedSigner, allowing the stalled-tail check to rewind valid blocks. The canonical-hash and finality guards do not recheck authorization.
Could we require a newly fetched authorization verdict before a destructive rewind and add a test for rotation immediately after a cache refresh?
There was a problem hiding this comment.
Agreed: a cache refreshed within the last two seconds only proves the poll loop got an answer recently, not that Heimdall committed no span since. Fixed in 7394f94.
Before rewinding, verifyStalledTail now calls the engine's new RefreshAuthorization (Bor fetches the latest span from Heimdall, with a 5 s timeout) and verifies the rejected header again. It rewinds only if that header is still ErrUnauthorizedSigner. If the refresh fails, the chain is left alone. Import and the other verification paths don't change, so the extra Heimdall call happens only before a rewind.
Tests:
TestRefreshAuthorizationAfterRotation(consensus/bor): a rotation lands right after a fresh cache refresh. The new span's producer first reads as an unauthorized signer, and afterRefreshAuthorizationit's authorized.TestVerifyStalledTailConfirmsWithFreshSpan(core): confirmed verdict rewinds, a refresh that reveals a rotation does not rewind, and a failed refresh does not rewind.
| p.mu.Unlock() | ||
|
|
||
| switch { | ||
| case recovering: |
There was a problem hiding this comment.
The writeDown return bypasses recoveredExactly. An inexact recovery therefore remains modeRecover, and SealBlock silently broadcasts it without queuing the corrective flush or arming the fallback gate.
Could we check recovery exactness before this return, or defensively in SealBlock, and add a test for an inexact recovery during a write outage?
There was a problem hiding this comment.
Good catch. The writeDown return comes before the exactness check, so an inexact recovery stayed modeRecover and SealBlock took the sealWithoutPublishLocked path. Fixed in e5ac932.
SealBlock now runs the same check (recoveredExactlyLocked) before it reads the mode, so every path into the seal flush is covered, not just the barrier. An inexact recovery during a write outage becomes the over-sealed fallback: its seal flush is queued for when the write path comes back, and the gate is armed with tolerateSealed. TestBarrierPassesRecoveryBuild gains exact and inexact cases with the write path down. (e6ecc4a only moves the flush into publishSealLocked to keep SealBlock within the size limit.)
The seal barrier returns early while the write path is down, before the recovery exactness check, so an inexact recovery stayed in recovery mode and SealBlock sealed it without publishing. SealBlock now applies the same check: an inexact recovery leaves as the over-sealed fallback, with its seal flush queued and its gate armed. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A latest-span cache can be fresh yet predate a span rotation committed right after its refresh, so a valid producer can read as unauthorized. Before rewinding, the stalled-tail check now has the engine refetch the latest span and re-verifies the rejected header, rewinding only if the verdict holds. A failed refresh leaves the chain alone. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@claude review |
Summary
When a producer is cut off from its peers while it can still reach the sequence store, it keeps sealing blocks and publishing them. Consumers serve those blocks' transactions as preconfirmations. Once production rotates to another producer, the new producer rebuilt only the first of those heights from the store. Every later height went to its pool in a new order, so many preconfirmed transactions ended up with different results (index, logs) on the canonical chain. A second problem, in core, could then leave the old producer stuck on its own fork.
This PR has two parts.
1.
eth/sequencer, miner: follow the old producer's sealed run across a takeoverBLOCKHASHresult, which leaves its receipt unchanged.AdoptedWindow.Complete). A dead producer's unsealed last window on an equivalent parent is seeded in the store's order.2.
core, consensus: rewind a stalled chain off blocks its span no longer authorizesconsensus.ErrUnauthorizedSigneris a new sentinel that the bor unauthorized signer and proposer errors match throughIs.Executed tests
Unit
eth/sequencer(full suite and race detector),miner,consensus/borand fullcore: pass.eth/sequencertests: parent-equivalence bridge (sealed and live windows), the seal flush carrying a bridged window, the barrier passing recovery builds, recovery surviving slow store reads, read-breaker suspension, probing and the bounded hold, and the state check:TestHistoryOnlyDiff(8 cases on real state),TestRecoveryBridgeFollowsTheRunsHistory(heights 2 to 4 end to end, including a transaction that stored a block hash) andTestStoreLineage. Each guards a specific fix and fails without it.eth/sequencer:TestReelectedProducerRecoversItsOwnRun(our run left the chain, was rewound away, races our own seal, or is extended) and the frozen-tail-above-head cases inreadbreaker_stale_test.go.minertests:TestCompleteWindowSkipsPoolFill,TestMutedBuildAuthorizedMidBuildRebuilds,TestSignerAuthorizedOutsideBor.coretests:TestStalledForkRecoversWithOneRetainedStatereproduces the stuck node end to end with path scheme,TriesInMemory = 1and a freezer, and recovers; it fails without the fix.TestVerifyStalledTailcovers 12 cases.TestRewindRechecksStaleVerdictimports a valid chain, finalizes it, or whitelists a milestone in the middle of verification, on both rewind paths; without the recheck all six stale cases rewind to block 5.minerprefetcher goroutine and heap threshold tests (TestPrefetchMultiBlock,TestPrefetchGoroutineLifecycle,TestRapidBlockProduction_WithoutWait) are flaky under load. They fail the same way ondevelop, and pass in isolation.Devnet (kurtosis: 3 producers, 1 witness-synced validator, 1 RPC consumer, sequence store). The in-turn producer was cut off from all other Bor nodes for 30 s, keeping store and Heimdall access, under two loads: 20 tx/s through the consumer and 10 tx/s sent straight to the cut-off producer. Each first-served receipt was compared with the canonical one (block number, index, status, gas, logs).
The matrix variants were short/long partitions, a store pause, a frozen gateway, a competing producer, the consumer on the old fork, killing the new producer, killing the consumer, and a contract reading
BLOCKHASH.Devnet fault matrix on this branch (same topology and load). On
5689ff86c, the partition-only and single-fault variants were clean apart from the documented store-outage andBLOCKHASHlimits. Killing the new producer and keeping it dead found the two takeover shapes fixed by the last three commits. Reruns with them:sealed run left the chainfired in each)The seal barrier fix didn't fire on a devnet run. Runs where the span rotated away from the cut-off producer still mismatch; see Rollout notes.
pos-workflows sequencer suites (CI config: kurtosis-pos
v1.4.3, heimdall-v2b1f5a008, store771c465; fresh devnet per image on a 16-core host)932ffa7a2)5689ff86ckurtosis_sequencer_test.shsequencer_rpc_test.shsequencer_chaos_test.shSeed 21403 is an existing issue: after its large-calldata burst (120 × 61 KB entries), one publisher's store tail reads keep running out of time and it flips between live and resyncing.
Gates on the last three commits
go test -race ./miner ./eth/sequencer: pass.golangci-lint(repo config): 0 issues.5689ff86cwith mutation testing: all checks pass, 47/47 mutants killed.Not yet done
Rollout notes
BLOCKHASH, or the EIP-2935 history contract) ends the bridge at the next height. Its own block still follows the store's order.0x7Ee4…66B0) and on the devnet. On a chain with no coinbase configured, fees go to each block's signer, the two producers' blocks differ in balance, and the bridge stops after the first rebuilt height.bor_getInvalidPreconfBlocks(and counted by the consumer audit) may have kept every result, because only the block hash changed.