Call SetFinality when processing a milestone

[sduchesneau]

Description

bc.CurrentFinalBlock() is currently always nil on bor nodes, because the only writers of blockchain.SetFinalized in upstream geth are the Engine API (eth/catalyst), which bor does not use. As a result:

• The headFinalizedBlockGauge / headSafeBlockGauge metrics never advance.
• The SetHead safety guard at core/blockchain.go that refuses to rewind past a finalized block is dead code on bor — it's gated on bc.CurrentFinalBlock() != nil.
• Any consumer that uses the standard geth BlockChain API 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:getFinalizedBlockNumber derives it from Downloader().GetWhitelistedMilestone() to answer the eth_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 in ethHandler.handleMilestone, right after the existing ProcessMilestone call. The header is fetched defensively via GetHeaderByHash; an unknown hash is a no-op.

Changes

• Bugfix (non-breaking change that solves an issue)
• Hotfix
• New feature
• Breaking change
• Changes only for a subset of nodes

Breaking changes

None at the protocol or RPC level. Two observable effects:

1. headFinalizedBlockGauge and headSafeBlockGauge will now advance on bor nodes (they were stuck at 0). Dashboards may need to acknowledge this.
2. The SetHead invalidated finalized block log path in core/blockchain.go becomes 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 through getFinalizedBlockNumber and the downloader whitelist.

Testing

• Unit tests
• Manual on mainnet:
  • eth_getBlockByNumber("finalized") continues to return the latest milestoned block (unchanged).
  • headFinalizedBlockGauge advances after the first milestone is processed (new behavior).
  • Live tracer / BlockChain.CurrentFinalBlock() returns the milestone header (new behavior, previously nil).

[claude]

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[pratikspatil024]

@claude review once

[claude]

Claude finished @pratikspatil024's task in 1m 33s —— View job

---

Review: Call SetFinalized when processing a milestone

• Gather context and read changed files
• Review the diff against origin/develop
• Analyze correctness, security, and edge cases
• Post review findings

---

Overall: This is a well-motivated, clean fix. The PR description accurately identifies the gap — Bor's milestone finality was never written to BlockChain.SetFinalized, leaving CurrentFinalBlock() permanently nil. The placement is correct: right after ProcessMilestone succeeds, with a defensive nil check on the header lookup.

Correctness

Looks good. The change is placed after ProcessMilestone(num, hash) at eth/handler_bor.go:129, which only runs when verifier.verify has succeeded (errors return early at lines 97–117). The GetHeaderByHash nil check at line 132 is the right defensive pattern — if the block has been pruned or is somehow missing, it's a no-op rather than a panic.

Thread safety is fine: SetFinalized uses atomic.Pointer[types.Header] for the in-memory store (core/blockchain.go:390, :1130), and rawdb.WriteFinalizedBlockHash is a single DB write.

Persistence across restarts

SetFinalized calls rawdb.WriteFinalizedBlockHash (core/blockchain.go:1133), and loadLastState reads it back on startup (core/blockchain.go:992-998). So the finalized marker will survive node restarts — good.

Suggestions (non-blocking)

1. Consider also calling SetSafe

In the Engine API path (eth/catalyst/api.go:403-417), both SetFinalized and SetSafe are set. On startup, loadLastState also sets both from the finalized hash (core/blockchain.go:994-997). However, this PR only calls SetFinalized, leaving currentSafeBlock as nil until the next restart (when loadLastState will copy finalized → safe).

For Bor, milestones represent the strongest finality signal, so the finalized block is by definition also "safe." Adding eth.blockchain.SetSafe(header) alongside SetFinalized would keep headSafeBlockGauge advancing in real-time too, and match the behavior consumers expect (safe ≤ finalized).

if header := eth.blockchain.GetHeaderByHash(hash); header != nil {
    eth.blockchain.SetFinalized(header)
    eth.blockchain.SetSafe(header)
}

2. Re-processing on the 1-second ticker

handleMilestone is called every 1 second via the ticker at eth/backend.go:893-901 to re-process the same milestone. With this PR, that means SetFinalized + WriteFinalizedBlockHash (a DB write) fires every second for the same block. This is functionally harmless (idempotent write of the same value), but worth noting. If you want to avoid the redundant DB writes, a simple guard like checking bc.CurrentFinalBlock() before writing would do — but this is a micro-optimization, not a correctness concern.

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

• No security concerns (no new external input, no panics in consensus path)
• No concurrency issues (atomic store, single DB write)
• Defensive nil check is present
• Correctly placed after verification succeeds

LGTM with the optional SetSafe addition as a nice-to-have.

---

[claude]

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.