Skip to content

fix(gnogenesis): decode auth/accounts as gnoland.GnoAccount#5647

Merged
moul merged 5 commits into
gnolang:masterfrom
gfanton:fix/gnogenesis-decode-gnoaccount
May 22, 2026
Merged

fix(gnogenesis): decode auth/accounts as gnoland.GnoAccount#5647
moul merged 5 commits into
gnolang:masterfrom
gfanton:fix/gnogenesis-decode-gnoaccount

Conversation

@gfanton
Copy link
Copy Markdown
Member

@gfanton gfanton commented May 11, 2026

Summary

Follow-up to #5511. Addresses one specific case of the "Still open" item:

queryAccountAtHeight silent nil — all error paths return nil with no indication; flaky RPC → wrong sequence metadata.

This isn't flaky RPC. The parse fails deterministically on any real gno.land chain.

The bug

auth/accounts/<addr> is handled by authHandler.queryAccount, which marshals whatever acck.GetAccount returns. On gno.land that's a *gnoland.GnoAccount:

type GnoAccount struct {
    std.BaseAccount
    Attributes BitSet `json:"attributes" yaml:"attributes"`
}

The wire format is therefore {"BaseAccount":{...},"attributes":...}. The previous parser tried two paths:

  1. A wrapper struct containing only BaseAccount — amino's strict-field policy rejects the unknown "attributes".
  2. A bare std.BaseAccount — same problem, plus "BaseAccount" is also unknown to it.

Both error. The function silently returns nil, and the caller in FetchTxs defaults accNum to 0 for the rest of the run. Every SignerInfo entry in the generated hardfork genesis claims the same accNum.

Impact

Affects every hardfork generated with gnogenesis fork generate against a real gno.land chain:

  • If the chain has more than one balance entry and balances[0].Address is not a tx signer, target boot fails loudly via the validateSignerInfo preflight added in feat(gnoland): chain hardfork mechanism v3 #5511: "account number 0 already assigned to X, cannot reassign to Y".
  • If balances[0].Address happens to be the only signer (single-account testbeds), the wrong accNum=0 still propagates into target account state via the SignerInfo force-set in loadAppState, just not loudly. That's silent corruption.

The hf-glue end-to-end run in #5511 didn't surface this because that testbed only validates against rpc.gno.land data; the dominant signer there happened to land at balance index 0.

Fix

Decode the response as gnoland.GnoAccount directly. Same pattern as gnoclient.QueryAccount (gno.land/pkg/gnoclient/client_queries.go:59-65). The package is already imported in this file, so there is no architectural cost. The std.BaseAccount fallback is dead — contribs/gnogenesis is a gno.land tool — and is removed.

7 lines replacing 11.

Discovery

Caught by an end-to-end transition harness driving source → halt → fork generate → target boot → replay-report assertion, against a multi-balance source chain. The handler-side query reproduction is mechanical: any gnogenesis fork generate -source http://... against a gno.land chain with signed historical txs hits this.

AI disclosure

Diagnosed and patched with Claude Code.

The auth/accounts handler marshals whatever acck.GetAccount returns,
which on gno.land is a *gnoland.GnoAccount (std.BaseAccount embedded +
Attributes BitSet). The previous parser only knew bare std.BaseAccount;
amino's strict-field policy rejected the "attributes" field and
queryAccountAtHeight silently returned nil, defaulting accNum to 0 for
every signer. Generated hardfork genesis files therefore claimed accNum=0
for every SignerInfo, which validateSignerInfo rejects when any
balance-init address occupies index 0.

Decode directly into gnoland.GnoAccount, matching gnoclient.QueryAccount.

Follow-up to gnolang#5511 ("queryAccountAtHeight silent nil" follow-up item).

assisted-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Gno2D2
Copy link
Copy Markdown
Collaborator

Gno2D2 commented May 11, 2026

🛠 PR Checks Summary

All Automated Checks passed. ✅

Manual Checks (for Reviewers):
  • IGNORE the bot requirements for this PR (force green CI check)
Read More

🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers.

✅ Automated Checks (for Contributors):

🟢 Maintainers must be able to edit this pull request (more info)

☑️ Contributor Actions:
  1. Fix any issues flagged by automated checks.
  2. Follow the Contributor Checklist to ensure your PR is ready for review.
    • Add new tests, or document why they are unnecessary.
    • Provide clear examples/screenshots, if necessary.
    • Update documentation, if required.
    • Ensure no breaking changes, or include BREAKING CHANGE notes.
    • Link related issues/PRs, where applicable.
☑️ Reviewer Actions:
  1. Complete manual checks for the PR, including the guidelines and additional checks if applicable.
📚 Resources:
Debug
Automated Checks
Maintainers must be able to edit this pull request (more info)

If

