app, helper, x/bor: full grpc implementation

[marcello33]

Summary

In this PR, we implement the client-side half of the heimdall↔bor gRPC migration. Every bor-facing JSON-RPC now has a gRPC path, selectable per-node via bor_grpc_flag. The HTTP path remains the default, fully supported, and untouched in behavior (besides some additional checks).

Results

• All 7 methods are faster or at parity on gRPC.
• Biggest win: GetBorChainBlockInfoInBatch with 77% latency reduction — the call used in the consensus-critical ExtendVoteHandler path.
• 50–67% fewer bytes allocated per RPC across the board — less GC pressure.

Latency (ns per op)

Method                          HTTP ns/op    gRPC ns/op    Improvement
-------------------------------------------------------------------------
GetBorChainBlock                  584 873       406 353       -30.5%
GetBorChainBlockAuthor            476 365       417 626       -12.3%
GetBorChainBlockTd                466 027       404 187       -13.3%
GetBorChainBlockInfoInBatch     7 819 395     1 771 033       -77.3%
GetRootHash                       452 269       482 345        +6.7%
CheckIfBlocksExist                540 378       416 757       -22.9%
GetBlockByNumber                  644 355       430 615       -33.2%

Allocations (bytes and count per op)

Method                        HTTP B/op    gRPC B/op      Δ       HTTP allocs  gRPC allocs
-------------------------------------------------------------------------------------------
GetBorChainBlock                22 020        9 197    -58.2%          143          129
GetBorChainBlockAuthor          12 083        5 827    -51.8%          100           98
GetBorChainBlockTd              13 040        5 843    -55.2%          114           96
GetBorChainBlockInfoInBatch  1 164 435      386 226    -66.8%         6615         3473
GetRootHash                     12 171        5 811    -52.3%           98           95
CheckIfBlocksExist              22 088        9 205    -58.3%          147          131
GetBlockByNumber                24 853        9 596    -61.4%          163          132

Ambient system-level resource usage (per phase, 5 s interval across 1000 blocks)

Metric                       HTTP phase    gRPC phase       Δ
-------------------------------------------------------------
bor CPU avg                      2.60%         2.77%      +6.5%
bor CPU p95                      5.75%         7.18%     +24.9%
bor mem avg                  107.3 MiB     127.9 MiB     +19.2%
bor mem p95                  343.4 MiB     346.7 MiB      ~flat

heimdall CPU avg                 8.03%         7.94%      -1.1%
heimdall CPU p95                10.75%        11.30%      +5.1%
heimdall mem avg             121.5 MiB     127.9 MiB      +5.3%
heimdall mem p95             148.1 MiB     150.9 MiB      +1.9%

samples per phase (n=)           592           576

CPU samples during bench window (pprof, 120 s each, aligned to bench run)

Service                HTTP phase         gRPC phase          Δ
------------------------------------------------------------------
bor total CPU          35.25 s (29.4%)    34.93 s (29.1%)    -0.9%
heimdall total CPU      5.41 s  (4.5%)     6.34 s  (5.3%)   +17.2%

Heimdall CPU is ~17% higher on gRPC — consistent with the client doing proto decode per call instead of JSON-RPC decode.

Details

