fix: should revert tx when block gas limit exceeded

[yihuang]

Closes: #10769

Description

Solution:

• create a WithBranchedStore to handle state snapshot and revert
• extract ConsumeBlockGasMiddleware out from RecoveryTxMiddleware.
• order the middlewares properly.

---

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)

[tomtau]

good to back port to 0.45 @robert-zaremba @aaronc @alexanderbez

[alexanderbez]

Also, let's add a few tests for this. Thanks @ethanfrey 🙏

[yihuang]

I've redone the PR in a cleaner way, by creating a SnapshotTxMiddlware outside of RecoveryTxMiddleware.

[amaury1093]

I like this SnapshotMiddleware solution

[yihuang]

I'm not sure how to write a unit test for this though, it seems we need to execute some msg that does storage operations, then we can check the side-effects are reverted when the block gas limit is exceeded.

[yihuang]

I like this SnapshotMiddleware solution

Do you think there's a similar issue with this tip logic: https://github.com/cosmos/cosmos-sdk/blob/master/x/auth/middleware/tips.go#L38 . It's also executed after tx msgs, and it seems to be possible to return error. Maybe we should put it inside the SnapshotTxMiddleware?

[alexanderbez]

I'm not sure how to write a unit test for this though, it seems we need to execute some msg that does storage operations, then we can check the side-effects are reverted when the block gas limit is exceeded.

1. Yes, this exactly how we should test it. In fact, there might already be some BaseApp tests that do something similar.
2. Can we rename the new middleware? Snapshot is not indicative of what is really going on. If anything, you may call it PersistTxMiddleware

[amaury1093]

Do you think there's a similar issue with this tip logic: https://github.com/cosmos/cosmos-sdk/blob/master/x/auth/middleware/tips.go#L38 . It's also executed after tx msgs, and it seems to be possible to return error. Maybe we should put it inside the SnapshotTxMiddleware?

Yes you're right. But with the current code, the TipsMiddleware is already inside SnapshotTxMiddleware (see #10770 (comment)), so all good

[tomtau]

looks good, just a small nit: given it's now renamed PersistTxMiddleware, perhaps the file could be persist_tx.go or persist.go instead of snapshot.go for consistency?

[yihuang]

looks good, just a small nit: given it's now renamed PersistTxMiddleware, perhaps the file could be persist_tx.go or persist.go instead of snapshot.go for consistency?

yeah, good catch, done.

[robert-zaremba]

Here is the followup issue: #10908 . It requires bigger refactor, so we won't be able to handle it in this PR unfortunately.

In this PR: let's add the missing tests and see what we can fix in the middleware we are changing.

[ethanfrey]

I'm looking forward to 0.45 release and this seems like the only blocker left.

Are there concrete actions needed for this? "Missing tests and fix the middleware" are a bit vague.
I have not done a thorough review, as I trust there are plenty of eyes, but if my input could help, let me know. Just want to bump this up. Nice fix to have, but looking forward to 0.45 which brings in some very important other changes

[amaury1093]

@robert-zaremba are you okay to lift the block, and implementing more complex gas deductions/refunds in a later PR?

[fedekunze]

implementing more complex gas deductions/refunds in a later PR

I agree this is a nice addition but outside the scope of the PR. We'd be happy to collaborate on a design

[robert-zaremba]

Thanks for adding tests. I've already created a new issue for other updates discussing here.

[amaury1093]

Nice, adding automerge!

I removed the backport label, there's already #10814