🟢 Condition met
└── 🟢 And
    ├── 🟢 The base branch matches this pattern: ^master$
    └── 🟢 The pull request was created from a fork (head branch repo: gfanton/gno)

Then

🟢 Requirement satisfied
└── 🟢 Maintainer can modify this pull request

Manual Checks
**IGNORE** the bot requirements for this PR (force green CI check)

If

🟢 Condition met
└── 🟢 On every pull request

Can be checked by

  • Any user with comment edit permission

@moul moul requested review from D4ryl00 and aeddi May 18, 2026 14:46
Copy link
Copy Markdown
Member

@moul moul left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, but ideally should be unit tested.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 18, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

Comment thread contribs/gnogenesis/internal/fork/source_rpc.go Outdated
Copy link
Copy Markdown
Contributor

@aeddi aeddi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you resolve the conflicts, address the comments then merge this?

gfanton added 2 commits May 22, 2026 17:33
…de-gnoaccount

# Conflicts:
#	contribs/gnogenesis/internal/fork/source_rpc.go
The original fix in source_rpc.go (gnolang#5647) was orphaned by gnolang#5696, which
split that file into source_txs_rpc.go + source_genesis_rpc.go + the
data-dir / jsonl-file txs sources. The buggy decode logic was carried
into source_txs_rpc.go unchanged, so the fix has to land there.

Re-applies the same change in queryAccountAtHeight on rpcTxsSource:
decode the auth/accounts response as gnoland.GnoAccount instead of the
two-path wrapper + bare BaseAccount fallback that amino's strict-field
policy rejects.

Also addresses davd-gzl's review comment by adding WARNING prints when
the ABCI response carries an Error or empty Data, mirroring the existing
warning on the transport-error path.

Adds TestQueryAccountAtHeight_WireFormatContract pinning the contract:
amino-encoded GnoAccount wire bytes round-trip through gnoland.GnoAccount
and break the two previous decoders (regression guard for moul's "ideally
should be unit tested" comment).

Generated by Claude
@moul moul merged commit cd3adeb into gnolang:master May 22, 2026
124 of 125 checks passed
jaekwon pushed a commit that referenced this pull request May 26, 2026
## Summary

Follow-up to #5511. Addresses one specific case of the "Still open"
item:

> `queryAccountAtHeight` silent nil — all error paths return nil with no
indication; flaky RPC → wrong sequence metadata.

This isn't flaky RPC. The parse fails deterministically on any real
gno.land chain.

## The bug

`auth/accounts/<addr>` is handled by `authHandler.queryAccount`, which
marshals whatever `acck.GetAccount` returns. On gno.land that's a
`*gnoland.GnoAccount`:

```go
type GnoAccount struct {
    std.BaseAccount
    Attributes BitSet `json:"attributes" yaml:"attributes"`
}
```

The wire format is therefore `{"BaseAccount":{...},"attributes":...}`.
The previous parser tried two paths:

1. A wrapper struct containing only `BaseAccount` — amino's strict-field
policy rejects the unknown `"attributes"`.
2. A bare `std.BaseAccount` — same problem, plus `"BaseAccount"` is also
unknown to it.

Both error. The function silently returns nil, and the caller in
`FetchTxs` defaults `accNum` to `0` for the rest of the run. Every
`SignerInfo` entry in the generated hardfork genesis claims the same
accNum.

## Impact

Affects every hardfork generated with `gnogenesis fork generate` against
a real gno.land chain:

- If the chain has more than one balance entry and `balances[0].Address`
is not a tx signer, target boot fails loudly via the
`validateSignerInfo` preflight added in #5511: *"account number 0
already assigned to X, cannot reassign to Y"*.
- If `balances[0].Address` happens to be the only signer (single-account
testbeds), the wrong `accNum=0` still propagates into target account
state via the `SignerInfo` force-set in `loadAppState`, just not loudly.
That's silent corruption.

The hf-glue end-to-end run in #5511 didn't surface this because that
testbed only validates against `rpc.gno.land` data; the dominant signer
there happened to land at balance index 0.

## Fix

Decode the response as `gnoland.GnoAccount` directly. Same pattern as
`gnoclient.QueryAccount`
(`gno.land/pkg/gnoclient/client_queries.go:59-65`). The package is
already imported in this file, so there is no architectural cost. The
`std.BaseAccount` fallback is dead — `contribs/gnogenesis` is a gno.land
tool — and is removed.

7 lines replacing 11.

## Discovery

Caught by an end-to-end transition harness driving source → halt → `fork
generate` → target boot → replay-report assertion, against a
multi-balance source chain. The handler-side query reproduction is
mechanical: any `gnogenesis fork generate -source http://...` against a
gno.land chain with signed historical txs hits this.

## AI disclosure

Diagnosed and patched with Claude Code.

---------

Co-authored-by: moul <94029+moul@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

6 participants