• New gRPC client methods: GetAuthor, GetTdByHash, GetTdByNumber, GetBlockInfoInBatch — matching bor PR #2194's server-side handlers.
• Full proto Header round-trip: protoHeaderToEthHeader now rebuilds ethTypes.Header fields with defensive bounds (see this PR).
• ContractCaller.BorChainGrpcClient is now BorGRPCClienter interface (concrete *grpc.BorGRPCClient still satisfies it); enables injectable fakes in tests without dialing a real bor.
• GetBorChainBlockAuthor signature change — now takes ctx context.Context as first argument, propagated through every caller / mock / test. Deterministic timeout propagation from ABCI handlers.
• Startup parity check — on the goroutine spawned at init, heimdall fetches the same bor header over both transports, compares ethTypes.Header.Hash(), and log.Fatals on three consecutive mismatches (guards against running with a stale bor that doesn't populate the full proto header).
• Comprehensive nil / bounds safety in the gRPC response decoders.
• GetRootHash response validation: both HTTP and gRPC transports share the checks.
• Single shared batch-size cap — MaxBlockInfoBatchSize = 10000 exported from x/bor/grpc and consumed by helper.GetBorChainBlockInfoInBatch, preventing end-start+1 int64 overflow and unbounded allocations on both transports from one source of truth.
• HTTP/gRPC behavioral parity across all 6 dispatcher functions. Especially, now they verify each returned header's block number against the expected slot, so a buggy/malicious bor cannot silently feed the wrong hash into milestone propositions on either transport.
• --bor_grpc_token CLI flag fully wired through UpdateWithFlags (alongside --bor_grpc_url / --bor_grpc_flag).

What this enables

• Milestone proposition path gets a single-round-trip batch call: GetBorChainBlockInfoInBatch replaces the 3×N JSON-RPC batch (eth_getHeaderByNumber × N, eth_getTdByNumber × N, bor_getAuthor × N). 4.4× faster, 66.8% fewer allocations on the devnet benchmark for a 90-block batch
• GetRootHash cache properly shared: bor-side fix ensures the LRU persists across gRPC calls. Was 2.1× slower before this (now at parity with HTTP).
• Real mainnet signal: we currently see ~130 GetBlockInfoInBatch HTTP timeouts per hour ("failed to get block batch info: Post \"http://localhost:8545\": context deadline exceeded" during milestone proposition generation). This PR is expected to eliminate that timeout class at the root.
• Heimdall↔bor no longer shares a JSON-RPC transport once operators flip bor_grpc_flag.

Enabling it

gRPC is opt-in. Default config has bor_grpc_flag = "false" — no behavior change.

Prerequisites

• Bor upgraded to a version that includes bor PR #2194 (the server-side handlers) and PR #2078 (the [grpc] config section). Heimdall refuses to run with bor_grpc_flag=true against a bor that doesn't serve the expanded header message (the parity check catches it at startup with a Warning / Fatal).

Step 1: Bor

On the bor node, enable [grpc] in config.toml (see bor PR description). Default is loopback 127.0.0.1:3131.

Step 2: Heimdall

Edit ~/.heimdalld/config/app.toml:

# Enable gRPC to bor.
bor_grpc_flag = "true"

# Dial address. Same-host validator pair:
bor_grpc_url  = "http://127.0.0.1:3131"
# Cross-host with TLS:
# bor_grpc_url = "https://bor.example.net:3131"

# Must match bor's --grpc.token / BOR_GRPC_TOKEN; empty if bor is unauthenticated.
bor_grpc_token = ""

Restart heimdall. Within ~5–30 seconds after startup the parity check emits one of:

• Bor gRPC hash parity check passed — proceed normally.
• Bor gRPC hash mismatch with HTTP for the same block — counting toward mismatch streak before fatal — followed by FATAL on the 3rd consecutive mismatch, typically meaning a bor upgrade is needed.
• Bor gRPC hash parity check gave up after retries — advisory only; means one transport was unreachable during the check window.

Security

• Plaintext + token refused on non-loopback: if bor_grpc_url = "http://remote:3131" with a non-empty bor_grpc_token, heimdall's client refuses to dial rather than transmit the token on the wire. Either use https:// for remote or leave the token empty on trusted internal networks.
• TLS transport gating: when bor_grpc_url starts with https://, heimdall sets RequireTransportSecurity=true on the per-RPC credentials. bearerToken will not leak if the server cert negotiation fails.
• Malformed server responses are non-fatal: every gRPC response decoder now nil-checks the outer and inner proto sub-messages, bounds-checks the variable-length byte fields (Bloom / Difficulty / BaseFee / Nonce), and verifies server-claimed block numbers against the request. A buggy or cross-version bor peer cannot panic a heimdall validator via a crafted response.

Tests

• Extensively unit-tested.
• Ran on a kurtosis-pos devnet by switching on-off the gRPC flag. Waited for 1000-block window each: producer rotation, milestone formation, state sync — all match between transports.
• HTTP vs gRPC benchmark against the same devnet: GetBlockInfoInBatch 4.4× faster on gRPC, 50–67% allocation reduction across all methods, GetRootHash regression closed via the cached API.

Dependencies

• Requires bor PR #2194 (server-side gRPC handlers + [grpc] section). Neither side is functional alone.
• Requires polyproto ≥ v0.0.8 for the new batch/td/author proto messages and the expanded Header with Cancun fields (see this PR).
• Requires kurtosis-pos changes (small: expose bor's [grpc] port, thread cl_bor_grpc_flag + el_grpc_url through the CL launcher template) if you want to run the end-to-end bench on a devnet. The changes are here.

Out of scope

• Receipt Bloom/Logs not decoded on the gRPC path — no heimdall consumer reads them today. Small follow-up when needed.
• Config override BorGRPCFlag bool merge is unconditional-overwrite (same behavior as every bool in CustomConfig.Merge). A proper fix requires switching to *bool or viper.IsSet, a broader config-overlay redesign tracked separately.

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

_{Tip: disable this comment in your organization's Code Review settings.}

[marcello33]

@claude review.

[claude]

Claude finished @marcello33's task in 2m 20s —— View job

---

Code Review: gRPC Implementation

• Gather context and understand the PR
• Review helper/config.go — gRPC configuration and connection management
• Review helper/call.go — gRPC-based contract calls
• Review x/bor/grpc/client.go — Bor gRPC client
• Review x/bor/grpc/query.go — Bor gRPC query layer
• Review test files
• Review remaining changes (go.mod, toml, linter config, etc.)
• Security review (helper/call.go is security-critical)
• Post final review

---

Overview

This PR adds comprehensive gRPC communication support between Heimdall and Bor, replacing or augmenting the existing HTTP JSON-RPC calls. Key changes:

1. x/bor/grpc/client.go: Full transport rework — URL scheme parsing (https/http/unix/bare), TLS support, bearer token auth, retry interceptors
2. x/bor/grpc/query.go: New gRPC query methods (GetAuthor, GetTdByHash, GetTdByNumber, GetBlockInfoInBatch), plus a full protoHeaderToEthHeader round-trip conversion
3. helper/call.go: All Bor-facing calls now dispatch to gRPC when BorChainGrpcFlag is set, via a new BorGRPCClienter interface
4. helper/config.go: Parity check system, gRPC config flags, startup accessibility checks
5. GetBorChainBlockAuthor signature change: Now accepts context.Context — breaking interface change propagated through all callers and mocks
6. Dependency bumps: polyproto, bor fork, grpc, otel, go 1.26.2

Overall this is a well-structured PR with excellent test coverage. The security-sensitive transport handling (TLS enforcement, token leak prevention) is well thought out. Below are findings by severity.

---

Security Findings

1. MEDIUM: Excessive gRPC retry configuration — potential for very long blocking calls

File: x/bor/grpc/client.go:155-158

retryOpts := []grpcRetry.CallOption{
    grpcRetry.WithMax(10000),
    grpcRetry.WithBackoff(grpcRetry.BackoffLinear(5 * time.Second)),
    grpcRetry.WithCodes(codes.Internal, codes.Unavailable, codes.Aborted, codes.NotFound),
}

WithMax(10000) with BackoffLinear(5s) means a single call could block for up to ~13.8 hours. In ABCI handlers (side handlers run in ExtendVote), the CometBFT timeout is ~10s. While the caller adds context.WithTimeout, if any path misses the timeout (or uses a long one), this retry config could stall the node.

Recommendation: Lower WithMax to something reasonable (e.g., 3-5 retries). The caller-side context.WithTimeout provides the primary budget — the retry interceptor should respect it, but defensive depth matters here. Also, codes.NotFound as a retry code is debatable — a block that doesn't exist won't appear on retry unless the chain advances.

Fix this →

2. MEDIUM: BorGRPCToken may appear in config dumps or process listings

File: helper/config.go:168

BorGRPCToken   string `mapstructure:"bor_grpc_token"`    // bearer token for bor gRPC; empty = no auth

The token is stored as a plain string in the CustomConfig struct and serialized to TOML (helper/toml.go:30). While the #nosec G101 on the constant name is appropriate, the token value itself could leak via:

• Config file on disk (readable by other processes)
• Debug log dumps of the config struct
• Process argument lists if passed via CLI flag

Recommendation: Consider masking the token in any String()/logging representation of the config. Ensure the TOML config file has restricted permissions (0600). The current secretFilePerm constant exists but isn't applied to the app.toml file.

3. LOW: protoHeaderToEthHeader does not validate Difficulty byte length

File: x/bor/grpc/query.go:240

Difficulty:  new(big.Int).SetBytes(p.Difficulty),

A malicious gRPC server could send an extremely large Difficulty byte slice, creating an arbitrarily large big.Int. Since this is consumed in side handlers (non-consensus), the impact is limited to the validator's own memory, but it's worth noting.

---

Code Quality Issues

4. Unreachable overflow check in HeaderByNumber and BlockByNumber

File: x/bor/grpc/query.go:49-51, 68-70

func (c *BorGRPCClient) HeaderByNumber(ctx context.Context, blockID int64) (*ethTypes.Header, error) {
    if blockID > math.MaxInt64 {
        return nil, fmt.Errorf("blockID too large: %d", blockID)
    }

blockID is already int64, so blockID > math.MaxInt64 is always false. The test comments correctly note this is unreachable. This dead code should be removed.

Fix this →

5. Inconsistent error returns — &ethTypes.Header{} vs nil

File: x/bor/grpc/query.go:62, 80

// On error:
return &ethTypes.Header{}, err  // HeaderByNumber
return &ethTypes.Block{}, err   // BlockByNumber

vs helper/call.go callers which check header == nil:

if header == nil || header.Number == nil {  // CheckIfBlocksExist, line 1131
    return false, nil
}

Returning a non-nil empty header on error means downstream nil checks won't catch the error case. The ethclient.Client.HeaderByNumber in go-ethereum returns nil, err on error. Consider aligning with that convention.

Fix this →

6. GetAuthor uses new() incorrectly with value type

File: x/bor/grpc/query.go:127

return new(common.BytesToAddress(arr[:])), nil

new() in Go allocates a zero-valued pointer. new(common.BytesToAddress(arr[:])) is not valid Go — this should be:

addr := common.BytesToAddress(arr[:])
return &addr, nil

The same pattern new(common.HexToAddress(...)) appears in many test files (e.g., query_test.go:539-543, keeper_test.go:453). This suggests a custom new function might exist in the codebase, or these won't compile. If this compiles, there's likely a Go 1.26+ language feature at play. If not, this is a build-breaking issue.

7. Merge logic for BorGRPCFlag is inverted

File: helper/config.go:1370-1372

if !cc.BorGRPCFlag {
    c.Custom.BorGRPCFlag = cc.BorGRPCFlag
}

This only merges when the override is false, meaning an override file can never enable gRPC — it can only disable it. Every other field in Merge uses the pattern "if override is non-zero, apply it." The gRPC flag should follow the same pattern, but booleans make this tricky since false is the zero value.

Recommendation: If the merge is intended to allow enabling gRPC from an override, this needs if cc.BorGRPCFlag { c.Custom.BorGRPCFlag = cc.BorGRPCFlag }. If it's intentionally only allowing disabling, add a comment explaining why.

8. receiptResponseToTypesReceipt doesn't convert Bloom or Logs

File: x/bor/grpc/query.go:193-210

The comment says "Bloom and Logs have been intentionally left out." This is fine for current usage, but if any future caller relies on Bloom or Logs from a gRPC receipt, they'll silently get empty values. Consider adding a // WARNING: comment or returning an error if these fields are needed.

---

Interface Change

9. GetBorChainBlockAuthor signature change (breaking)

Files: helper/call.go:68, x/bor/keeper/keeper.go:478,531,553, all test files

The IContractCaller interface method GetBorChainBlockAuthor changed from:

GetBorChainBlockAuthor(*big.Int) (*common.Address, error)

to:

GetBorChainBlockAuthor(context.Context, *big.Int) (*common.Address, error)

This is a good change — adding context allows proper timeout propagation. The propagation through keepers, mocks, and tests looks complete. The mock was regenerated correctly.

---

Architecture & Design (Positive)

• Transport resolution (resolveTransport, resolveHTTPS, resolveHTTP, resolveUnix, resolveNoScheme) is well-factored and handles security correctly — refusing bearer tokens over non-local plaintext
• Parity check system (verifyBorGRPCHashParity) is a smart safety net for detecting proto header conversion bugs. The streak-based mismatch detection avoids false positives from transient races
• BorGRPCClienter interface in helper/call.go enables clean testing and decouples the contract caller from the concrete gRPC client
• Test coverage is excellent — round-trip hash tests for Cancun/pre-Shanghai headers, batch layout tests, parity check tests with configurable retry/fatal functions
• Benchmark suite (grpc_vs_http_bench_test.go) behind a build tag is a good approach for performance comparison

---

Minor Observations

• app/abci_test.go: Changes are mostly test cleanup (removing unused variables, inlining message construction, fixing mock argument counts). No behavioral concerns.
• go.mod: polyproto pinned to a pre-release commit (v0.0.8-0.20260423132317-7d955b45ef8a). This should be updated to a tagged release before merging.
• bor fork: Also pinned to a pre-release commit. Same recommendation.
• packaging/templates/config/: bor_grpc_flag, bor_grpc_url, bor_grpc_token correctly added to both amoy and mainnet templates with sensible defaults (flag=false, url=localhost:3131, token empty).

---

Summary

This is a solid, well-tested implementation of gRPC communication with Bor. The main actionable items are:

1. Reduce retry count from 10,000 to something reasonable (security/availability)
2. Fix error return convention in HeaderByNumber/BlockByNumber to return nil instead of empty structs
3. Remove dead overflow checks in query.go
4. Pin dependencies to tagged releases before merge
5. Clarify the Merge logic for BorGRPCFlag
   | Branch: mardizzone/grpc_improv

[Copilot]

Pull request overview

This WIP PR introduces a Bor gRPC client implementation (including auth token support) and updates Heimdall’s Bor integration to use gRPC-capable APIs, alongside extensive test/config updates to validate and support the new transport.

Changes:

• Add Bor gRPC client features (bearer token auth, batch block info APIs, proto↔eth header conversion) and expand unit tests around these behaviors.
• Update Bor keeper/helper call paths and mocks to pass context.Context into Bor author lookups and support gRPC dispatch paths.
• Add config surface (bor_grpc_token) and introduce a background HTTP↔gRPC hash parity check during init to detect incompatible Bor versions.

Reviewed changes

Copilot reviewed 25 out of 26 changed files in this pull request and generated 21 comments.

Show a summary per file

File	Description
x/bor/keeper/side_msg_server_test.go	Update mock expectations for new GetBorChainBlockAuthor(ctx, ...) signature.
x/bor/keeper/keeper_test.go	Adjust tests for new contract-caller signature and seed producer storage.
x/bor/keeper/keeper.go	Pass ctx into GetBorChainBlockAuthor calls.
x/bor/keeper/grpc_query_test.go	Update query tests for new mock signature / seed producer setup.
x/bor/grpc/query.go	Implement new gRPC query methods (author/TD/batch) and proto header conversion helpers.
x/bor/grpc/query_test.go	Add/extend tests for header conversion and new gRPC query APIs.
x/bor/grpc/client.go	Add token-aware transport resolution (http/https/unix/no-scheme), interceptors, and bearer credentials.
x/bor/grpc/client_test.go	Expand unit tests for transport resolution, token behavior, dial timeout, and retry interceptors.
helper/call.go	Add gRPC dispatch for Bor calls and implement HTTP batch helpers (headers/TD/authors).
helper/config.go	Add bor_grpc_token config + background HTTP↔gRPC hash parity check logic.
helper/config_test.go	Add tests for nil-client parity dispatcher and mismatch streak state machine; verify flags registration.
helper/tx_test.go	Disable t.Parallel() in tests that mutate package-level globals.
helper/toml.go / helper/toml_test.go	Add bor_grpc_token to config template + tests.
packaging/templates/config/*/app.toml	Add bor_grpc_token to packaged templates.
helper/call_grpc_author_test.go	Add unit tests for gRPC author retrieval behavior in helper dispatcher.
helper/call_dispatcher_test.go	Add unit tests for gRPC dispatch error paths when gRPC client is nil.
helper/call_batch_test.go	Add unit tests for HTTP batch element layout/collation logic.
helper/parity_check_test.go	Add unit tests for parity check core loop and edge cases.
helper/grpc_vs_http_bench_test.go	Add bench-only comparison harness for HTTP vs gRPC against devnet.
helper/mocks/i_contract_caller.go	Regenerate mocks for updated GetBorChainBlockAuthor(ctx, blockNum) signature.
app/abci_test.go	Update tests for new contract-caller signature and msg construction patterns.
go.mod / go.sum	Bump Go version and update dependencies (polyproto/bor/otel/grpc/etc.).
.golangci.yml	Align golangci-lint Go version with module Go version.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov]

Codecov Report

❌ Patch coverage is 77.57009% with 96 lines in your changes missing coverage. Please review.
✅ Project coverage is 43.3%. Comparing base (25d9887) to head (2b4d0e7).

Files with missing lines	Patch %	Lines
x/bor/grpc/query.go	72.1%	21 Missing and 21 partials ⚠️
helper/call.go	69.7%	21 Missing and 5 partials ⚠️
helper/config.go	80.6%	18 Missing and 1 partial ⚠️
helper/mocks/i_contract_caller.go	25.0%	3 Missing and 3 partials ⚠️
x/bor/grpc/client.go	96.3%	2 Missing and 1 partial ⚠️

❌ Your patch check has failed because the patch coverage (77.5%) is below the target coverage (90.0%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff            @@
##           develop    #576     +/-   ##
=========================================
+ Coverage     41.6%   43.3%   +1.6%     
=========================================
  Files          172     172             
  Lines        17500   17772    +272     
=========================================
+ Hits          7294    7696    +402     
+ Misses        9126    8948    -178     
- Partials      1080    1128     +48     

Files with missing lines	Coverage Δ
helper/toml.go	60.0% <ø> (ø)
x/bor/keeper/keeper.go	66.7% <100.0%> (ø)
x/bor/grpc/client.go	92.3% <96.3%> (+40.5%)	⬆️
helper/mocks/i_contract_caller.go	18.6% <25.0%> (ø)
helper/config.go	44.1% <80.6%> (+24.5%)	⬆️
helper/call.go	23.2% <69.7%> (+12.7%)	⬆️
x/bor/grpc/query.go	79.6% <72.1%> (-15.2%)	⬇️

... and 2 files with indirect coverage changes

Files with missing lines	Coverage Δ
helper/toml.go	60.0% <ø> (ø)
x/bor/keeper/keeper.go	66.7% <100.0%> (ø)
x/bor/grpc/client.go	92.3% <96.3%> (+40.5%)	⬆️
helper/mocks/i_contract_caller.go	18.6% <25.0%> (ø)
helper/config.go	44.1% <80.6%> (+24.5%)	⬆️
helper/call.go	23.2% <69.7%> (+12.7%)	⬆️
x/bor/grpc/query.go	79.6% <72.1%> (-15.2%)	⬇️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[marcello33]

@claude addressed all your points.
Point 9 is noted, no change required.
Regarding point 8, we leave that for a potential next PR.
Review again.

[claude]

Claude finished @marcello33's task in 3m 10s —— View job

---

Re-Review: gRPC Implementation

• Gather context — read updated code for previously flagged issues
• Verify fix: inner-pointer nil checks in protoH256ToHash/protoH160ToAddress
• Verify fix: dead code overflow checks removed
• Verify fix: resolveHTTP empty host check
• Verify fix: HeaderByNumber/BlockByNumber nil response handling
• Verify fix: Bloom length validation
• Check previously raised issues (retry config, error return convention, Merge logic, token exposure)
• Check for any new issues in the latest commits
• Post final re-review

---

All major issues from the first review have been addressed. Here's the verification:

Verified Fixes

Issue	Status	Details
Retry config (10,000 → 4)	Fixed	client.go:162-165 — WithMax(4), BackoffLinear(500ms), removed codes.Internal/codes.NotFound
Dead overflow checks	Fixed	if blockID > math.MaxInt64 removed from both HeaderByNumber and BlockByNumber
Error return convention	Fixed	Both methods now return nil, err on error and nil, ethereum.NotFound on nil response, matching go-ethereum convention
Inner nil checks (H256/H160)	Fixed	protoH256ToHash checks h.Hi == nil || h.Lo == nil; protoH160ToAddress checks a.Hi == nil (query.go:290,299)
Bloom length validation	Fixed	protoHeaderToEthHeader returns nil if len(p.Bloom) > BloomByteLength (query.go:240)
Difficulty/BaseFee bounds	Fixed	protoHeaderToEthHeader returns nil if len(p.Difficulty) > 32 || len(p.BaseFee) > 32 (query.go:244)
HeaderByNumber nil response	Fixed	Returns nil, ethereum.NotFound when res == nil || res.Header == nil (query.go:59-61)
BlockByNumber nil response	Fixed	Returns nil, ethereum.NotFound when res == nil || res.Block == nil || res.Block.Header == nil (query.go:77-79)
resolveHTTP empty host	Fixed	client.go:111-116 — rejects http:// with empty host, matching resolveHTTPS behavior
Merge logic for BorGRPCFlag	Fixed	config.go:1370 — now unconditionally assigns c.Custom.BorGRPCFlag = cc.BorGRPCFlag
Receipt safe wrappers	Fixed	Latest commit (d8aab00) updated receiptResponseToTypesReceipt to use protoH256ToHash/protoH160ToAddress instead of raw protoutil calls

Test coverage for all fixes is solid — inner nil tests (TestProtoH256ToHash_InnerNil, TestProtoH160ToAddress_InnerNil), oversized bloom test (TestProtoHeaderToEthHeader_OversizedBloom), nil response tests for HeaderByNumber/BlockByNumber, empty host test for resolveHTTP.

---

One Remaining Issue

LOW: GetAuthor and GetBlockInfoInBatch still use raw protoutil.ConvertH160toAddress

Files: x/bor/grpc/query.go:130 and x/bor/grpc/query.go:189

Both GetAuthor and GetBlockInfoInBatch call protoutil.ConvertH160toAddress(res.Author) / protoutil.ConvertH160toAddress(b.Author) directly, bypassing the safe protoH160ToAddress wrapper that guards against a nil inner Hi field. While there's an outer nil check (res.Author == nil / b.Author != nil), a malicious server could send a non-nil H160 with a nil inner Hi sub-message, which would panic inside protoutil.ConvertH160toAddress.

The fix is trivial — use the safe wrapper that already exists:

// GetAuthor, line 130-131:
addr := protoH160ToAddress(res.Author)
return &addr, nil

// GetBlockInfoInBatch, line 188-190:
addr = protoH160ToAddress(b.Author)

Fix this →

---

Summary

The "address comments" and "safety check" commits thoroughly address all findings from the first review. The code is in good shape — transport security, nil safety, retry bounds, and error conventions are all solid. The one remaining item (using the safe H160 wrapper consistently) is low severity and a two-line fix.
| Branch: mardizzone/grpc_improv

[Copilot]

Pull request overview

Copilot reviewed 25 out of 26 changed files in this pull request and generated 9 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[marcello33]

@claude addressed your points from the previous run.
Review once more.

[claude]

Claude finished @marcello33's task in 1m 54s —— View job

---

Re-Review: gRPC Implementation (Round 3)

• Gather context — read latest code for all previously flagged files
• Verify fixes from re-review round 2 (receipt converter, GetAuthor safe wrappers, res nil guards)
• Verify fixes from copilot review (protoHeaderToEthHeader nil propagation, fatalFunc, range validation, duplicate log keys)
• Verify IPv6 ServerName fix in client.go
• Check for any new issues in latest commits
• Post final re-review

---

All issues from previous review rounds have been addressed. Here's the full verification:

Verified Fixes (latest commit cd7e0fe)

Issue	Status	Location
Receipt converter uses safe wrappers	Fixed	query.go:227-228,233 — protoH256ToHash/protoH160ToAddress
GetAuthor uses safe H160 wrapper	Fixed	query.go:142 — protoH160ToAddress(res.Author)
GetBlockInfoInBatch uses safe wrappers	Fixed	query.go:213 — protoH160ToAddress(b.Author)
HeaderByNumber nil-from-malformed-header	Fixed	query.go:65-68 — checks h == nil, returns ethereum.NotFound
BlockByNumber nil-from-malformed-header	Fixed	query.go:87-90 — checks header == nil, returns ethereum.NotFound
GetBlockInfoInBatch nil-from-malformed-header	Fixed	query.go:205-208 — checks h == nil, breaks
res == nil guards (GetTdByHash/GetTdByNumber/GetBlockInfoInBatch)	Fixed	query.go:153,167,191
fatalFunc now logs keysAndValues	Fixed	config.go:601-603 — Logger.Error(msg, keysAndValues...) + os.Exit(1)
Range validation in GetBorChainBlockInfoInBatch	Fixed	call.go:579-581 — shared start < 0 || end < 0 || end < start guard
Duplicate "error" log keys	Fixed	call.go:658 — "headerErr" / "tdErr"
IPv6 ServerName	Fixed	client.go:104 — u.Hostname() instead of strings.Split(addr, ":")[0]

Two Remaining Nits (non-blocking)

1. GetAuthor missing outer res == nil guard — query.go:135

if res.Author == nil {

Every sibling method now has if res == nil || ... before accessing response fields. GetAuthor skips the outer nil check. Not a real crash vector (standard gRPC stubs always return non-nil on success), but breaks the defensive pattern this PR established everywhere else. One-line fix: if res == nil || res.Author == nil {.

2. GetAuthor returns fmt.Errorf instead of ethereum.NotFound — query.go:136

return nil, fmt.Errorf("bor grpc GetAuthor: nil author")

All sibling methods return ethereum.NotFound for the "response present, payload nil" case. The dispatcher at helper/call.go:761-763 has a dead author == nil guard that would return ethereum.NotFound, but the gRPC path never returns (nil, nil) — it returns (nil, error) with a non-NotFound error. No behavioral impact on current callers since they only check err != nil, but a future errors.Is(err, ethereum.NotFound) caller would diverge between transports.

---

Summary

The PR is in excellent shape. All security-relevant findings (inner-pointer nil panics, malformed header propagation, receipt converter, retry config, IPv6 SNI, transport safety) have been thoroughly addressed across the three review rounds. The two remaining items are low-severity consistency nits — fine to fix now or defer.

| Branch: mardizzone/grpc_improv | View job

[marcello33]

@claude addressed your latest comments. Review.

[claude]

Claude finished @marcello33's task in 1m 10s —— View job

---

Re-Review: gRPC Implementation (Round 4)

• Gather context — read latest commit and updated code
• Verify fix: GetAuthor outer res == nil guard
• Verify fix: GetAuthor returns ethereum.NotFound instead of fmt.Errorf
• Verify fix: Block number validation in HeaderByNumber/BlockByNumber
• Verify fix: GetBlockInfoInBatch nil-author break behavior
• Verify: BorGRPCFlag merge logic (acknowledged as-is)
• Check for any new issues in latest commit
• Post final re-review

---

All issues from previous review rounds have been addressed. Here's the full verification of commit a163232:

Verified Fixes (latest commit a163232)

Issue	Status	Location
GetAuthor outer res == nil guard	Fixed	query.go:145 — if res == nil || res.Author == nil
GetAuthor returns ethereum.NotFound	Fixed	query.go:146 — return nil, ethereum.NotFound with clear comment explaining the convention match
Block number validation in HeaderByNumber	Fixed	query.go:69-71 — if blockID >= 0 && h.Number.Int64() != blockID returns descriptive error
Block number validation in BlockByNumber	Fixed	query.go:94-97 — same guard with matching comment
GetBlockInfoInBatch nil-author break	Fixed	query.go:219 — if b.Author == nil && b.Header.Number != 0 { break } with comment explaining HTTP parity
Test updates for nil-author break	Fixed	query_test.go:907,978 — tests now supply non-nil Author for non-genesis blocks, correctly exercising the new break logic

No New Issues Found

The latest commit is clean. The block number validation guards correctly use blockID >= 0 to skip sentinel values (negative block numbers like -1 for latest), the nil-author break matches the HTTP collateBorBatchResults semantics, and the GetAuthor refactor aligns with the convention established across all sibling methods.

Overall Assessment

This PR is ready. Across four review rounds, all security findings (inner-pointer nil panics, malformed header propagation, receipt converter safety, retry config, IPv6 SNI, transport security, block number validation, cross-transport parity) have been thoroughly addressed. The code is well-structured, defensively coded, and has excellent test coverage.

The only remaining item is the pre-existing BorGRPCFlag *bool refactor (acknowledged as out of scope for this PR) and pinning polyproto/bor to tagged releases before merge.

| Branch: mardizzone/grpc_improv | View job

[Copilot]

Pull request overview

Copilot reviewed 25 out of 26 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 25 out of 26 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[claude]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

[Copilot]

Pull request overview

Copilot reviewed 25 out of 26 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 25 out of 26 changed files in this pull request and generated 3 comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[claude]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

[claude]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

[Copilot]

Pull request overview

Copilot reviewed 25 out of 26 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
1 Accepted issue

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

See analysis details on SonarQube Cloud