Accessing the state at height == 0 fails and throws errors

[ttl33]

Summary of Bug

One issue that we are noticing when upgrading to v0.47.0 is that accessing IAVL tree store at height 0 returns an error. The bad side effects for this is that RPC calls at height 0 will start resulting in errors.

Similarly in PrepareProposalHandler, when accessing the state via the context throws a runtime error and halts the chain.

IIUC, the state is not ready at height 0. Or perhaps, height 0 is reserved for the app to get initialized.

Previously, this did not cause an error because the default behavior for accessing the state at height == 0 was to return an empty state. But this changed as of this PR

It seems like there are paths that trigger the app to access the state at height 0 and this access should not happen. If so, should there be a better handling of such paths? For example, adding "if height == 0, this access should be rejected, rather than throwing an error".

Shoutout to Lucas for debugging this. He discovered:

Error is being returned when creating the context for the query. It seems that the IAVL tree does not exist at height 0, so the height hasn't been created yet (here is the VersionExists method on the IAVL struct).
Our previous Cosmos SDK version previously returned an empty IAVL tree in this scenario. PR that introduced the change is here.

Version

v0.47.0-alpha2

Steps to Reproduce

For the RPC path, steps to reproduce:

1. have a thread that ping some RPC endpoint that accesses IAVL store repeatedly
2. start the node
3. observe error logs

For the PrepareProposal path,

1. in PrepareProposalHandler call a method that accesses state via ctx
   Note that this might not crash the node after adding panic recovery

[alexanderbez]

So am I correct in understanding that Tendermint is calling PrepareProposal with a RequestPrepareProposal struct with height=0?

Versions in the SDK and by proxy Tendermint, start at height 1 (where Genesis is considered 0 but not explicitly saved under 0).

Do you see InitGenesis being called before PrepareProposal?

[tac0turtle]

We need to handle 0 height in the store, it seems this has come up else where as well

[facundomedica]

We could return an empty tree on height 0 and an error whenever height != 0 (and if it doesn't exists; although I don't think there's a case in which a tree will be available when height == 0).

[facundomedica]

So we are not passing the block height to prepareProposal https://github.com/cosmos/cosmos-sdk/blob/main/baseapp/abci.go#L250
On ProcessProposal we do WithBlockHeight(req.Height). but on PrepareProposal we don't (This was pointed out by @AmauryM during the audit)

This might solve the issue of PrepareProposal crashing/rejecting when there's state access.

EDIT: I still need to figure out how to test this. Although @thtl33 if you want to test it in the meantime, you can add that and test if you can produce blocks. This is not related to RPC queries failing tho, that sounds like a separate issue

[ttl33]

RE: @alexanderbez

So am I correct in understanding that Tendermint is calling PrepareProposal with a RequestPrepareProposal struct with height=0?

Yes, printing out ctx.BlockHeight() shows up as 0. This could be from the field not being set or the height being 0. It's unclear which is the case.

Do you see InitGenesis being called before PrepareProposal?

Yes, I see InitGenesis being called before PrepareProposal.

[ttl33]

RE: @facundomedica

So we are not passing the block height to prepareProposal https://github.com/cosmos/cosmos-sdk/blob/main/baseapp/abci.go#L250. On ProcessProposal we do WithBlockHeight(req.Height). but on PrepareProposal we don't (This was pointed out by @AmauryM during the audit)

I think you can do ctx.BlockHeight() to get the height in PrepareProposal.

[ttl33]

One way to test this is to:

1. Add/find some keeper func that accesses state
2. In PrepareProposalHandler (ie. no-op handler), modify the impl so that the handler uses the func mentioned above to trigger state access