[R4R] Cache-wrap context during ante handler exec

[alexanderbez]

• Cache-wrap the context (multi-store) during the ante handler execution in runTx. State will only be persisted if the ante handler does fail (abort signaled).
• Made getState a method of BaseApp -- not sure why it wasn't before as we were passing the reference in directly as a parameter.
• Added initial BaseApp documentation.

I believe the existing base app unit tests cover the cases that need to be covered.

closes: #2772

---

• Targeted PR against correct branch (see CONTRIBUTING.md)

• Linked to github-issue with discussion and accepted design OR link to spec that describes this work.

• Wrote tests

• Updated relevant documentation (docs/)

• Added entries in PENDING.md with issue #

• rereviewed Files changed in the github PR explorer

---

For Admin Use:

• Added appropriate labels to PR (ex. wip, ready-for-review, docs)
• Reviewers Assigned
• Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

[codecov]

Codecov Report

Merging #2781 into develop will increase coverage by 0.02%.
The diff coverage is 61.9%.

@@             Coverage Diff             @@
##           develop    #2781      +/-   ##
===========================================
+ Coverage    56.74%   56.77%   +0.02%     
===========================================
  Files          131      131              
  Lines         8564     8545      -19     
===========================================
- Hits          4860     4851       -9     
+ Misses        3371     3363       -8     
+ Partials       333      331       -2

[alessio]

Looks good

[cwgoes]

See comment. Can we add a section to the SDK docs covering baseapp and the expected sequence of transactions through CheckTx / DeliverTx, explaining when ante handler state will be persisted, and explaining that a proposer can bypass CheckTx? All of these are non-obvious to developers (and quite possibly also to us).

(and then link to parts of that spec directly in this code)

[alexanderbez]

@cwgoes I can absolutely do that, I think that is a fantastic idea!

[alexanderbez]

@cwgoes addressed your concerns about state transitions + added BaseApp docs

[cwgoes]

Minor comments, mostly looks good.

[ebuchman]

Some questions and minor comments

[ebuchman]

When should it panic?

[alexanderbez]

During any state transition that doesn't gracefully return an sdk.Result that's also not out of gas.

[ebuchman]

Don't we want to do this for all modes? Otherwise eg invalid txs will change the checktx state

[alexanderbez]

This is the current behavior so I left as-is and I don't see any direct reason to change it, unless you do?

[jaekwon]

Ok so I guess the first arg, 1, means that the ante handler for one of the two above succeeded? But it's not clear which one.

The second 0 means that none of the msgs succeeded, but that wasn't clear either...

[alexanderbez]

I've added explicit store checks via getting the app's state. Lmk if this still isn't clear. This logic is baked into the testing ante handler and message handler, so I'm trying to work with what I already have without making further drastic changes.

[jaekwon]

state transitions that occur during the AnteHandler are persisted even when the following Handler processing logic fails.

It is possible that a malicious proposer may include a transaction in a block that fails CheckTx. In this case (i.e. the AnteHandler processing itself fails during DeliverTx), all state transitions for the offending transaction are discarded.

[alexanderbez]

I'm not sure what you're suggesting here @jaekwon. I'm simply stating ante handler state updates are discarded/reverted if the ante handler fails during deliverTx.

If you want rewording, can you please suggest something?

[jaekwon]

Please address the comments and ping me back!

[jaekwon]

Lol, Ethan and I left more or less the same comments.

[alexanderbez]

@jaekwon your change is failing on TestGaiaCLISendGenerateSignAndBroadcast due to the same reason my attempt failed previously:

ERROR: {"codespace":"sdk","code":3,"message":"Invalid sequence. Got 1, expected 2"}

I think this was the reason why I only did context caching on DeliverTx. Because before this PR we did not persist CheckTx state, only state from running messages.

[cwgoes]

Perhaps we need to change the testcase.