Call SetFinality when processing a milestone#2226
Conversation
|
@claude review once |
|
Claude finished @pratikspatil024's task in 1m 33s —— View job Review: Call SetFinalized when processing a milestone
Overall: This is a well-motivated, clean fix. The PR description accurately identifies the gap — Bor's milestone finality was never written to CorrectnessLooks good. The change is placed after Thread safety is fine: Persistence across restarts
Suggestions (non-blocking)1. Consider also calling In the Engine API path ( For Bor, milestones represent the strongest finality signal, so the finalized block is by definition also "safe." Adding if header := eth.blockchain.GetHeaderByHash(hash); header != nil {
eth.blockchain.SetFinalized(header)
eth.blockchain.SetSafe(header)
}2. Re-processing on the 1-second ticker
3. No monotonicity check There's no check that the new finalized block number is ≥ the current finalized block number. In practice this should always hold because milestones are monotonically increasing, but a defensive check would make the invariant explicit: if header := eth.blockchain.GetHeaderByHash(hash); header != nil {
if current := eth.blockchain.CurrentFinalBlock(); current == nil || header.Number.Uint64() > current.Number.Uint64() {
eth.blockchain.SetFinalized(header)
}
}Again, non-blocking — the milestone protocol already guarantees monotonicity. No issues found
LGTM with the optional |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, defensive 5-line change that mirrors the existing ProcessMilestone call.
Extended reasoning...
Overview
Single-file, 5-line addition in eth/handler_bor.go. After the existing h.downloader.ProcessMilestone(num, hash) call in handleMilestone, the PR fetches the corresponding header via GetHeaderByHash and, if non-nil, calls eth.blockchain.SetFinalized(header). This brings bor's finality signal into the canonical geth BlockChain API (CurrentFinalBlock()), which was previously always nil on bor since the only upstream writer is the (unused) Engine API.
Security risks
None identified. SetFinalized is a simple setter that stores the header pointer, writes the finalized hash to rawdb, and updates a metrics gauge — no consensus or state-transition impact. GetHeaderByHash is a safe read and returns nil for unknown hashes; the unknown-hash case is handled as a defensive no-op. No new external inputs, auth, or crypto surface area.
Level of scrutiny
Low. The change is purely additive metadata propagation: it makes existing geth machinery (metrics, the SetHead safety guard, tracer-visible finality) reflect a value bor already computes elsewhere (getFinalizedBlockNumber via the downloader whitelist). The two observable effects called out in the description (gauge advancing, SetHead invalidated finalized block log becoming reachable on rewinds below a milestone) are intended and benign.
Other factors
SonarQube quality gate passed. Maintainer (pratikspatil024) explicitly requested review on this PR. The change has been manually tested on mainnet per the PR description. No bugs flagged by the bug hunting system.
write time handleMilestone now resolves the milestone block via GetBlockByNumber and verifies the canonical hash matches milestone.Hash before calling SetFinalized/SetSafe to avoid promoting a stale header if a reorg lands before this write. Calls SetSafe alongside SetFinalized so both advance in real time.
|
|
This PR is stale because it has been open 21 days with no activity. Remove stale label or comment or this will be closed in 14 days. |
|
This is still valid, PR approved was just missing a merge window. |
|
@claude review once |
There was a problem hiding this comment.
LGTM — the change is a small, well-reviewed addition that wires milestone processing into BlockChain.SetFinalized. The three findings are all nits (a stale comment, a non-blocking monotonicity suggestion already raised and accepted as non-blocking in the original review, and a subtle safe==finalized resurfacing on restart via the pre-existing loadLastState path) — none affect correctness or introduce security/consensus risk, and the core placement/nil-checks are correct.
Extended reasoning...
Overview
The diff adds a single call, eth.blockchain.SetFinalized(block.Header()), in handleMilestone (eth/handler_bor.go), right after ProcessMilestone succeeds. It re-resolves the milestone's end block by number and checks the hash matches before publishing, guarding against a reorg between verification and this write. This wires Heimdall milestone finality into the standard geth BlockChain finality bookkeeping (atomic pointer + headFinalizedBlockGauge + rawdb.WriteFinalizedBlockHash), which was previously always nil/zero on Bor since only the (unused) Engine API called SetFinalized upstream.
Security risks
No new external input surface is introduced — the milestone data was already verified by verifier.verify before this call executes, and the block is re-resolved defensively by canonical number->hash mapping rather than trusting a stale reference. The only consumers of CurrentFinalBlock() are metrics, live tracers, and downstream tooling — Bor's actual reorg-safety mechanism (the whitelist finality.IsValidChain) is untouched. The RPC eth_getBlockByNumber("finalized") path is also unchanged (still goes through the downloader whitelist). I confirmed the three inline findings are all real relative to the current code (checked SetFinalized/SetSafe/loadLastState in core/blockchain.go), but all are Nit-severity: none allow an external actor to corrupt state, bypass validation, or crash the node.
Level of scrutiny
This is a narrowly-scoped, single-function change (7 added lines) touching a non-consensus-critical bookkeeping path (finality metrics/API surface, not block validation or whitelist logic). It underwent a full review cycle with two maintainers (kamuikatsurgi, maoueh) explicitly debating and resolving the SetSafe question, and a maintainer reconfirmed on 2026-07-13 that it remains valid and was previously approved pending a merge window. Given the limited blast radius and the depth of prior human review, a light-touch confirmation is appropriate here rather than blocking again on nits that don't change behavior in any harmful way.
Other factors
All three inline findings are explicitly non-blocking suggestions (documentation staleness, a defensive monotonicity check already proposed and consciously deferred in the original review, and a restart-time edge case in pre-existing upstream loadLastState logic that this PR merely activates for the first time on Bor). None require a design decision beyond what's already been discussed in the thread.
| // 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()) | ||
| } |
There was a problem hiding this comment.
🟡 This PR's first non-nil SetFinalized call on Bor writes the finalized hash to the database via rawdb.WriteFinalizedBlockHash. On the very next node restart, BlockChain.loadLastState reads that persisted hash and sets currentSafeBlock equal to currentFinalBlock, reintroducing exactly the safe == finalized state that was explicitly removed from this PR after review discussion. This makes eth_getBlockByNumber("safe") return nil at 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 via rawdb.WriteFinalizedBlockHash whenever 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 both bc.currentFinalBlock and bc.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, SetFinalized was 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 the rawdb finalized-hash key was never written on Bor, and the loadLastState safe-restore branch quoted above was permanently dead code for this chain. This PR adds the first Bor-side call to SetFinalized (in handleMilestone, 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) alongside SetFinalized. During review, kamuikatsurgi asked to remove it: 'Remove .SetSafe since we don't have soft finality in Bor.' When maoueh pushed back that safe == finalized might 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 calls SetFinalized. But this only prevents safe == finalized at runtime between restarts — it does nothing to prevent loadLastState from reintroducing it on the next restart, because that path is keyed off the same persisted rawdb value this PR now writes for the first time.
Concrete walkthrough:
- Node starts with this PR deployed.
currentSafeBlockis nil, matching the reviewed intent (eth_getBlockByNumber("safe")returns 'safe block not found'). - A milestone is processed successfully;
handleMilestonecallseth.blockchain.SetFinalized(block.Header()). This stores the header incurrentFinalBlockand writes its hash to disk viarawdb.WriteFinalizedBlockHash. - Node operator restarts the node (routine maintenance, upgrade, crash recovery — nothing pathological).
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: loadLastState is shared upstream geth logic; this PR only touches eth/handler_bor.go and never touches loadLastState or clears currentSafeBlock after startup. There is no Bor-specific override for the RPC 'safe' tag analogous to the one that exists for 'finalized' in getFinalizedBlockNumber, so whatever CurrentSafeBlock() holds is returned as-is.
Impact: This is a semantic/observability inconsistency, not a crash or fund-loss bug — the resulting safe value 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 (safe nil) diverges from post-restart behavior (safe == finalized) with no code change in between.
Suggested fix: Either explicitly clear currentSafeBlock after loadLastState runs on Bor (or add a Bor-specific override for the 'safe' RPC tag, mirroring how 'finalized' is already special-cased in getFinalizedBlockNumber), or accept safe == finalized consistently and document it — the current merged state produces neither of those outcomes consistently.
| // 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()) | ||
| } |
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 header.Number.Uint64() > current.Number.Uint64() guard would close the gap.
Extended reasoning...
The gap: handleMilestone calls eth.blockchain.SetFinalized(block.Header()) (eth/handler_bor.go:131-136) as soon as the milestone's end block resolves to a canonical block via GetBlockByNumber/hash-match. Neither of the two gates upstream of this call enforce monotonicity:
verifier.verify(eth/bor_checkpoint_verifier.go:69-96) only checks that the local chain head is not behind the milestone's end block (errChainOutOfSync) and that the end-block hash matches canonical history. An older milestone whose end block is still part of current canonical history satisfies both checks just fine.whitelist finality.Process(eth/downloader/whitelist/finality.go:69-80) unconditionally overwritesNumber/Hash, so it doesn't provide a monotonicity backstop either.
Trigger path: Bor supports MultiHeimdallClient (consensus/bor/heimdall/failover_client.go), which cascades FetchMilestone across multiple independently-polled heimdall endpoints, including a fallback pass over 'unhealthy' clients. If the active/selected endpoint is (or becomes) a lagging replica, FetchMilestone can return a milestone whose EndBlock is lower than one already finalized locally. handleMilestone re-resolves that lower EndBlock via GetBlockByNumber, finds it's still canonical (it is — it's just older), and calls SetFinalized with a header whose number is less than the current CurrentFinalBlock(). The same can happen via the WS subscription client (consensus/bor/heimdallws/client.go), which also fails over between multiple heimdall URLs and can redeliver a stale milestone on reconnect.
Step-by-step proof:
- Node is configured with two heimdall endpoints, A (up to date) and B (lagging by several milestones), via MultiHeimdallClient.
- A milestone with EndBlock=1000 is fetched from A, passes
verifier.verify, andSetFinalizedsetsCurrentFinalBlock()to block 1000. - Endpoint A has a transient error (or health-registry rotation) and the failover client cascades to B.
- B's latest known milestone is EndBlock=900 (block 900 is still canonical — no reorg has happened).
verifier.verify(ctx, eth, h, 800, 900, hash900)passes: chain head (>1000) is not behind 900, and block 900's hash matches canonical history.handleMilestonecallseth.blockchain.SetFinalized(block900.Header()), andcore/blockchain.go'sSetFinalizedunconditionally stores it and persists it viarawdb.WriteFinalizedBlockHash, movingheadFinalizedBlockGaugefrom 1000 back to 900.- Any consumer reading
CurrentFinalBlock()directly (live tracers, external indexers) between steps 6 and the next healthy poll observes finality moving backward, violating the implicit invariant that finalized blocks are monotonically non-decreasing.
Why this doesn't block merge: SetFinalized here only feeds the standard geth BlockChain finality bookkeeping (atomic pointer + metric + rawdb hash) — it is not consulted by Bor's actual reorg-safety mechanism, which is the separate whitelist (finality.IsValidChain/IsValidPeer) and remains untouched by this PR. The RPC eth_getBlockByNumber("finalized") path (getFinalizedBlockNumber) also doesn't read CurrentFinalBlock() — it goes through the downloader's whitelisted milestone, so it's unaffected. The regression is also self-correcting: the very next successful poll of a caught-up endpoint (milestones are polled roughly every ~1s) moves CurrentFinalBlock() forward again. And upstream geth's own SetFinalized (used from the Engine API) has no monotonicity guard either, so this isn't a new invariant that Bor is uniquely breaking.
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.
| // 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()) | ||
| } |
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 eth/handler_bor.go:131 reads:
// 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 eth.blockchain.SetFinalized(block.Header()) — there is no SetSafe call anywhere in this block.
How this happened: Looking at the PR timeline, the original version of this diff called both SetFinalized and SetSafe, matching the wording "finalized and safe." Reviewer @kamuikatsurgi asked to remove SetSafe ("Remove .SetSafe since we don'''t have soft finality in Bor"), and after some discussion with @maoueh, the removal was confirmed as the right call: "while safe == finalised, we don'''t want teams who are not aware of certain mechanics of the chain take wrong decisions based on safe == finalised." That removal landed in commit bc0fd91 ("remove SetSafe as was suggested by @kamuikatsurgi"), which is visible in the recent git log. The code was correctly updated, but the comment text was never revisited and still references the old, now-removed behavior.
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 safe == finalized semantics that Bor deliberately does not provide). A stale comment that implies functionality which doesn'''t exist can lead to wrong assumptions downstream, e.g. someone assuming headSafeBlockGauge advances here when it doesn'''t.
Step-by-step proof:
- Read the comment at line 131-133: it claims the block is published "as finalized and safe."
- Read the code at lines 134-136: only
eth.blockchain.SetFinalized(block.Header())executes; grep the function body forSetSafe— zero matches. - Check git history:
git log --oneline -5showsbc0fd91 remove SetSafe as was suggested by @kamuikatsurgias the most recent commit, confirmingSetSafewas removed intentionally and recently, but the comment wasn'''t updated in the same commit. - Conclusion: the comment is factually stale relative to the code it documents.
Fix: Simply update the comment to remove "and safe", e.g.: // Publish the milestone block as finalized. 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.
This is purely a documentation/comment mismatch — no runtime, correctness, or security impact, and does not block merge.
5 participants
Description
bc.CurrentFinalBlock()is currently alwaysnilon bor nodes, because the only writers ofblockchain.SetFinalizedin upstream geth are the Engine API (eth/catalyst), which bor does not use. As a result:headFinalizedBlockGauge/headSafeBlockGaugemetrics never advance.BlockChainAPI to read finality — live tracers (core.BlockchainLogger), external indexers, downstream tooling — sees no finality on bor, even though Heimdall milestones are by definition the chain's strongest finality signal.Bor already knows what's finalized:
eth/api.go:getFinalizedBlockNumberderives it fromDownloader().GetWhitelistedMilestone()to answer theeth_getBlockByNumber("finalized")RPC. This PR closes the gap by also writing that information into the canonical place (bc.SetFinalized) the moment a milestone has been successfully processed inethHandler.handleMilestone, right after the existingProcessMilestonecall. The header is fetched defensively viaGetHeaderByHash; an unknown hash is a no-op.This PR only writes to
CurrentFinalBlock()from milestones, because milestones are the strongest/live signal. The RPCfinalized-tag path ingetFinalizedBlockNumber(milestone → checkpoint fallback) is unchanged.Changes
Breaking changes
None at the protocol or RPC level. Two observable effects:
headFinalizedBlockGaugeandheadSafeBlockGaugewill now advance on bor nodes (they were stuck at 0). Dashboards may need to acknowledge this.SetHead invalidated finalized blocklog path incore/blockchain.gobecomes reachable. By construction it only triggers on a rewind below a confirmed Heimdall milestone, which is already a pathological situation — making it visible is the desired behavior.he JSON-RPC
eth_getBlockByNumber("finalized")response is unchanged: it still flows throughgetFinalizedBlockNumberand the downloader whitelist.Testing
eth_getBlockByNumber("finalized")continues to return the latest milestoned block (unchanged).headFinalizedBlockGaugeadvances after the first milestone is processed (new behavior).BlockChain.CurrentFinalBlock()returns the milestone header (new behavior, previouslynil).