Not really part of this PR, but in line 824, below, it states that either Commit completed and last_block_height is now H or Commit failed and last_block_height is still H-1, but even if Commit crashed midway, it may have crashed after the advancing to H. So the restriction should be last_block_height and last_block_app_hash agree, not that they are agree on H-1.

Suggestion

If the app successfully committed block `H`, then `last_block_height = H` and `last_block_app_hash = <hash returned by Commit for block H>`. 
If the app failed during the Commit of block `H`, then either 
- `last_block_height = H-1` and `last_block_app_hash = <hash returned by Commit for block H-1, which is the hash in the header of block H>`; or, 
- `last_block_height = H` and `last_block_app_hash = <hash returned by Commit for block H>`. 

I would, first, refer that last_block_height and last_block_app_hash are the fields returned by the Info method (https://github.com/cometbft/cometbft/blob/7c7b48039453aa66fd6b128af6701651e1398319/spec/abci/abci%2B%2B_methods.md). This because it was not clear to me, and these variables are only used here on the text.

Second, is it not possible to have one of the fields updated (to H) but the other not?

Third, I agree with Lasaro here that the app could crash during the Commit of height H and it will have the two fields updated to H state, although Comet will not be aware of this because the crash occurs before returning the control to Comet.

Is the value assigned mid-commit before the crash persisted? As in, is the state not updated atomically (or at least saved)?

I hope #493 can help us.

Is the value assigned mid-commit before the crash persisted? As in, is the state not updated atomically (or at least saved)?

It must be the case that both values are updated during commit, very likely as the the last steps, and the problem is ensuring that both updates are part of one atomic operation, probably along with putting the final nail on the old's state coffin. Since we don't control that, as it happens on the app side, we must require the app to implement this behavior.

The application typically uses something like leveldb. Correct me if I am wrong, but this kind of databases do not support transactions. So, while both writes are probably atomic, if a crash occurs between them the state can become inconsistent.

The main invariant is that the App state is always committed after consensus persists the results (returned by the app) of executing the block.
So the app and consensus "persist" actions don't need to be atomic. Upon recovery, the logic described later on in this section is used to replay anything the app has "forgotten" (this is figured out with the handshake after recovery, using Info method).

I would, first, refer that last_block_height and last_block_app_hash are the fields returned by the Info method (https://github.com/cometbft/cometbft/blob/7c7b48039453aa66fd6b128af6701651e1398319/spec/abci/abci%2B%2B_methods.md). This because it was not clear to me, and these variables are only used here on the text.

Second, is it not possible to have one of the fields updated (to H) but the other not?

Third, I agree with Lasaro here that the app could crash during the Commit of height H and it will have the two fields updated to H state, although Comet will not be aware of this because the crash occurs before returning the control to Comet.

APP: Execute block Persist application
/ return ResultFinalizeBlock / state
/ / |
Event: -------------- block_stored -----------/-------------state_stored -------------/---app_persisted_state
| / | /
CometBFT: Decide ---- Persist ----------- Call ------------- Persist ------------ Call Commit----
on Block block in the FinalizeBlock results
blockstore (txResults, validator updated,..)

This is rendered weirdly :) I got the suggestion I think, but please confirm :)