Fix GasMeter reset in AnteHandler - EthGasConsumeDecorator

[loredanacirstea]

Conforming to the spec:
https://github.com/cosmos/cosmos-sdk/blob/e7066c4271ff3d33dc426dc6313c82a1201ae3c6/docs/basics/gas-fees.md#antehandler

AnteHandler
Set newCtx.GasMeter to 0, with a limit of GasWanted.
This step is extremely important, as it not only makes sure the transaction cannot consume infinite gas,
but also that ctx.GasMeter is reset in-between each DeliverTx
(ctx is set to newCtx after anteHandler is run, and the anteHandler is run each time DeliverTx is called).

Fixes evmos/evmos#311

Description

EthSetupContextDecorator ante handler initializes the gas meter as an infinite gas meter (with gas limit 0) - https://github.com/tharsis/ethermint/blob/7b22a535569f558dc9083e0833915c745c68be43/app/ante/eth.go#L463

This gas meter needs to be reset with the correct gas limit, which accounts for all messages in the transaction. Because then, this value is used to calculate the block's consumed gas, when it is prepared. See:

https://github.com/cosmos/cosmos-sdk/blob/2646b474c7beb0c93d4fafd395ef345f41afc251/baseapp/baseapp.go#L658-L659

https://github.com/tendermint/tendermint/blob/abdf717761e71dca53389a9d218fce8a84f43991/internal/mempool/mempool.go#L365

What was happening was gasWanted was always 0 (infinite gas meter has a limit of 0), so the block that was proposed would contain all the transactions from the mempool found at that time. When the block was applied, at DeliverTx time, all transactions over the block gas limit were not applied (which is correct), but they would have no way to return to the mempool, because they were already processed. Trying to get a transaction receipt for them would just fail with transaction not found.

See these logs: https://gist.github.com/loredanacirstea/a935737b2b1cf3cbc0e679efa1787cd0#file-evmos_logs2-txt-L1464

• We see receipts not found because the block has not been finalized yet: https://gist.github.com/loredanacirstea/a935737b2b1cf3cbc0e679efa1787cd0#file-evmos_logs2-txt-L759, which is normal.
• Block is finalized with 50! transactions: https://gist.github.com/loredanacirstea/a935737b2b1cf3cbc0e679efa1787cd0#file-evmos_logs2-txt-L908
• Immediately after the block is being processed and transactions applied to the state. Now we see a correct calculation of block gas, with each transaction.
• Block gas limit is reached at https://gist.github.com/loredanacirstea/a935737b2b1cf3cbc0e679efa1787cd0#file-evmos_logs2-txt-L1197 and the rest of the transactions included in the block are not processed.
• Receipts for the processed tx are returned https://gist.github.com/loredanacirstea/a935737b2b1cf3cbc0e679efa1787cd0#file-evmos_logs2-txt-L1245, but for the ones that are over the block gas limit (unprocessed), eth_getTransactionReceipt fails https://gist.github.com/loredanacirstea/a935737b2b1cf3cbc0e679efa1787cd0#file-evmos_logs2-txt-L1270.
• Next block is finalized with no data: https://gist.github.com/loredanacirstea/a935737b2b1cf3cbc0e679efa1787cd0#file-evmos_logs2-txt-L1464

---

For contributor use:

• 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.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer

---

For admin use:

• Added appropriate labels to PR (ex. WIP, R4R, docs, etc)
• Reviewers assigned
• Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

[fedekunze]

ACK, can you add a changelog.md entry under bug fixes? 🙏

[fedekunze]

cc: @yihuang @thomas-nguy @tomtau @crypto-facs

[thomas-nguy]

The gas meter is setup in EthSetupContextDecorator , with this approach it becomes redundant

[fedekunze]

@loredanacirstea tests are failing

[codecov]

Codecov Report

Merging #964 (eef0555) into main (805b2ea) will decrease coverage by 0.24%.
The diff coverage is 17.94%.

❗ Current head eef0555 differs from pull request most recent head 0a5cc37. Consider uploading reports for the commit 0a5cc37 to get more accurate results

