Call SetFinality when processing a milestone #2226
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -128,6 +128,13 @@ | |
|
|
||
| h.downloader.ProcessMilestone(num, hash) | ||
|
|
||
| // Publish the milestone block as finalized and safe. Re-resolve via the | ||
| // canonical number→hash mapping so a reorg between verification and this | ||
| // write can't promote a stale or non-canonical header. | ||
| if block := eth.blockchain.GetBlockByNumber(num); block != nil && block.Hash() == hash { | ||
| eth.blockchain.SetFinalized(block.Header()) | ||
| } | ||
|
Check warning on line 136 in eth/handler_bor.go
|
||
|
Comment on lines
+131
to
+136
There was a problem hiding this comment. 🟡 SetFinalized(block.Header()) is called unconditionally whenever a milestone's end block resolves to a canonical block, with no check that the new finalized number is >= the currently stored CurrentFinalBlock(). Because MultiHeimdallClient (and the WS failover client) can fail over to a lagging heimdall endpoint, a stale-but-still-canonical milestone could regress currentFinalBlock, the persisted finalized hash, and headFinalizedBlockGauge. This was already flagged as a non-blocking suggestion in an earlier review pass (suggestion #3); a simple Extended reasoning...The gap:
Trigger path: Bor supports Step-by-step proof:
Why this doesn't block merge: Suggested fix: guard the call with a simple monotonicity check, as already suggested non-blockingly in the original review thread: if block := eth.blockchain.GetBlockByNumber(num); block != nil && block.Hash() == hash {
if current := eth.blockchain.CurrentFinalBlock(); current == nil || block.NumberU64() > current.Number.Uint64() {
eth.blockchain.SetFinalized(block.Header())
}
}This is a one-line robustness improvement, not a correctness-blocking fix, since nothing durably breaks if it's omitted.
Comment on lines
+131
to
+136
There was a problem hiding this comment. 🟡 The block comment above the SetFinalized call still says the milestone block is published as "finalized and safe", but the SetSafe call was removed per reviewer request (Bor has no soft finality), leaving only SetFinalized. Please drop "and safe" from the comment so it matches the code. Extended reasoning...What the bug is: The comment at // Publish the milestone block as finalized and safe. Re-resolve via the
// canonical number→hash mapping so a reorg between verification and this
// write can'\''t promote a stale or non-canonical header.but the code beneath it only calls How this happened: Looking at the PR timeline, the original version of this diff called both Why this matters: A future reader of this function — without access to the PR discussion — will read "Publish the milestone block as finalized and safe" and reasonably conclude that safe-block tracking happens here. That'''s exactly the kind of incorrect assumption @kamuikatsurgi'''s review comment was trying to prevent (teams making decisions based on Step-by-step proof:
Fix: Simply update the comment to remove "and safe", e.g.: This is purely a documentation/comment mismatch — no runtime, correctness, or security impact, and does not block merge. |
||
|
|
||
| return nil | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 This PR's first non-nil
SetFinalizedcall on Bor writes the finalized hash to the database viarawdb.WriteFinalizedBlockHash. On the very next node restart,BlockChain.loadLastStatereads that persisted hash and setscurrentSafeBlockequal tocurrentFinalBlock, reintroducing exactly thesafe == finalizedstate that was explicitly removed from this PR after review discussion. This makeseth_getBlockByNumber("safe")returnnilat runtime but a non-nil block after any restart following a processed milestone — an inconsistency worth documenting or explicitly clearing.Extended reasoning...
The bug:
SetFinalized(core/blockchain.go:1128-1139) unconditionally persists the finalized block hash viarawdb.WriteFinalizedBlockHashwhenever called with a non-nil header. Separately,BlockChain.loadLastState(core/blockchain.go:989-999) reads that same persisted hash on every node startup and, if present, stores the corresponding block into bothbc.currentFinalBlockandbc.currentSafeBlock— the inline comment there literally says 'the safe block is not stored on disk and it is set to the last known finalized block on startup.'Why this PR activates a dormant path: Before this PR,
SetFinalizedwas never called anywhere in Bor's production code path — the only other caller is the Engine API (eth/catalyst/api.go), which Bor does not use. So therawdbfinalized-hash key was never written on Bor, and theloadLastStatesafe-restore branch quoted above was permanently dead code for this chain. This PR adds the first Bor-side call toSetFinalized(inhandleMilestone,eth/handler_bor.go:131-136), which means the first successfully processed milestone now writes that key for the first time in Bor's history.Why this defeats the PR's own explicit decision: The PR originally also called
SetSafe(header)alongsideSetFinalized. During review, kamuikatsurgi asked to remove it: 'Remove .SetSafe since we don't have soft finality in Bor.' When maoueh pushed back thatsafe == finalizedmight be a reasonable outcome, kamuikatsurgi reaffirmed the removal: 'we don't want teams who are not aware of certain mechanics of the chain take wrong decisions based on safe == finalised.' The author complied, and the merged diff only callsSetFinalized. But this only preventssafe == finalizedat runtime between restarts — it does nothing to preventloadLastStatefrom reintroducing it on the next restart, because that path is keyed off the same persistedrawdbvalue this PR now writes for the first time.Concrete walkthrough:
currentSafeBlockis nil, matching the reviewed intent (eth_getBlockByNumber("safe")returns 'safe block not found').handleMilestonecallseth.blockchain.SetFinalized(block.Header()). This stores the header incurrentFinalBlockand writes its hash to disk viarawdb.WriteFinalizedBlockHash.loadLastStateruns, reads the now-populatedReadFinalizedBlockHash, resolves the block, and stores it into bothcurrentFinalBlockandcurrentSafeBlock.eth_getBlockByNumber("safe")— resolved directly fromb.eth.blockchain.CurrentSafeBlock()ineth/api_backend.go:118-124/186-193andinternal/ethapi/bor_api.go:462-464, with no Bor-specific override the way the 'finalized' tag has — now returns the milestone block instead of nil.headSafeBlockGaugeadvances too.Why nothing in the current code prevents this:
loadLastStateis shared upstream geth logic; this PR only toucheseth/handler_bor.goand never touchesloadLastStateor clearscurrentSafeBlockafter startup. There is no Bor-specific override for the RPC 'safe' tag analogous to the one that exists for 'finalized' ingetFinalizedBlockNumber, so whateverCurrentSafeBlock()holds is returned as-is.Impact: This is a semantic/observability inconsistency, not a crash or fund-loss bug — the resulting
safevalue is conservative (safe == finalized ≤ head) and no state corruption occurs. But it silently reintroduces the exact behavior (safe == finalized) that two maintainers explicitly debated and rejected in this PR's own review thread, and it does so nondeterministically depending on whether the node has been restarted since the feature shipped — runtime behavior (safenil) diverges from post-restart behavior (safe == finalized) with no code change in between.Suggested fix: Either explicitly clear
currentSafeBlockafterloadLastStateruns on Bor (or add a Bor-specific override for the 'safe' RPC tag, mirroring how 'finalized' is already special-cased ingetFinalizedBlockNumber), or acceptsafe == finalizedconsistently and document it — the current merged state produces neither of those outcomes consistently.