feature: block tag support(safe&finalize)#277
Conversation
tomatoishealthy
requested review from
panos-xyz
and removed request for
a team
📝 WalkthroughWalkthroughThe pull request adds safe and finalized block tracking to the blockchain by introducing atomic pointer fields, setter/getter methods, database persistence for finalized block state recovery on startup, and updates multiple APIs (EthAPI, L2 consensus, filters, gas price oracle, engine client) to resolve block numbers using these new block states. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Engine as Engine API
participant L2API as L2 Consensus API
participant BC as BlockChain
participant DB as Database
Client->>Engine: SetBlockTags(safeHash, finalizedHash)
Engine->>L2API: SetBlockTags(safeHash, finalizedHash)
L2API->>BC: HeaderByHash(safeHash)
BC-->>L2API: safeHeader
L2API->>BC: HeaderByHash(finalizedHash)
BC-->>L2API: finalizedHeader
L2API->>BC: SetSafe(safeHeader)
BC->>BC: Store in currentSafeBlock
L2API->>BC: SetFinalized(finalizedHeader)
BC->>BC: Store in currentFinalizedBlock
BC->>DB: WriteFinalizedBlockHash(hash)
DB-->>BC: ✓
L2API-->>Engine: nil/error
Engine-->>Client: error response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@core/blockchain_l2.go`:
- Around line 263-268: SetFinalized may panic or write an invalid hash when
header is nil; add a nil check at the start of BlockChain.SetFinalized (the
method) and return early if header == nil (or log and return) so you do not call
currentFinalizedBlock.Store(nil) nor rawdb.WriteFinalizedBlockHash with
header.Hash(), and avoid accessing header.Number in log.Debug; only perform
Store, rawdb.WriteFinalizedBlockHash and log.Debug when header is non-nil.
- Around line 254-257: The SetSafe method can panic if header is nil because it
dereferences header.Number and header.Hash(); add a nil guard at the start of
BlockChain.SetSafe(header *types.Header) that returns early (or logs a warning)
when header == nil, and only call bc.currentSafeBlock.Store(header) and
log.Debug(...) when header is non-nil (use the function name SetSafe and
variable header to locate the change).
In `@core/blockchain.go`:
- Around line 502-504: When recovering from a chain rewind in
setHeadBeyondRoot/loadLastState, ensure any finalized or safe block references
are validated and cleared if their headers are missing: after calling
GetHeaderByHash() in loadLastState (or the rewind recovery path), check for a
nil return and if nil set currentFinalizedBlock and/or currentSafeBlock to nil
(or otherwise invalidate them) and update any dependent state; also add the same
validation in setHeadBeyondRoot where headers may be deleted so you don't retain
stale pointers to removed headers.
In `@eth/api.go`:
- Around line 179-197: When header is nil, the error currently prints the raw
blockNr (which can be negative sentinel values); change the nil-check/error path
around header (after switch that calls
CurrentSafeBlock()/CurrentFinalizedBlock()/CurrentBlock()) to map those sentinel
constants (rpc.LatestBlockNumber, rpc.SafeBlockNumber, rpc.FinalizedBlockNumber)
to human-readable names and return errors like "latest block not found", "safe
block not found", or "finalized block not found" instead of "block `#-4` not
found"; keep the existing behavior for numeric block numbers (the default case
using GetBlockByNumber) and use the blockNr variable only when it represents an
actual numeric block index.
In `@eth/gasprice/feehistory.go`:
- Around line 171-194: The switch handling reqEnd (in the fee history resolution
block) can leave resolved and err nil for unsupported block tags; add a default
case to the switch that sets err to a descriptive error (e.g., fmt.Errorf or
errors.New) indicating an unsupported/unknown block tag and ensure the function
returns that error instead of nil, so callers can distinguish invalid reqEnd
values; update any references to resolved/err after the switch (same function)
to rely on this explicit error path.
🧹 Nitpick comments (6)
core/blockchain.go (1)
484-491: Duplicate logging detected.The finalized block is logged at line 489 and again at lines 502-504. Consider removing one of these log statements to avoid redundant output during startup.
🔧 Suggested fix
// Restore the last known finalized block, and set safe = finalized on startup if head := rawdb.ReadFinalizedBlockHash(bc.db); head != (common.Hash{}) { if header := bc.GetHeaderByHash(head); header != nil { bc.currentFinalizedBlock.Store(header) bc.currentSafeBlock.Store(header) // safe = finalized on startup - log.Info("Loaded finalized block", "number", header.Number, "hash", header.Hash()) } }eth/catalyst/l2_api.go (2)
367-391: Consider atomicity of safe/finalized block updates.If
safeBlockHashlookup fails afterfinalizedBlockHashhas been successfully set, the chain will have an inconsistent state where finalized is updated but safe is not. This could be problematic since the invariantfinalized <= safe <= headmay be violated.Consider validating both headers exist before applying any changes:
♻️ Suggested refactor for atomic updates
func (api *l2ConsensusAPI) SetBlockTags(safeBlockHash common.Hash, finalizedBlockHash common.Hash) error { bc := api.eth.BlockChain() + // Validate both headers exist before making any changes + var finalizedHeader, safeHeader *types.Header + + if finalizedBlockHash != (common.Hash{}) { + finalizedHeader = bc.GetHeaderByHash(finalizedBlockHash) + if finalizedHeader == nil { + return fmt.Errorf("finalized block %s not found", finalizedBlockHash.Hex()) + } + } + + if safeBlockHash != (common.Hash{}) { + safeHeader = bc.GetHeaderByHash(safeBlockHash) + if safeHeader == nil { + return fmt.Errorf("safe block %s not found", safeBlockHash.Hex()) + } + } + + // Apply changes after validation + if finalizedHeader != nil { + bc.SetFinalized(finalizedHeader) + log.Info("Set finalized block", "number", finalizedHeader.Number, "hash", finalizedBlockHash) + } + + if safeHeader != nil { + bc.SetSafe(safeHeader) + log.Info("Set safe block", "number", safeHeader.Number, "hash", safeBlockHash) + } + - // Set finalized block - if finalizedBlockHash != (common.Hash{}) { - finalizedHeader := bc.GetHeaderByHash(finalizedBlockHash) - if finalizedHeader == nil { - return fmt.Errorf("finalized block %s not found", finalizedBlockHash.Hex()) - } - bc.SetFinalized(finalizedHeader) - log.Info("Set finalized block", "number", finalizedHeader.Number, "hash", finalizedBlockHash) - } - - // Set safe block - if safeBlockHash != (common.Hash{}) { - safeHeader := bc.GetHeaderByHash(safeBlockHash) - if safeHeader == nil { - return fmt.Errorf("safe block %s not found", safeBlockHash.Hex()) - } - bc.SetSafe(safeHeader) - log.Info("Set safe block", "number", safeHeader.Number, "hash", safeBlockHash) - } - return nil }
377-377: Duplicate logging withSetFinalized.The
log.Infohere duplicates thelog.Debuginbc.SetFinalized(). Consider keeping only one log statement, preferably atInfolevel in this API method since it's the external entry point.eth/api_backend.go (1)
128-143: Potential nil return without error for safe/finalized blocks.If the header exists but the block body is missing (edge case),
GetBlockreturnsnilwithout an error. This could cause confusion for API consumers who receive a nil block with no error.Consider adding a nil check similar to
BlockByNumberOrHash:Proposed fix
// Safe block if number == rpc.SafeBlockNumber { header := b.eth.blockchain.CurrentSafeBlock() if header == nil { return nil, errors.New("safe block not available") } - return b.eth.blockchain.GetBlock(header.Hash(), header.Number.Uint64()), nil + block := b.eth.blockchain.GetBlock(header.Hash(), header.Number.Uint64()) + if block == nil { + return nil, errors.New("safe block body not found") + } + return block, nil } // Finalized block if number == rpc.FinalizedBlockNumber { header := b.eth.blockchain.CurrentFinalizedBlock() if header == nil { return nil, errors.New("finalized block not available") } - return b.eth.blockchain.GetBlock(header.Hash(), header.Number.Uint64()), nil + block := b.eth.blockchain.GetBlock(header.Hash(), header.Number.Uint64()) + if block == nil { + return nil, errors.New("finalized block body not found") + } + return block, nil }eth/filters/filter.go (2)
125-152: Errors fromHeaderByNumberare silently discarded.The
resolveSpecialhelper ignores errors returned byHeaderByNumber(Lines 129, 135, 141). While the nil header check catches most failure cases, discarding the actual error loses potentially useful diagnostic information.Proposed fix to propagate errors
resolveSpecial := func(number int64) (uint64, error) { switch number { case rpc.PendingBlockNumber.Int64(), // inheriting the current logic, pending logs will be disabled in the future rpc.LatestBlockNumber.Int64(): - hdr, _ := f.sys.backend.HeaderByNumber(ctx, rpc.LatestBlockNumber) + hdr, err := f.sys.backend.HeaderByNumber(ctx, rpc.LatestBlockNumber) + if err != nil { + return 0, err + } if hdr == nil { return 0, errors.New("latest header not found") } return hdr.Number.Uint64(), nil case rpc.SafeBlockNumber.Int64(): - hdr, _ := f.sys.backend.HeaderByNumber(ctx, rpc.SafeBlockNumber) + hdr, err := f.sys.backend.HeaderByNumber(ctx, rpc.SafeBlockNumber) + if err != nil { + return 0, err + } if hdr == nil { return 0, errors.New("safe header not found") } return hdr.Number.Uint64(), nil case rpc.FinalizedBlockNumber.Int64(): - hdr, _ := f.sys.backend.HeaderByNumber(ctx, rpc.FinalizedBlockNumber) + hdr, err := f.sys.backend.HeaderByNumber(ctx, rpc.FinalizedBlockNumber) + if err != nil { + return 0, err + } if hdr == nil { return 0, errors.New("finalized header not found") } return hdr.Number.Uint64(), nil
169-171: Silent empty result whenbegin > endmay hide user errors.Returning
nil, nilwhenbegin > endsilently produces an empty result. This could mask misconfigured filter ranges. Consider returning an error to alert users of invalid ranges.Alternative: return an error for invalid range
if begin > end { - return nil, nil + return nil, fmt.Errorf("invalid block range: begin (%d) > end (%d)", begin, end) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
core/blockchain.gocore/blockchain_l2.gocore/blockchain_reader.gocore/rawdb/accessors_chain.gocore/rawdb/schema.goeth/api.goeth/api_backend.goeth/catalyst/l2_api.goeth/filters/filter.goeth/gasprice/feehistory.goethclient/authclient/engine.go
🧰 Additional context used
🧬 Code graph analysis (9)
ethclient/authclient/engine.go (2)
rpc/client.go (1)
Client(77-106)ethclient/authclient/client.go (1)
Client(10-12)
core/rawdb/accessors_chain.go (2)
ethdb/database.go (2)
KeyValueReader(23-29)KeyValueWriter(32-38)common/types.go (1)
BytesToHash(54-58)
core/blockchain_reader.go (1)
core/blockchain.go (1)
BlockChain(164-220)
core/blockchain_l2.go (3)
core/blockchain.go (1)
BlockChain(164-220)core/types/block.go (1)
Header(70-114)core/rawdb/accessors_chain.go (1)
WriteFinalizedBlockHash(228-232)
eth/catalyst/l2_api.go (1)
core/blockchain.go (1)
BlockChain(164-220)
eth/filters/filter.go (1)
rpc/types.go (4)
PendingBlockNumber(68-68)LatestBlockNumber(69-69)SafeBlockNumber(66-66)FinalizedBlockNumber(67-67)
eth/api_backend.go (1)
rpc/types.go (2)
SafeBlockNumber(66-66)FinalizedBlockNumber(67-67)
eth/api.go (5)
core/types/gen_header_json.go (2)
Header(18-43)Header(74-98)core/types/block.go (1)
Header(70-114)rpc/types.go (3)
LatestBlockNumber(69-69)SafeBlockNumber(66-66)FinalizedBlockNumber(67-67)core/state/dump.go (2)
Dump(65-68)IteratorDump(81-85)core/blockchain.go (1)
BlockChain(164-220)
core/blockchain.go (2)
core/types/block.go (1)
Header(70-114)core/rawdb/accessors_chain.go (1)
ReadFinalizedBlockHash(219-225)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (go)
🔇 Additional comments (7)
core/rawdb/schema.go (1)
46-48: LGTM!The new
headFinalizedBlockKeyfollows the established naming convention and placement pattern for head block keys in this file.core/rawdb/accessors_chain.go (1)
218-232: LGTM!The new accessor functions follow the established patterns in this file:
ReadFinalizedBlockHashreturns zero hash when data is missing (consistent withReadHeadHeaderHash,ReadHeadBlockHash)WriteFinalizedBlockHashuseslog.Criton failure (consistent with other write functions)core/blockchain.go (1)
197-199: LGTM!Using
atomic.Pointer[types.Header]is appropriate for thread-safe access to these headers without requiring mutex locks for readers.ethclient/authclient/engine.go (1)
60-64: LGTM!The new
SetBlockTagsmethod follows the established pattern of other engine RPC methods in this file. The signature is clean, and delegating validation to the RPC handler is consistent with sibling methods likeCommitBatchandAppendBlsSignature.core/blockchain_reader.go (1)
52-64: LGTM!The new accessors are clean, thread-safe via atomic pointers, and well-documented. Returning
*types.Header(rather than*types.Block) is consistent with the underlying atomic field types and appropriate for the safe/finalized block tracking use case.eth/api_backend.go (1)
78-93: LGTM!The
HeaderByNumberimplementation for safe and finalized blocks is correct. The nil checks are properly placed, and the error messages are clear and consistent.eth/gasprice/feehistory.go (1)
199-207: LGTM!Early exit check and genesis boundary clamping logic are correct. The
uint64(reqEnd)cast is safe since all negative block tags are resolved to positive values by this point.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| func (bc *BlockChain) SetSafe(header *types.Header) { | ||
| bc.currentSafeBlock.Store(header) | ||
| log.Debug("Set safe block", "number", header.Number, "hash", header.Hash()) | ||
| } |
There was a problem hiding this comment.
Add nil check to prevent potential panic.
If header is nil, the log.Debug call will panic when accessing header.Number and header.Hash(). Consider adding a nil guard.
🛡️ Suggested fix
func (bc *BlockChain) SetSafe(header *types.Header) {
+ if header == nil {
+ return
+ }
bc.currentSafeBlock.Store(header)
log.Debug("Set safe block", "number", header.Number, "hash", header.Hash())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (bc *BlockChain) SetSafe(header *types.Header) { | |
| bc.currentSafeBlock.Store(header) | |
| log.Debug("Set safe block", "number", header.Number, "hash", header.Hash()) | |
| } | |
| func (bc *BlockChain) SetSafe(header *types.Header) { | |
| if header == nil { | |
| return | |
| } | |
| bc.currentSafeBlock.Store(header) | |
| log.Debug("Set safe block", "number", header.Number, "hash", header.Hash()) | |
| } |
🤖 Prompt for AI Agents
In `@core/blockchain_l2.go` around lines 254 - 257, The SetSafe method can panic
if header is nil because it dereferences header.Number and header.Hash(); add a
nil guard at the start of BlockChain.SetSafe(header *types.Header) that returns
early (or logs a warning) when header == nil, and only call
bc.currentSafeBlock.Store(header) and log.Debug(...) when header is non-nil (use
the function name SetSafe and variable header to locate the change).
| func (bc *BlockChain) SetFinalized(header *types.Header) { | ||
| bc.currentFinalizedBlock.Store(header) | ||
| // Persist to rawdb for recovery on restart | ||
| rawdb.WriteFinalizedBlockHash(bc.db, header.Hash()) | ||
| log.Debug("Set finalized block", "number", header.Number, "hash", header.Hash()) | ||
| } |
There was a problem hiding this comment.
Add nil check to prevent potential panic.
Similar to SetSafe, if header is nil, the code will panic when accessing header.Hash() and header.Number in the log statement. The WriteFinalizedBlockHash call would also write a zero hash, which might cause issues on restart recovery.
🛡️ Suggested fix
func (bc *BlockChain) SetFinalized(header *types.Header) {
+ if header == nil {
+ return
+ }
bc.currentFinalizedBlock.Store(header)
// Persist to rawdb for recovery on restart
rawdb.WriteFinalizedBlockHash(bc.db, header.Hash())
log.Debug("Set finalized block", "number", header.Number, "hash", header.Hash())
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (bc *BlockChain) SetFinalized(header *types.Header) { | |
| bc.currentFinalizedBlock.Store(header) | |
| // Persist to rawdb for recovery on restart | |
| rawdb.WriteFinalizedBlockHash(bc.db, header.Hash()) | |
| log.Debug("Set finalized block", "number", header.Number, "hash", header.Hash()) | |
| } | |
| func (bc *BlockChain) SetFinalized(header *types.Header) { | |
| if header == nil { | |
| return | |
| } | |
| bc.currentFinalizedBlock.Store(header) | |
| // Persist to rawdb for recovery on restart | |
| rawdb.WriteFinalizedBlockHash(bc.db, header.Hash()) | |
| log.Debug("Set finalized block", "number", header.Number, "hash", header.Hash()) | |
| } |
🤖 Prompt for AI Agents
In `@core/blockchain_l2.go` around lines 263 - 268, SetFinalized may panic or
write an invalid hash when header is nil; add a nil check at the start of
BlockChain.SetFinalized (the method) and return early if header == nil (or log
and return) so you do not call currentFinalizedBlock.Store(nil) nor
rawdb.WriteFinalizedBlockHash with header.Hash(), and avoid accessing
header.Number in log.Debug; only perform Store, rawdb.WriteFinalizedBlockHash
and log.Debug when header is non-nil.
| if finalizedBlock := bc.CurrentFinalizedBlock(); finalizedBlock != nil { | ||
| log.Info("Loaded most recent local finalized block", "number", finalizedBlock.Number, "hash", finalizedBlock.Hash()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the structure of core/blockchain.go and locate setHeadBeyondRoot
wc -l core/blockchain.goRepository: morph-l2/go-ethereum
Length of output: 87
🏁 Script executed:
# Search for setHeadBeyondRoot and SetHead implementations
rg -n "func.*setHeadBeyondRoot|func.*SetHead" core/blockchain.goRepository: morph-l2/go-ethereum
Length of output: 224
🏁 Script executed:
# Check currentFinalizedBlock and currentSafeBlock usages throughout the codebase
rg -n "currentFinalizedBlock|currentSafeBlock" --type go core/Repository: morph-l2/go-ethereum
Length of output: 746
🏁 Script executed:
# Read the SetHead function first
sed -n '514,526p' core/blockchain.goRepository: morph-l2/go-ethereum
Length of output: 718
🏁 Script executed:
# Read setHeadBeyondRoot implementation - first part
sed -n '527,570p' core/blockchain.goRepository: morph-l2/go-ethereum
Length of output: 2322
🏁 Script executed:
# Read more of setHeadBeyondRoot
sed -n '570,620p' core/blockchain.goRepository: morph-l2/go-ethereum
Length of output: 2462
🏁 Script executed:
# Read the rest of setHeadBeyondRoot
sed -n '620,662p' core/blockchain.goRepository: morph-l2/go-ethereum
Length of output: 1743
🏁 Script executed:
# Search for loadLastState implementation
rg -n "func.*loadLastState" core/blockchain.goRepository: morph-l2/go-ethereum
Length of output: 113
🏁 Script executed:
# Read the loadLastState function
sed -n '481,510p' core/blockchain.goRepository: morph-l2/go-ethereum
Length of output: 1768
🏁 Script executed:
# Search for GetHeaderByHash implementation
rg -n "func.*GetHeaderByHash" core/blockchain.goRepository: morph-l2/go-ethereum
Length of output: 46
🏁 Script executed:
# Read the GetHeaderByHash function
sed -n '246,260p' core/blockchain.goRepository: morph-l2/go-ethereum
Length of output: 585
🏁 Script executed:
# Search for GetHeaderByHash more broadly
rg -n "GetHeaderByHash" --type go core/Repository: morph-l2/go-ethereum
Length of output: 1741
🏁 Script executed:
# Get the line numbers for GetHeaderByHash in blockchain_reader.go
rg -n "func.*GetHeaderByHash" --type go core/blockchain_reader.goRepository: morph-l2/go-ethereum
Length of output: 138
🏁 Script executed:
# Read the GetHeaderByHash function from blockchain_reader.go
sed -n '89,110p' core/blockchain_reader.goRepository: morph-l2/go-ethereum
Length of output: 655
🏁 Script executed:
# Read the actual GetHeaderByHash implementation in headerchain.go
sed -n '469,480p' core/headerchain.goRepository: morph-l2/go-ethereum
Length of output: 490
🏁 Script executed:
# Check what comes after GetHeaderByHash in headerchain.go to see full implementation
sed -n '469,500p' core/headerchain.goRepository: morph-l2/go-ethereum
Length of output: 1209
🏁 Script executed:
# Check if SetHead or setHeadBeyondRoot updates the finalized block hash in database
rg -n "WriteFinalizedBlockHash|WriteFinalized" --type go core/Repository: morph-l2/go-ethereum
Length of output: 359
🏁 Script executed:
# Check what WriteFinalizedBlockHash does and where it's called
rg -n "WriteFinalizedBlockHash\|WriteSafeBlockHash" --type goRepository: morph-l2/go-ethereum
Length of output: 46
🏁 Script executed:
# Check where WriteFinalizedBlockHash is defined and what it does
sed -n '227,235p' core/rawdb/accessors_chain.goRepository: morph-l2/go-ethereum
Length of output: 484
🏁 Script executed:
# Check if the finalized block header would be deleted during SetHead chain rewind
# Look at the delFn function more closely - what gets deleted?
sed -n '650,670p' core/blockchain.goRepository: morph-l2/go-ethereum
Length of output: 771
🏁 Script executed:
# Check how blockchain_l2.go updates finalized blocks
sed -n '255,275p' core/blockchain_l2.goRepository: morph-l2/go-ethereum
Length of output: 714
Clear safe/finalized blocks during chain rewind to prevent stale references.
When setHeadBeyondRoot rewinds the chain below the current finalized block, the finalized block header gets deleted from the database. However, loadLastState() attempts to reload it via GetHeaderByHash(), which returns nil if the header is no longer in the canonical chain. In this case, currentFinalizedBlock is not explicitly cleared and remains pointing to a stale block that is no longer part of the canonical chain. Add logic to clear or validate currentFinalizedBlock and currentSafeBlock when GetHeaderByHash() fails during chain rewind recovery.
🤖 Prompt for AI Agents
In `@core/blockchain.go` around lines 502 - 504, When recovering from a chain
rewind in setHeadBeyondRoot/loadLastState, ensure any finalized or safe block
references are validated and cleared if their headers are missing: after calling
GetHeaderByHash() in loadLastState (or the rewind recovery path), check for a
nil return and if nil set currentFinalizedBlock and/or currentSafeBlock to nil
(or otherwise invalidate them) and update any dependent state; also add the same
validation in setHeadBeyondRoot where headers may be deleted so you don't retain
stale pointers to removed headers.
| var header *types.Header | ||
| switch blockNr { | ||
| case rpc.LatestBlockNumber: | ||
| header = api.eth.blockchain.CurrentBlock().Header() | ||
| case rpc.SafeBlockNumber: | ||
| header = api.eth.blockchain.CurrentSafeBlock() | ||
| case rpc.FinalizedBlockNumber: | ||
| header = api.eth.blockchain.CurrentFinalizedBlock() | ||
| default: | ||
| block := api.eth.blockchain.GetBlockByNumber(uint64(blockNr)) | ||
| if block == nil { | ||
| return state.Dump{}, fmt.Errorf("block #%d not found", blockNr) | ||
| } | ||
| header = block.Header() | ||
| } | ||
| if block == nil { | ||
| if header == nil { | ||
| return state.Dump{}, fmt.Errorf("block #%d not found", blockNr) | ||
| } | ||
| stateDb, err := api.eth.BlockChain().StateAt(block.Root()) | ||
| stateDb, err := api.eth.BlockChain().StateAt(header.Root) |
There was a problem hiding this comment.
Error message shows raw negative block number for safe/finalized.
When CurrentSafeBlock() or CurrentFinalizedBlock() returns nil, the error at Line 195 will display the raw blockNr value (e.g., -4 or -3), which is confusing to users.
Proposed fix with descriptive error messages
var header *types.Header
switch blockNr {
case rpc.LatestBlockNumber:
header = api.eth.blockchain.CurrentBlock().Header()
case rpc.SafeBlockNumber:
header = api.eth.blockchain.CurrentSafeBlock()
+ if header == nil {
+ return state.Dump{}, errors.New("safe block not available")
+ }
case rpc.FinalizedBlockNumber:
header = api.eth.blockchain.CurrentFinalizedBlock()
+ if header == nil {
+ return state.Dump{}, errors.New("finalized block not available")
+ }
default:
block := api.eth.blockchain.GetBlockByNumber(uint64(blockNr))
if block == nil {
return state.Dump{}, fmt.Errorf("block #%d not found", blockNr)
}
header = block.Header()
}
- if header == nil {
- return state.Dump{}, fmt.Errorf("block #%d not found", blockNr)
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var header *types.Header | |
| switch blockNr { | |
| case rpc.LatestBlockNumber: | |
| header = api.eth.blockchain.CurrentBlock().Header() | |
| case rpc.SafeBlockNumber: | |
| header = api.eth.blockchain.CurrentSafeBlock() | |
| case rpc.FinalizedBlockNumber: | |
| header = api.eth.blockchain.CurrentFinalizedBlock() | |
| default: | |
| block := api.eth.blockchain.GetBlockByNumber(uint64(blockNr)) | |
| if block == nil { | |
| return state.Dump{}, fmt.Errorf("block #%d not found", blockNr) | |
| } | |
| header = block.Header() | |
| } | |
| if block == nil { | |
| if header == nil { | |
| return state.Dump{}, fmt.Errorf("block #%d not found", blockNr) | |
| } | |
| stateDb, err := api.eth.BlockChain().StateAt(block.Root()) | |
| stateDb, err := api.eth.BlockChain().StateAt(header.Root) | |
| var header *types.Header | |
| switch blockNr { | |
| case rpc.LatestBlockNumber: | |
| header = api.eth.blockchain.CurrentBlock().Header() | |
| case rpc.SafeBlockNumber: | |
| header = api.eth.blockchain.CurrentSafeBlock() | |
| if header == nil { | |
| return state.Dump{}, errors.New("safe block not available") | |
| } | |
| case rpc.FinalizedBlockNumber: | |
| header = api.eth.blockchain.CurrentFinalizedBlock() | |
| if header == nil { | |
| return state.Dump{}, errors.New("finalized block not available") | |
| } | |
| default: | |
| block := api.eth.blockchain.GetBlockByNumber(uint64(blockNr)) | |
| if block == nil { | |
| return state.Dump{}, fmt.Errorf("block #%d not found", blockNr) | |
| } | |
| header = block.Header() | |
| } | |
| stateDb, err := api.eth.BlockChain().StateAt(header.Root) |
🤖 Prompt for AI Agents
In `@eth/api.go` around lines 179 - 197, When header is nil, the error currently
prints the raw blockNr (which can be negative sentinel values); change the
nil-check/error path around header (after switch that calls
CurrentSafeBlock()/CurrentFinalizedBlock()/CurrentBlock()) to map those sentinel
constants (rpc.LatestBlockNumber, rpc.SafeBlockNumber, rpc.FinalizedBlockNumber)
to human-readable names and return errors like "latest block not found", "safe
block not found", or "finalized block not found" instead of "block `#-4` not
found"; keep the existing behavior for numeric block numbers (the default case
using GetBlockByNumber) and use the blockNr variable only when it represents an
actual numeric block index.
| var header *types.Header | ||
| switch number { | ||
| case rpc.LatestBlockNumber: | ||
| header = api.eth.blockchain.CurrentBlock().Header() | ||
| case rpc.SafeBlockNumber: | ||
| header = api.eth.blockchain.CurrentSafeBlock() | ||
| case rpc.FinalizedBlockNumber: | ||
| header = api.eth.blockchain.CurrentFinalizedBlock() | ||
| default: | ||
| block := api.eth.blockchain.GetBlockByNumber(uint64(number)) | ||
| if block == nil { | ||
| return state.IteratorDump{}, fmt.Errorf("block #%d not found", number) | ||
| } | ||
| header = block.Header() | ||
| } | ||
| if block == nil { | ||
| if header == nil { | ||
| return state.IteratorDump{}, fmt.Errorf("block #%d not found", number) | ||
| } | ||
| stateDb, err = api.eth.BlockChain().StateAt(block.Root()) | ||
| stateDb, err = api.eth.BlockChain().StateAt(header.Root) |
There was a problem hiding this comment.
Same error message issue in AccountRange.
The error at Line 292 has the same issue as DumpBlock — it will show confusing negative block numbers for safe/finalized requests.
Proposed fix with descriptive error messages
var header *types.Header
switch number {
case rpc.LatestBlockNumber:
header = api.eth.blockchain.CurrentBlock().Header()
case rpc.SafeBlockNumber:
header = api.eth.blockchain.CurrentSafeBlock()
+ if header == nil {
+ return state.IteratorDump{}, errors.New("safe block not available")
+ }
case rpc.FinalizedBlockNumber:
header = api.eth.blockchain.CurrentFinalizedBlock()
+ if header == nil {
+ return state.IteratorDump{}, errors.New("finalized block not available")
+ }
default:
block := api.eth.blockchain.GetBlockByNumber(uint64(number))
if block == nil {
return state.IteratorDump{}, fmt.Errorf("block #%d not found", number)
}
header = block.Header()
}
- if header == nil {
- return state.IteratorDump{}, fmt.Errorf("block #%d not found", number)
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var header *types.Header | |
| switch number { | |
| case rpc.LatestBlockNumber: | |
| header = api.eth.blockchain.CurrentBlock().Header() | |
| case rpc.SafeBlockNumber: | |
| header = api.eth.blockchain.CurrentSafeBlock() | |
| case rpc.FinalizedBlockNumber: | |
| header = api.eth.blockchain.CurrentFinalizedBlock() | |
| default: | |
| block := api.eth.blockchain.GetBlockByNumber(uint64(number)) | |
| if block == nil { | |
| return state.IteratorDump{}, fmt.Errorf("block #%d not found", number) | |
| } | |
| header = block.Header() | |
| } | |
| if block == nil { | |
| if header == nil { | |
| return state.IteratorDump{}, fmt.Errorf("block #%d not found", number) | |
| } | |
| stateDb, err = api.eth.BlockChain().StateAt(block.Root()) | |
| stateDb, err = api.eth.BlockChain().StateAt(header.Root) | |
| var header *types.Header | |
| switch number { | |
| case rpc.LatestBlockNumber: | |
| header = api.eth.blockchain.CurrentBlock().Header() | |
| case rpc.SafeBlockNumber: | |
| header = api.eth.blockchain.CurrentSafeBlock() | |
| if header == nil { | |
| return state.IteratorDump{}, errors.New("safe block not available") | |
| } | |
| case rpc.FinalizedBlockNumber: | |
| header = api.eth.blockchain.CurrentFinalizedBlock() | |
| if header == nil { | |
| return state.IteratorDump{}, errors.New("finalized block not available") | |
| } | |
| default: | |
| block := api.eth.blockchain.GetBlockByNumber(uint64(number)) | |
| if block == nil { | |
| return state.IteratorDump{}, fmt.Errorf("block #%d not found", number) | |
| } | |
| header = block.Header() | |
| } | |
| stateDb, err = api.eth.BlockChain().StateAt(header.Root) |
| switch reqEnd { | ||
| case rpc.PendingBlockNumber: | ||
| if pendingBlock, pendingReceipts, _ = oracle.backend.Pending(); pendingBlock != nil { | ||
| resolved = pendingBlock.Header() | ||
| } else { | ||
| // Pending block not supported by backend, process only until latest block. | ||
| resolved = headBlock | ||
|
|
||
| // Update total blocks to return to account for this. | ||
| blocks-- | ||
| } | ||
| case rpc.LatestBlockNumber: | ||
| // Retrieved above. | ||
| resolved = headBlock | ||
| case rpc.SafeBlockNumber: | ||
| resolved, err = oracle.backend.HeaderByNumber(ctx, rpc.SafeBlockNumber) | ||
| case rpc.FinalizedBlockNumber: | ||
| resolved, err = oracle.backend.HeaderByNumber(ctx, rpc.FinalizedBlockNumber) | ||
| //case rpc.EarliestBlockNumber: // TODO: uncomment this when HistoryPruningCutoff() added | ||
| // resolved, err = oracle.backend.HeaderByNumber(ctx, rpc.EarliestBlockNumber) | ||
| } | ||
| } | ||
| if pendingBlock == nil { | ||
| // if pending block is not fetched then we retrieve the head header to get the head block number | ||
| if latestHeader, err := oracle.backend.HeaderByNumber(ctx, rpc.LatestBlockNumber); err == nil { | ||
| headBlock = rpc.BlockNumber(latestHeader.Number.Uint64()) | ||
| } else { | ||
| if resolved == nil || err != nil { | ||
| return nil, nil, 0, 0, err | ||
| } |
There was a problem hiding this comment.
Missing default case can cause silent failures for unsupported block tags.
If reqEnd is a negative value not handled by the switch (e.g., rpc.EarliestBlockNumber = -5), resolved remains nil and err is nil, causing the function to return nil, nil, 0, 0, nil. Callers cannot distinguish this from a legitimate "no blocks available" scenario.
Proposed fix: Add a default case to return an explicit error
case rpc.FinalizedBlockNumber:
resolved, err = oracle.backend.HeaderByNumber(ctx, rpc.FinalizedBlockNumber)
//case rpc.EarliestBlockNumber: // TODO: uncomment this when HistoryPruningCutoff() added
// resolved, err = oracle.backend.HeaderByNumber(ctx, rpc.EarliestBlockNumber)
+ default:
+ return nil, nil, 0, 0, fmt.Errorf("unsupported block tag: %d", reqEnd)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| switch reqEnd { | |
| case rpc.PendingBlockNumber: | |
| if pendingBlock, pendingReceipts, _ = oracle.backend.Pending(); pendingBlock != nil { | |
| resolved = pendingBlock.Header() | |
| } else { | |
| // Pending block not supported by backend, process only until latest block. | |
| resolved = headBlock | |
| // Update total blocks to return to account for this. | |
| blocks-- | |
| } | |
| case rpc.LatestBlockNumber: | |
| // Retrieved above. | |
| resolved = headBlock | |
| case rpc.SafeBlockNumber: | |
| resolved, err = oracle.backend.HeaderByNumber(ctx, rpc.SafeBlockNumber) | |
| case rpc.FinalizedBlockNumber: | |
| resolved, err = oracle.backend.HeaderByNumber(ctx, rpc.FinalizedBlockNumber) | |
| //case rpc.EarliestBlockNumber: // TODO: uncomment this when HistoryPruningCutoff() added | |
| // resolved, err = oracle.backend.HeaderByNumber(ctx, rpc.EarliestBlockNumber) | |
| } | |
| } | |
| if pendingBlock == nil { | |
| // if pending block is not fetched then we retrieve the head header to get the head block number | |
| if latestHeader, err := oracle.backend.HeaderByNumber(ctx, rpc.LatestBlockNumber); err == nil { | |
| headBlock = rpc.BlockNumber(latestHeader.Number.Uint64()) | |
| } else { | |
| if resolved == nil || err != nil { | |
| return nil, nil, 0, 0, err | |
| } | |
| switch reqEnd { | |
| case rpc.PendingBlockNumber: | |
| if pendingBlock, pendingReceipts, _ = oracle.backend.Pending(); pendingBlock != nil { | |
| resolved = pendingBlock.Header() | |
| } else { | |
| // Pending block not supported by backend, process only until latest block. | |
| resolved = headBlock | |
| // Update total blocks to return to account for this. | |
| blocks-- | |
| } | |
| case rpc.LatestBlockNumber: | |
| // Retrieved above. | |
| resolved = headBlock | |
| case rpc.SafeBlockNumber: | |
| resolved, err = oracle.backend.HeaderByNumber(ctx, rpc.SafeBlockNumber) | |
| case rpc.FinalizedBlockNumber: | |
| resolved, err = oracle.backend.HeaderByNumber(ctx, rpc.FinalizedBlockNumber) | |
| //case rpc.EarliestBlockNumber: // TODO: uncomment this when HistoryPruningCutoff() added | |
| // resolved, err = oracle.backend.HeaderByNumber(ctx, rpc.EarliestBlockNumber) | |
| default: | |
| return nil, nil, 0, 0, fmt.Errorf("unsupported block tag: %d", reqEnd) | |
| } | |
| if resolved == nil || err != nil { | |
| return nil, nil, 0, 0, err | |
| } |
🤖 Prompt for AI Agents
In `@eth/gasprice/feehistory.go` around lines 171 - 194, The switch handling
reqEnd (in the fee history resolution block) can leave resolved and err nil for
unsupported block tags; add a default case to the switch that sets err to a
descriptive error (e.g., fmt.Errorf or errors.New) indicating an
unsupported/unknown block tag and ensure the function returns that error instead
of nil, so callers can distinguish invalid reqEnd values; update any references
to resolved/err after the switch (same function) to rely on this explicit error
path.
2 participants
1. Purpose or design rationale of this PR
...
2. PR title
Your PR title must follow conventional commits (as we are doing squash merge for each PR), so it must start with one of the following types:
3. Deployment tag versioning
Has the version in
params/version.gobeen updated?4. Breaking change label
Does this PR have the
breaking-changelabel?Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.