fix: remove setting of finalizeBlockState in FinalizeBlock + fix initialHeight + add ProcessProposal in tests/sims

[facundomedica]

Description

This is problem because if for some reason FinalizeBlock gets called without first calling ProcessProposal/InitChain a faulty block might be created (using old state probably) or some other undefined behavior.
So with this change if FinalizeBlock gets called in an unexpected way, it should panic given that there is no initialized state to read/write on.

Fixes some of the issues outlined here: #16796
A follow up PR is needed to fix integration tests (working on it now)

---

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

• included the correct type prefix in the PR title
• added ! to the type prefix if API or client breaking change
• targeted the correct branch (see PR Targeting)
• provided a link to the relevant issue or specification
• followed the guidelines for building modules
• included the necessary unit and integration tests
• added a changelog entry to CHANGELOG.md
• included comments for documenting Go code
• updated the relevant documentation or specification
• reviewed "Files changed" and left comments if necessary
• confirmed all CI checks have passed

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

• confirmed the correct type prefix in the PR title
• confirmed ! in the type prefix if API or client breaking change
• confirmed all author checklist items have been addressed
• reviewed state machine logic
• reviewed API design and naming
• reviewed documentation is accurate
• reviewed tests and test coverage
• manually tested (if applicable)

[facundomedica]

There's a test failing because finalizeBlockState is nil. So that means there is a test that calls FinalizeBlock without initializing finalizeBlockState first?
There are 2 options:

• we initialize in InitChain (height == initialHeight)
• we initialize in ProcessProposal (height > initialHeight)

So we are calling FinalizeBlock without executing either of those 🤔

[alexanderbez]

ProcessProposal sets the finalize block state and then its cleared on Commit

[alexanderbez]

Seems that ProcessProposal does not set finalizeBlock state..which I swear it did 🤔

[alexanderbez]

utACK. Changes make sense to me. The setting of finalizeBlockState is set on ProcessProposal and thus should not be in FinalizeBlock. Good work

[alexanderbez]

I would put this in the BaseApp constructor, not here.

Also, can you describe what issue this was causing?

[facundomedica]

I don't think we can do it in the constructor, we have to get it in InitChain which contains InitialHeight (the line above app.initialHeight = req.InitialHeight).

The issue here is the following:

1. InitChain is called with InitialHeight of 0 (according to CometBFT it 0 == 1 in this context) and app.initialHeight is set to 0.
2. After that ProcessProposal gets called with Height 1 (calling it with height 0 is invalid; we have that check in place)
3. getContextForProposal is called with height 1. Then height != app.initialHeight (1 != 0), so it doesn't return the right context (the one that contains all the state written during InitChain).

Writing this makes me doubt if PrepareProposal/ProcessProposal can actually get called with height 0, AFAIK no, but I'm checking Comet's code. Do you know this @alexanderbez ?

See getContextForProposal

[alexanderbez]

PrepareProposal/ProcessProposal can actually get called with height 0

This should never be the case I'd imagine, but either way, yeah, let's keep it here.

[facundomedica]

merging this to continue working on it in a separate PR