fix(fsm): recover account address from state key in bulk queries - #606
Open
infoboy27 wants to merge 1 commit into
Open
fix(fsm): recover account address from state key in bulk queries#606infoboy27 wants to merge 1 commit into
infoboy27 wants to merge 1 commit into
Conversation
/v1/query/accounts returns "address": "" for accounts whose stored value does not carry an address, while /v1/query/account resolves the same accounts correctly. The Explorer accounts page shows N/A as a result. Accounts are keyed by address (KeyForAccount), so the key is authoritative, but GetAccounts and GetAccountsPaginated read only the iterator value and discard the key. Records written without the address in the value therefore report empty. GetAccount is unaffected because it sets acc.Address from the requested address. Observed live on a devnet, where both cases appear in one response: eight untouched genesis accounts return an empty address, while the validator account - re-marshalled every block by reward crediting, so its value does carry the address - renders correctly through the same code path. Those legacy bytes persist until the account transacts, so the read path has to tolerate them. Derive the address from the key via AddressFromKey when the unmarshalled value has none. Note that slicing the key manually is incorrect here: lib.JoinLenPrefix writes a length byte per segment, so key[len(AccountPrefix()):] is off by one byte. Read-only change. The only callers are ExportState (RPC state export and a debug logger) and the /v1/query/accounts handler - no state writes and no hashing, so no consensus impact. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Symptom
POST /v1/query/accounts(the paginated/bulk query) returns an empty address for some accounts, whilePOST /v1/query/accountresolves the same accounts correctly. Balances are correct in both. The Explorer accounts page renders Address as N/A.Reproducible unauthenticated against a devnet node:
Root cause
Both cases appear in a single response, through one code path — which is what identifies the cause:
Accounts are keyed by address (
KeyForAccount→lib.JoinLenPrefix(accountPrefix, addr)), so the key is authoritative. But both bulk getters read only the iterator value and discard the key:GetAccounts—s.unmarshalAccount(it.Value()),it.Key()unused.GetAccountsPaginated— thepage.Loadcallback isfunc(_, b []byte); the key goes to_.Records whose stored value carries no
Addresstherefore report empty.GetAccountis unaffected because it setsacc.Addressfrom the requested address.Entry
[8]is the validator: reward crediting re-marshals it every block viaSetAccount, andmarshalAccountislib.Marshal(account)— the whole struct, address included. So its value does carry the address and it renders correctly. Entries[0..7]have never transacted and still hold their original bytes.Two implications worth noting:
spendableAccountView(cmd/rpc/query.go:784) copiesAddressfaithfully, as[8]proves. Separately, the reason it renders as""rather than being omitted is thatAccountView.Address(cmd/rpc/types.go:176) has noomitemptywhilelib.HexBytes.MarshalJSONalways emits a string — cosmetic, and left alone here.Fix
In both bulk getters, when the unmarshalled value has no address, derive it from the state key via
AddressFromKey(fsm/key.go:114). Guarded bylen(acc.Address) == 0, so accounts whose value already carries an address take the existing path untouched.Flagging for review: slicing the key manually is incorrect here.
lib.JoinLenPrefix(lib/util.go:807) writes a length byte per segment, sokey[len(AccountPrefix()):]yields an address shifted by one byte.AddressFromKeydecodes the length prefixes properly and takes the last segment.Consensus impact: none
The only callers are read-only:
cmd/rpc/query.go:80/v1/query/accountshandlerfsm/genesis.go:168(ExportState)cmd/rpc/query.go:507,525,530(RPC state export/diff) andcontroller/block.go:825(debugDumpHeaderDiff, logging only)No state writes, no re-marshalling back into state, no participation in block hashing.
Verification
TestGetAccountsRecoversAddressFromKeywrites an account record whose marshalled value omits the address (reproducing the legacy on-disk shape), then asserts both bulk getters report it. It fails before the fix (actual: []byte(nil), matching the live"") and passes after.go test -count=1 ./fsm/green.Two pre-existing environment notes, unrelated to this change — each verified by
git stashing the patch and reproducing the identical failure on the untouched tree:GOTOOLCHAIN=go1.26.0). Under Go 1.27 the transitive dependencycockroachdb/swissfails to compile (undefined: getRuntimeHasher,fastrand64) as it reaches runtime internals viago:linkname.go build ./...andgo test ./cmd/rpc/both fail withcmd/rpc/server.go:343:12: pattern all:web/explorer/dist: no matching files foundunless the web frontend is built first, so a full-repo build isn't usable as a check in a bare clone.Optional follow-up (deliberately not in this PR)
The empty-address records are a data artifact that persists for any account that never transacts. If you would rather normalise state than tolerate it on read,
SetAccountalready writes the address, so a one-off migration rewriting affected accounts would clear it — but that is a state write and would need consensus-safe sequencing. Happy to follow that route instead if you prefer.