@@            Coverage Diff             @@
##             main     #964      +/-   ##
==========================================
- Coverage   59.33%   59.09%   -0.25%     
==========================================
  Files          78       79       +1     
  Lines        6468     6508      +40     
==========================================
+ Hits         3838     3846       +8     
- Misses       2412     2444      +32     
  Partials      218      218              

Impacted Files	Coverage Δ
types/gasmeter.go	0.00% <0.00%> (ø)
app/ante/eth.go	81.98% <100.00%> (+0.45%)	⬆️
x/evm/keeper/state_transition.go	75.61% <100.00%> (ø)

[yihuang]

Sorry for the confusion from my comments, but we should do it for deliverTx too, but not for the msg handler, so we should remove the checkTx condition, but override with an infinite gas meter at the beginning of Keeper.EthereumTx method.

[fedekunze]

tests are failing

[loredanacirstea]

The TestOutOfGasWhenDeployContract test is failing because it sets a gas meter with a small gas limit, which gets reset with the infinite gas meter from Keeper. EthereumTx.
https://github.com/tharsis/ethermint/blob/caa1c5a6c6b7ed8ba4aaf6e0b0848f6be5ba6668/x/evm/handler_test.go#L409
@yihuang Should the test be rewritten using some other API or is it a cause for concern?

The rpc tests are failing with the same errors as before. I will take a look in the morning.

[peskypunk]

in case it helps, i am also seeing failures due to some sort of gas metering issue, which i tried to write up here:
https://gist.github.com/peskypunk/8c344c2028dbd319b6a4771e786cfa09

this is coming from the state of ethermint with @loredanacirstea's first 4 commits (which was the state of this PR until this morning)

edit: nvm, after pulling in the other 5 commits on this branch it no longer errors, nice

[yihuang]

The TestOutOfGasWhenDeployContract test is failing because it sets a gas meter with a small gas limit, which gets reset with the infinite gas meter from Keeper. EthereumTx.

https://github.com/tharsis/ethermint/blob/caa1c5a6c6b7ed8ba4aaf6e0b0848f6be5ba6668/x/evm/handler_test.go#L409

@yihuang Should the test be rewritten using some other API or is it a cause for concern?
The rpc tests are failing with the same errors as before. I will take a look in the morning.

I think it should not expect a panic, but a OutOfGas error returned from here: https://github.com/tharsis/ethermint/blob/main/x/evm/keeper/state_transition.go#L383, the error should be able to check in the response, similar to how the VM error is checked in test case TestErrorWhenDeployContract.

[loredanacirstea]

I think it should not expect a panic, but a OutOfGas error returned from here: https://github.com/tharsis/ethermint/blob/main/x/evm/keeper/state_transition.go#L383, the error should be able to check in the response, similar to how the VM error is checked in test case TestErrorWhenDeployContract.

@yihuang , fixed the test here: 892fc66
Turns out that the out of gas error manifests as an ErrIntrinsicGas error from
https://github.com/tharsis/ethermint/blob/f21592ebfe74da7590eb42ed926dae970b2a9a3f/x/evm/keeper/state_transition.go#L365-L369

RPC tests pass now: I moved the gas meter reset in the innermost EthAnteHandler. Previously it was failing because there was not enough gas for cosmos SDK operations (reading balances, creating accounts) that were done after the EthGasConsumeDecorator.

Note: First I tried adding another decorator after EthIncrementSenderSequenceDecorator with a more appropriate name, but the code was not being executed, even though I added it to newEthAnteHandler in ante/handler_options.go). So, then I just used EthIncrementSenderSequenceDecorator

[loredanacirstea]

Do not merge yet. I just saw another problem, even though tests pass.

[loredanacirstea]

@fedekunze , @yihuang
PR ready. The cleanest way to fix the issue was a new type of gas meter that keeps an unenforced gas limit.

(Resetting the gas meter in Keeper.EthereumTx did not work, because the original gas meter was probably used in another context which was causing the final gasUsed amount to be lower than reality).

[fedekunze]

ACK

[yihuang]

LGTM