Skip to content

fix(controller): guard against nil account in UpdateTelemetry - #535

Open
erhnysr wants to merge 3 commits into
canopy-network:developmentfrom
erhnysr:fix/update-telemetry-nil-account
Open

fix(controller): guard against nil account in UpdateTelemetry#535
erhnysr wants to merge 3 commits into
canopy-network:developmentfrom
erhnysr:fix/update-telemetry-nil-account

Conversation

@erhnysr

@erhnysr erhnysr commented Aug 21, 2026

Copy link
Copy Markdown

Problem

UpdateTelemetry() in controller/block.go:775 drops the error from GetAccount() and dereferences the returned account directly:

// update account metrics
if a, _ := c.FSM.GetAccount(address); a.Amount != 0 {
	c.Metrics.UpdateAccount(address.String(), a.Amount)
}

GetAccount() returns a nil account together with an error on a store read or unmarshal failure (fsm/account.go:31-35):

bz, err := s.Get(KeyForAccount(address))
if err != nil {
	return nil, err
}
acc, err := s.unmarshalAccount(bz)
if err != nil {
	return nil, err
}

So when the store errors, a is nil and a.Amount panics with a nil pointer dereference. UpdateTelemetry() runs in a defer after the block is committed, so the panic surfaces on the post-commit path.

The core argument is an internal inconsistency: the validator branch immediately above already guards its result, while the account branch does not:

// update validator metric
if v, _ := c.FSM.GetValidator(address); v != nil && v.StakedAmount != 0 {   // guarded
	...
}
// update account metrics
if a, _ := c.FSM.GetAccount(address); a.Amount != 0 {                       // not guarded
	...
}

GetValidator() and GetAccount() share the same (nil, err)-on-failure contract, so the two branches should be guarded the same way.

Fix

A single clause, matching the validator branch directly above:

if a, _ := c.FSM.GetAccount(address); a != nil && a.Amount != 0 {

Returning the error is not an option here: UpdateTelemetry() is a void best-effort telemetry function, and the in-function convention is drop-the-error, guard-the-result (exactly what the validator branch does). Propagating the error would mean changing the signature and every caller for a metrics-only side effect, which is out of scope and inconsistent with the surrounding code.

Tests

New test in controller/block_test.go that injects a store whose Get() always fails, forcing GetAccount() to return (nil, err), then asserts UpdateTelemetry() does not panic. This follows the failing-store injection pattern from merged PR #530, with two deliberate differences:

  • It asserts require.NotPanics rather than fix(fsm): return LoadCommittee errors in ApplyBlock #530's require.ErrorContains, because this symptom is a panic, not a returned error (UpdateTelemetry() returns nothing).
  • I could not reuse fix(fsm): return LoadCommittee errors in ApplyBlock #530's in-package newTestStateMachine, which constructs a StateMachine by setting unexported fields directly (only legal inside the fsm package). From the controller package I used the exported SetStore() plus minimal reflection to allocate the unexported cache field — the same cross-package construction pattern the cmd/rpc tests already use.

Failing first — before the fix (cdfd182)

=== RUN   TestUpdateTelemetryHandlesGetAccountError
    block_test.go:44:
        	Error Trace:	/Users/macbookpro/canopy/controller/block_test.go:44
        	Error:      	func (assert.PanicTestFunc)(0x101493f40) should not panic
        	            		Panic value:	runtime error: invalid memory address or nil pointer dereference
        	            		Panic stack:	goroutine 38 [running]:
        	            	...
        	            	panic({0x10214f9a0?, 0x102377d60?})
        	            		/opt/homebrew/Cellar/go/1.26.3/libexec/src/runtime/panic.go:860 +0x12c
        	            	github.com/canopy-network/canopy/controller.(*Controller).UpdateTelemetry(...)
        	            		/Users/macbookpro/canopy/controller/block.go:775 +0x1ec
        	            	github.com/canopy-network/canopy/controller.TestUpdateTelemetryHandlesGetAccountError.func1()
        	            		/Users/macbookpro/canopy/controller/block_test.go:45 +0x28
        	            	...
        	Test:       	TestUpdateTelemetryHandlesGetAccountError
--- FAIL: TestUpdateTelemetryHandlesGetAccountError (0.00s)
FAIL
FAIL	github.com/canopy-network/canopy/controller	0.650s
FAIL

The panic stack points straight at controller/block.go:775.

Passing — after the fix (6b95794)

=== RUN   TestUpdateTelemetryHandlesGetAccountError
--- PASS: TestUpdateTelemetryHandlesGetAccountError (0.00s)
PASS
ok  	github.com/canopy-network/canopy/controller	0.540s

Full package run:

ok  	github.com/canopy-network/canopy/controller	0.438s

Notes

CONTRIBUTING.md mentions updating .docs/CHANGELOG.md, but that file does not exist in the repository and recent merged PRs (e.g. #494) did not add one, so I did not fabricate a changelog entry.

pablocampogo and others added 3 commits August 28, 2026 19:42
…count

Add a regression test that drives UpdateTelemetry() with a state machine whose
store Get() always fails, forcing GetAccount() to return a nil account with an
error. The test asserts UpdateTelemetry() does not panic; it fails on the
current code because the account metric branch dereferences the nil account.
UpdateTelemetry() dropped the error from GetAccount() and dereferenced the
returned account directly. GetAccount() returns a nil account together with an
error on a store read or unmarshal failure, so the account metric branch could
panic with a nil pointer dereference.

Add a nil check before reading the account amount, matching the validator
metric branch directly above which already guards its result with 'v != nil'.
@erhnysr
erhnysr force-pushed the fix/update-telemetry-nil-account branch from 6b95794 to ed2dbfd Compare August 30, 2026 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants