Skip to content

Call SetFinality when processing a milestone #2226

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
sduchesneau wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Call SetFinality when processing a milestone #2226

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
61dec30
call SetFinalized when successfully processing a milestone
sduchesneau May 14, 2026
05c624e
eth: also publish milestone block as safe and re-check canonical at
sduchesneau May 21, 2026
bc0fd91
remove SetSafe as was suggested by @kamuikatsurgi
sduchesneau May 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions eth/handler_bor.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,14 @@ func (h *ethHandler) handleMilestone(ctx context.Context, eth *Ethereum, milesto

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())
eth.blockchain.SetSafe(block.Header())
Comment thread
kamuikatsurgi marked this conversation as resolved.
Outdated
}
Comment on lines +131 to +136

@claude claude Bot Jul 14, 2026

Copy link
Copy Markdown

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 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:

  1. Node starts with this PR deployed. currentSafeBlock is nil, matching the reviewed intent (eth_getBlockByNumber("safe") returns 'safe block not found').
  2. A milestone is processed successfully; handleMilestone calls eth.blockchain.SetFinalized(block.Header()). This stores the header in currentFinalBlock and writes its hash to disk via rawdb.WriteFinalizedBlockHash.
  3. Node operator restarts the node (routine maintenance, upgrade, crash recovery — nothing pathological).
  4. loadLastState runs, reads the now-populated ReadFinalizedBlockHash, resolves the block, and stores it into both currentFinalBlock and currentSafeBlock.
  5. eth_getBlockByNumber("safe") — resolved directly from b.eth.blockchain.CurrentSafeBlock() in eth/api_backend.go:118-124/186-193 and internal/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. headSafeBlockGauge advances 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.

Comment on lines +131 to +136

@claude claude Bot Jul 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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 overwrites Number/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:

  1. Node is configured with two heimdall endpoints, A (up to date) and B (lagging by several milestones), via MultiHeimdallClient.
  2. A milestone with EndBlock=1000 is fetched from A, passes verifier.verify, and SetFinalized sets CurrentFinalBlock() to block 1000.
  3. Endpoint A has a transient error (or health-registry rotation) and the failover client cascades to B.
  4. B's latest known milestone is EndBlock=900 (block 900 is still canonical — no reorg has happened).
  5. verifier.verify(ctx, eth, h, 800, 900, hash900) passes: chain head (>1000) is not behind 900, and block 900's hash matches canonical history.
  6. handleMilestone calls eth.blockchain.SetFinalized(block900.Header()), and core/blockchain.go's SetFinalized unconditionally stores it and persists it via rawdb.WriteFinalizedBlockHash, moving headFinalizedBlockGauge from 1000 back to 900.
  7. 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.

Comment on lines +131 to +136

@claude claude Bot Jul 14, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 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:

  1. Read the comment at line 131-133: it claims the block is published "as finalized and safe."
  2. Read the code at lines 134-136: only eth.blockchain.SetFinalized(block.Header()) executes; grep the function body for SetSafe — zero matches.
  3. Check git history: git log --oneline -5 shows bc0fd91 remove SetSafe as was suggested by @kamuikatsurgi as the most recent commit, confirming SetSafe was removed intentionally and recently, but the comment wasn'''t updated in the same commit.
  4. 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.


return nil
}

Expand Down