Call SetFinality when processing a milestone#2226
Open
sduchesneau wants to merge 3 commits into
Open
Claude / Claude Code Review
completed
Jul 14, 2026 in 16m 21s
Code review found 3 potential issues
Found 5 candidates, confirmed 3. See review comments for details.
Details
| Severity | Count |
|---|---|
| 🔴 Important | 0 |
| 🟡 Nit | 3 |
| 🟣 Pre-existing | 0 |
| Severity | File:Line | Issue |
|---|---|---|
| 🟡 Nit | eth/handler_bor.go:131-136 |
loadLastState resurrects safe==finalized on restart after SetSafe removal |
| 🟡 Nit | eth/handler_bor.go:131-136 |
SetFinalized has no monotonicity guard |
| 🟡 Nit | eth/handler_bor.go:131-136 |
Stale comment references removed SetSafe call |
Annotations
Check warning on line 136 in eth/handler_bor.go
claude / Claude Code Review
loadLastState resurrects safe==finalized on restart after SetSafe removal
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 follow
Check warning on line 136 in eth/handler_bor.go
claude / Claude Code Review
SetFinalized has no monotonicity guard
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
Check warning on line 136 in eth/handler_bor.go
claude / Claude Code Review
Stale comment references removed SetSafe call
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.
Loading