fix(rpc): guard nil height deref in CometBlockResultByNumber

[Aboudjem]

Fixes #1066.

Was tracing an intermittent crash on a pruned node that only showed up under load. The stack trace pointed at comet.go:71 in the error path of CometBlockResultByNumber. When the caller passes height=0, the pointer gets set to nil before the BlockResults RPC call. If that call fails (timeout, pruned state, etc), the error formatting dereferences the nil pointer and panics.

The fix captures the height value before the nil reassignment and uses it in the error message. The variable already existed for the tracing span attribute so this just reuses it in the format string.

Added a table-driven test covering the zero-height error path to confirm it no longer panics.

[greptile-apps]

Greptile Summary

This PR fixes an intermittent nil pointer dereference panic in CometBlockResultByNumber that could occur on pruned nodes under load. The panic was triggered when a caller passed height=0 — which gets remapped to nil before the BlockResults RPC call — and that call subsequently failed; the original error format string dereferenced the now-nil height pointer.

Key changes:

• rpc/backend/comet.go: Replace *height with the already-captured heightAttr local variable in the BlockResults error format string. heightAttr is set before the nil reassignment and is always safe to dereference.
• rpc/backend/comet_test.go: New table-driven test covering nil, zero, and non-zero height inputs with both success and error returns from BlockResults, confirming the function no longer panics on the zero-height error path.

Minor issues found:

• The test struct includes an expectPanic field that is declared but never read or used in any test case assertion, making it dead code that could confuse future maintainers.
• When height is nil (fetch latest) and BlockResults fails, heightAttr is 0, making the error message "failed to fetch block result from CometBFT 0: ..." ambiguous — it looks identical to an explicit request for block 0. This is a pre-existing ambiguity left untouched by the fix.

Confidence Score: 5/5

• This PR is safe to merge; it is a minimal, targeted bug fix with no behavior changes outside the error path.
• The change is a single-line fix replacing an unsafe pointer dereference with a pre-captured integer value that already existed in scope. The logic is straightforward and the new test directly validates the previously crashing code path. No existing behavior is altered on the success path.
• No files require special attention beyond the minor unused expectPanic field in comet_test.go.

Important Files Changed

Filename	Overview
rpc/backend/comet.go	One-line fix replacing *height with the already-captured heightAttr in the error format string, correctly preventing a nil pointer dereference when height=0 is remapped to nil before the BlockResults call fails.
rpc/backend/comet_test.go	New table-driven test covering the nil/zero-height error paths; correctly validates no panic occurs. Minor issue: the expectPanic field in the test struct is declared but never used or set to true in any case, making it dead code.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["CometBlockResultByNumber(ctx, height *int64)"] --> B{height != nil?}
    B -- "yes" --> C["heightAttr = *height"]
    B -- "no" --> D["heightAttr = 0 (default)"]
    C --> E["Start tracing span with heightAttr"]
    D --> E
    E --> F{height != nil && *height == 0?}
    F -- "yes (zero remapped)" --> G["height = nil"]
    F -- "no" --> H["height unchanged"]
    G --> I["RPCClient.BlockResults(ctx, nil)"]
    H --> I
    I --> J{err != nil?}
    J -- "yes" --> K["❌ BEFORE: fmt.Errorf(..., *height, err)\n💥 PANIC if height is nil"]
    J -- "yes" --> L["✅ AFTER: fmt.Errorf(..., heightAttr, err)\n✔ Safe — heightAttr is always int64"]
    J -- "no" --> M["return res, nil"]
    style K fill:#ffcccc,stroke:#cc0000
    style L fill:#ccffcc,stroke:#009900

Loading

_{Last reviewed commit: 3c077ef}

[greptile-apps]

Unused struct field expectPanic

The expectPanic field is declared in the test case struct but is never read or used anywhere in the test body. All cases rely on require.NotPanics to assert no panic, and none of the test cases even set expectPanic: true. This dead field is misleading — a future maintainer might assume it drives some assertion when it does not.

Either remove the field or wire it up to an actual require.Panics / require.NotPanics branch:

Suggested change

	expectPanic bool
	expectErr bool
	testCases := []struct {
	name string
	height *int64
	rpcErr error
	expectErr bool
	}{

[greptile-apps]

Ambiguous height in error message when caller passes nil

When CometBlockResultByNumber is called with a nil height (i.e., "fetch latest block"), heightAttr defaults to 0. If BlockResults then fails, the error message will read "failed to fetch block result from CometBFT 0: ...", which is indistinguishable from a caller that explicitly requested block 0 (which is also remapped to nil). This is a pre-existing ambiguity but the fix keeps it intact.

Consider using a more descriptive format string to differentiate the two cases, for example:

heightStr := "latest"
if heightAttr != 0 {
    heightStr = fmt.Sprintf("%d", heightAttr)
}
return nil, fmt.Errorf("failed to fetch block result from CometBFT (height=%s): %w", heightStr, err)

This is a minor style concern and does not affect correctness.

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}