Uninitialized accounts are unmarshalled with invalid data

[zivkovicmilos]

Uninitialized accounts are returned with invalid data

Description

Executing a query to auth/accounts/<address> for a previously unseen address returns a nil response to an ABCI query result. This can lead to an invalid account (zero address) being returned (when unmarshalling this ABCI response on the client).

Your environment

• macOS 13.3
• master (60eeb9f77b6d4268fd11fcd484625624807e824a)

Steps to reproduce

• Generate an address that has not been used with the gno chain before (ex. with gnokey generate / add)
• Query it using the path auth/accounts/<address> from a client (ex. client.ABCIQuery)
• When unmarshalling the results into a gnoland.GnoAccount, the data will not match what was queries (address is zero address)

Expected behaviour

The account data should be returned (and filled correctly) even if the account is not found in the blockchain storage

Actual behaviour

If the account is not found, nil is returned, leading to an invalid unmarshal operation on ABCI clients.

Logs

./build/gnokey query --remote localhost:26657 --height 1 auth/accounts/g1zhrdmurv3yqwpct7s09rgk0l3yj6kgtyf7hd2f
height: 0
data: null

Proposed solution

Account fetches should never return nil, but rather initialize the account and return it.

[zivkovicmilos]

cc @moul

Initializing and saving unknown accounts on query requests can have real side-effects on storage, so I suggest we discuss how we want to go about solving a problem like this

[moul]

Hey, thank you.

@zivkovicmilos: what do you suggest?

Anyone: any feedback on how other chains handle this is a manner you prefer?

[zivkovicmilos]

Hey, thank you.

@zivkovicmilos: what do you suggest?

Anyone: any feedback on how other chains handle this is a manner you prefer?

Not exactly sure what should be the appropriate fix that doesn't have any unwanted side effects.

Ethereum handles these situations (querying of previously untouched accounts) in the following manner:

• accounts are not initialized in the state tree until they need to be (they are part of a state transition)
• when a node does not find the account in its storage (it's "unknown"), it constructs a response for the user with default parameters (non-empty ones, so it fills out the account object as if it were fetched from storage, but note that it doesn't initialize the account in the storage itself)

I honestly see no problem in doing the same on our Tendermint node, the only thing I'm worried about is the Account Number param for the BaseAccount. If I understood correctly, this is dependent on state, meaning the account needs to be initialized already (number secured, so no two accounts have the same one). Is this correct?

[tbruyelle]

I honestly see no problem in doing the same on our Tendermint node, the only thing I'm worried about is the Account Number param for the BaseAccount. If I understood correctly, this is dependent on state, meaning the account needs to be initialized already (number secured, so no two accounts have the same one). Is this correct?

I like the idea of returning a pre-initialized account if the passed address doesn't exist, unfortunately as you mentioned it's not possible due to the account number, which is a global counter incremented each time a new account is created.

The other way of dealing with non-existent account would be to return an error instead of nil, but not sure I prefer that...

[moul]

I suggest we return an AccountNotFound error or panic.

This is a straightforward issue in tm2/pkg/sdk/auth if someone wants to give it a try.