Issues with current implementation of ABCI++

[facundomedica]

Summary of Bug

On logic:

• State in FinalizeBlock is being reset every time (we must ALWAYS get a ready-to-use state in FinalizeBlock, otherwise something went wrong and we didn't call InitChain or ProcessProposal). Necessary because ProcessProposal isn't called on replay.
• If we receive a RequestInitChain with height 0, we should then store 1 as app.InitialHeight; otherwise we'll get the wrong context in ProcessProposal (which must never get height 0). In other words: getContextForProposal, doesn’t return the correct context when height == 0. Also this causes an undesired re-set of state in ProcessProposal

On tests:

• On many tests ProcessProposal is not being called, we call FinalizeBlock directly which is not compatible with ABCI++. Tests are not failing because we are setting state if it's nil, we must remove this.
• Commit during simulations is optional, why? This makes the height check fail for FinalizeBlock height (validateFinalizeBlockHeight) if no commit occurred
• Operations in the simulator don't run inside a block (in FinalizeBlock) they run after it.
• WithAutomaticFinalizeBlock shouldn't exist as any msg MUST run inside it instead of after it (integration tests: RunMsg should turn the msg into txbytes somehow ???)

Trying to solve some of these here: #16794

Running TestAppStateDeterminism will fail when we remove the (supposedly unnecessary) initialization of finalizeBlockState

I think this happens because SimulateFromSeed calls FinalizeBlock without calling ProcessProposal first.

[facundomedica]

This will be partially resolved by #16898 . I would like to dig deeper into tests, as I think they don't fully reflect what happens with CometBFT calls. Maybe it's ok, but would like to double-check

[hacheigriega]

I'm experiencing an issue during simulation testing where messages are delivered correctly using app.SimDeliver(), but their future operations don't detect the state changes by these messages. Do you think this problem could be relevant to this? I'm on SDK v0.50.3.

[alexanderbez]

@hacheigriega are you calling FinalizeBlock and Commit? If not, I would tweak your implementation to use those methods instead.

[hacheigriega]

Yes, BaseApp's SimDeliver() calls runTx with execModeFinalize, and I checked that Commit is called by the simulation code at every block. It's particularly strange that the SDK modules' simulation operations seem to go through just fine. I can also check using keeper methods immediately after SimDeliver() that the store does contain the new data. However, a few blocks later when the future operation is run, the chain state doesn't have this data anymore.

My code if you have time to take a gander: https://github.com/sedaprotocol/seda-chain/blob/8a3e964098f16b37582dfa14ca0d00652c525972/x/vesting/simulation/operations.go#L122

Thank you.

[hacheigriega]

Actually, it seems the states are not being persisted for SDK's default module simulations, either.

[alexanderbez]

Sorry, but what is "default module simulations"? Can you provide a link?

[hacheigriega]

Ah yes I wasn't clear. I was looking specifically at SimulateMsgSubmitProposal in gov

cosmos-sdk/x/gov/simulation/operations.go

Line 269 in 7e6948f

_, _, err = app.SimDeliver(txGen.TxEncoder(), tx)

Usually its future operation operationSimulateMsgVote doesn't get triggered, and when it does, a proposal that had been set in SimulateMsgSubmitProposal is not there anymore.

[alexanderbez]

For changes to persist, you should ensure you're calling Commit as well

[facundomedica]

All of these issues have been fixed, or are not issues anymore 👌