Skip to content

consensus/bor, internal/cli: full grpc implementation #2194

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
marcello33 wants to merge 8 commits into
base: develop
Choose a base branch
from

address comments

fe62ec8
Select commit
Loading
Failed to load commit list.
Open

consensus/bor, internal/cli: full grpc implementation #2194

address comments
fe62ec8
Select commit
Loading
Failed to load commit list.
Claude / Claude Code Review completed Apr 23, 2026 in 14m 56s

Code review found 1 important issue

Found 5 candidates, confirmed 4. See review comments for details.

Details

Severity Count
🔴 Important 1
🟡 Nit 1
🟣 Pre-existing 1
Severity File:Line Issue
🔴 Important internal/cli/server/api_service.go:176-187 GetTdByHash panics on unset/partial Hash field
🟡 Nit internal/cli/server/api_service.go:241-270 fetchBlockInfo swallows backend errors as gap-stop
🟣 Pre-existing internal/cli/server/api_service.go:189-202 GetTdByNumber non-deterministic for special block tags

Annotations

Check failure on line 187 in internal/cli/server/api_service.go

See this annotation in the file changed.

@claude claude / Claude Code Review

GetTdByHash panics on unset/partial Hash field 

The newly-added `GetTdByHash` handler passes `req.Hash` directly to `protoutil.ConvertH256ToHash` without a nil guard — any request with `Hash=nil` (proto3 default), `Hash=&H256{}`, or a partially populated `H256` panics inside `ConvertH256ToHash` when it dereferences `h256.Hi.Hi` / `h256.Lo.Hi`. grpc-go recovers the panic and returns `codes.Internal`, so the process survives, but every malformed request logs a full stack trace (log-flood vector) and clients receive the wrong status code (should

Check warning on line 270 in internal/cli/server/api_service.go

See this annotation in the file changed.

@claude claude / Claude Code Review

fetchBlockInfo swallows backend errors as gap-stop 

`fetchBlockInfo` (api_service.go:241-271) collapses three outcomes — a legitimate gap (header==nil && err==nil), a backend error from HeaderByNumber, and an ecrecover/Author failure — into the same `(nil, false)` return, which the caller treats as gap-stop with no error/log/metric. An Author() error in particular is never a gap (it indicates a corrupted seal) yet silently truncates the response. Practical exposure today is narrow (HeaderByNumber currently swallows backend errors to nil, and Auth

Check notice on line 202 in internal/cli/server/api_service.go

See this annotation in the file changed.

@claude claude / Claude Code Review

GetTdByNumber non-deterministic for special block tags 

The new gRPC `GetTdByNumber` (api_service.go:189-202) forwards special block tags ("latest", "earliest", "finalized", "safe", "pending") straight into `EthAPIBackend.GetTdByNumber` (eth/api_backend.go:357-362), which calls `blockchain.GetTd(header.Hash(), uint64(blockNr.Int64()))`. For tag=-1 (latest) that becomes `uint64(-1)=MaxUint64`, so the DB key is wrong — the RPC returns the cached TD on a hash cache hit and `"total difficulty not found"` on cache miss, i.e. it's non-deterministic. This i
View more details on Claude