From 6ff8739d9f13770e861f3dbf345ef3e7acc5afdb Mon Sep 17 00:00:00 2001 From: Oleg Nikonychev Date: Fri, 1 Nov 2024 21:56:36 +0400 Subject: [PATCH] fix: gas used discrepancy in estimate gas vs actual execution gas --- evm-e2e/.gitignore | 4 +++ x/evm/keeper/call_contract.go | 30 ++++++++----------- x/evm/keeper/erc20.go | 20 ++++++------- x/evm/keeper/keeper.go | 34 +++++++++++++++++---- x/evm/keeper/msg_server.go | 56 +++++++++++++++++++++-------------- x/evm/precompile/funtoken.go | 25 +++++++++++----- 6 files changed, 106 insertions(+), 63 deletions(-) create mode 100644 evm-e2e/.gitignore diff --git a/evm-e2e/.gitignore b/evm-e2e/.gitignore new file mode 100644 index 000000000..3419dc450 --- /dev/null +++ b/evm-e2e/.gitignore @@ -0,0 +1,4 @@ +types +artifacts +cache +.env diff --git a/x/evm/keeper/call_contract.go b/x/evm/keeper/call_contract.go index 1a89eaf51..2b494873c 100644 --- a/x/evm/keeper/call_contract.go +++ b/x/evm/keeper/call_contract.go @@ -71,11 +71,7 @@ func (k Keeper) CallContractWithInput( ) (evmResp *evm.MsgEthereumTxResponse, evmObj *vm.EVM, err error) { // This is a `defer` pattern to add behavior that runs in the case that the // error is non-nil, creating a concise way to add extra information. - defer func() { - if err != nil { - err = fmt.Errorf("CallContractError: %w", err) - } - }() + defer HandleOutOfGasPanic(&err, "CallContractError") nonce := k.GetAccNonce(ctx, fromAcc) unusedBigInt := big.NewInt(0) @@ -108,11 +104,8 @@ func (k Keeper) CallContractWithInput( // sent by a user txConfig := k.TxConfig(ctx, gethcommon.BigToHash(big.NewInt(0))) - // Using tmp context to not modify the state in case of evm revert - tmpCtx, commitCtx := ctx.CacheContext() - evmResp, evmObj, err = k.ApplyEvmMsg( - tmpCtx, evmMsg, evm.NewNoOpTracer(), commit, evmCfg, txConfig, true, + ctx, evmMsg, evm.NewNoOpTracer(), commit, evmCfg, txConfig, true, ) if err != nil { // We don't know the actual gas used, so consuming the gas limit @@ -135,20 +128,21 @@ func (k Keeper) CallContractWithInput( } else { // Success, committing the state to ctx if commit { - commitCtx() - totalGasUsed, err := k.AddToBlockGasUsed(ctx, evmResp.GasUsed) + blockGasUsed, err := k.AddToBlockGasUsed(ctx, evmResp.GasUsed) if err != nil { k.ResetGasMeterAndConsumeGas(ctx, ctx.GasMeter().Limit()) return nil, nil, errors.Wrap(err, "error adding transient gas used to block") } - k.ResetGasMeterAndConsumeGas(ctx, totalGasUsed) + k.ResetGasMeterAndConsumeGas(ctx, blockGasUsed) k.updateBlockBloom(ctx, evmResp, uint64(txConfig.LogIndex)) - err = k.EmitEthereumTxEvents(ctx, contract, gethcore.LegacyTxType, evmMsg, evmResp) - if err != nil { - return nil, nil, errors.Wrap(err, "error emitting ethereum tx events") - } - blockTxIdx := uint64(txConfig.TxIndex) + 1 - k.EvmState.BlockTxIndex.Set(ctx, blockTxIdx) + // TODO: remove after migrating logs + //err = k.EmitLogEvents(ctx, evmResp) + //if err != nil { + // return nil, nil, errors.Wrap(err, "error emitting tx logs") + //} + + //blockTxIdx := uint64(txConfig.TxIndex) + 1 + //k.EvmState.BlockTxIndex.Set(ctx, blockTxIdx) } return evmResp, evmObj, nil } diff --git a/x/evm/keeper/erc20.go b/x/evm/keeper/erc20.go index a37173c49..c430a3bb5 100644 --- a/x/evm/keeper/erc20.go +++ b/x/evm/keeper/erc20.go @@ -81,37 +81,37 @@ Transfer implements "ERC20.transfer" func (e erc20Calls) Transfer( contract, from, to gethcommon.Address, amount *big.Int, ctx sdk.Context, -) (balanceIncrease *big.Int, err error) { +) (balanceIncrease *big.Int, resp *evm.MsgEthereumTxResponse, err error) { recipientBalanceBefore, err := e.BalanceOf(contract, to, ctx) if err != nil { - return balanceIncrease, errors.Wrap(err, "failed to retrieve recipient balance") + return balanceIncrease, nil, errors.Wrap(err, "failed to retrieve recipient balance") } input, err := e.ABI.Pack("transfer", to, amount) if err != nil { - return balanceIncrease, fmt.Errorf("failed to pack ABI args: %w", err) + return balanceIncrease, nil, fmt.Errorf("failed to pack ABI args: %w", err) } - resp, _, err := e.CallContractWithInput(ctx, from, &contract, true, input, Erc20GasLimitExecute) + resp, _, err = e.CallContractWithInput(ctx, from, &contract, true, input, Erc20GasLimitExecute) if err != nil { - return balanceIncrease, err + return balanceIncrease, nil, err } var erc20Bool ERC20Bool err = e.ABI.UnpackIntoInterface(&erc20Bool, "transfer", resp.Ret) if err != nil { - return balanceIncrease, err + return balanceIncrease, nil, err } // Handle the case of success=false: https://github.com/NibiruChain/nibiru/issues/2080 success := erc20Bool.Value if !success { - return balanceIncrease, fmt.Errorf("transfer executed but returned success=false") + return balanceIncrease, nil, fmt.Errorf("transfer executed but returned success=false") } recipientBalanceAfter, err := e.BalanceOf(contract, to, ctx) if err != nil { - return balanceIncrease, errors.Wrap(err, "failed to retrieve recipient balance") + return balanceIncrease, nil, errors.Wrap(err, "failed to retrieve recipient balance") } balanceIncrease = new(big.Int).Sub(recipientBalanceAfter, recipientBalanceBefore) @@ -121,13 +121,13 @@ func (e erc20Calls) Transfer( // the call "amount". Instead, verify that the recipient got tokens and // return the amount. if balanceIncrease.Sign() <= 0 { - return balanceIncrease, fmt.Errorf( + return balanceIncrease, nil, fmt.Errorf( "amount of ERC20 tokens received MUST be positive: the balance of recipient %s would've changed by %v for token %s", to.Hex(), balanceIncrease.String(), contract.Hex(), ) } - return balanceIncrease, err + return balanceIncrease, resp, err } // BalanceOf retrieves the balance of an ERC20 token for a specific account. diff --git a/x/evm/keeper/keeper.go b/x/evm/keeper/keeper.go index 1fb352fe3..84c023339 100644 --- a/x/evm/keeper/keeper.go +++ b/x/evm/keeper/keeper.go @@ -2,6 +2,7 @@ package keeper import ( + "fmt" "math/big" "cosmossdk.io/math" @@ -101,13 +102,19 @@ func (k Keeper) EthChainID(ctx sdk.Context) *big.Int { // block tx. func (k *Keeper) AddToBlockGasUsed( ctx sdk.Context, gasUsed uint64, -) (uint64, error) { - result := k.EvmState.BlockGasUsed.GetOr(ctx, 0) + gasUsed - if result < gasUsed { +) (blockGasUsed uint64, err error) { + + // Either k.EvmState.BlockGasUsed.GetOr() or k.EvmState.BlockGasUsed.Set() + // also consume gas and could panic. + defer HandleOutOfGasPanic(&err, "") + + blockGasUsed = k.EvmState.BlockGasUsed.GetOr(ctx, 0) + gasUsed + if blockGasUsed < gasUsed { return 0, sdkerrors.Wrap(evm.ErrGasOverflow, "transient gas used") } - k.EvmState.BlockGasUsed.Set(ctx, result) - return result, nil + k.EvmState.BlockGasUsed.Set(ctx, blockGasUsed) + + return blockGasUsed, nil } // GetMinGasUsedMultiplier - value from 0 to 1 @@ -143,3 +150,20 @@ func (k Keeper) Tracer( ) vm.EVMLogger { return evm.NewTracer(k.tracer, msg, ethCfg, ctx.BlockHeight()) } + +// HandleOutOfGasPanic gracefully captures "out of gas" panic and just sets the value to err +func HandleOutOfGasPanic(err *error, format string) func() { + return func() { + if r := recover(); r != nil { + switch r.(type) { + case sdk.ErrorOutOfGas: + *err = vm.ErrOutOfGas + default: + panic(r) + } + } + if err != nil && format != "" { + *err = fmt.Errorf("%s: %w", format, *err) + } + } +} diff --git a/x/evm/keeper/msg_server.go b/x/evm/keeper/msg_server.go index 37e508c55..5feae14c9 100644 --- a/x/evm/keeper/msg_server.go +++ b/x/evm/keeper/msg_server.go @@ -64,6 +64,7 @@ func (k *Keeper) EthereumTx( // pass true to commit the StateDB evmResp, _, err = k.ApplyEvmMsg(tmpCtx, evmMsg, nil, true, evmConfig, txConfig, false) + if err != nil { // when a transaction contains multiple msg, as long as one of the msg fails // all gas will be deducted. so is not msg.Gas() @@ -74,32 +75,35 @@ func (k *Keeper) EthereumTx( if !evmResp.Failed() { commitCtx() } + k.updateBlockBloom(ctx, evmResp, uint64(txConfig.LogIndex)) + + blockGasUsed, err := k.AddToBlockGasUsed(ctx, evmResp.GasUsed) + if err != nil { + return nil, errors.Wrap(err, "error adding transient gas used to block") + } // refund gas in order to match the Ethereum gas consumption instead of the // default SDK one. refundGas := uint64(0) - if evmMsg.Gas() > evmResp.GasUsed { - refundGas = evmMsg.Gas() - evmResp.GasUsed + if evmMsg.Gas() > blockGasUsed { + refundGas = evmMsg.Gas() - blockGasUsed } weiPerGas := txMsg.EffectiveGasPriceWeiPerGas(evmConfig.BaseFeeWei) if err = k.RefundGas(ctx, evmMsg.From(), refundGas, weiPerGas); err != nil { return nil, errors.Wrapf(err, "error refunding leftover gas to sender %s", evmMsg.From()) } - k.updateBlockBloom(ctx, evmResp, uint64(txConfig.LogIndex)) - - totalGasUsed, err := k.AddToBlockGasUsed(ctx, evmResp.GasUsed) - if err != nil { - return nil, errors.Wrap(err, "error adding transient gas used to block") - } - // reset the gas meter for current TxMsg (EthereumTx) - k.ResetGasMeterAndConsumeGas(ctx, totalGasUsed) + k.ResetGasMeterAndConsumeGas(ctx, blockGasUsed) err = k.EmitEthereumTxEvents(ctx, tx.To(), tx.Type(), evmMsg, evmResp) if err != nil { return nil, errors.Wrap(err, "error emitting ethereum tx events") } + err = k.EmitLogEvents(ctx, evmResp) + if err != nil { + return nil, errors.Wrap(err, "error emitting tx logs") + } blockTxIdx := uint64(txConfig.TxIndex) + 1 k.EvmState.BlockTxIndex.Set(ctx, blockTxIdx) @@ -584,7 +588,7 @@ func (k Keeper) convertCoinToEvmBornERC20( // converted to its Bank Coin representation, a balance of the ERC20 is left // inside the EVM module account in order to convert the coins back to // ERC20s. - actualSentAmount, err := k.ERC20().Transfer( + actualSentAmount, _, err := k.ERC20().Transfer( erc20Addr, evm.EVM_MODULE_ADDRESS, recipient, @@ -644,17 +648,6 @@ func (k *Keeper) EmitEthereumTxEvents( return errors.Wrap(err, "failed to emit event ethereum tx") } - // Typed event: eth.evm.v1.EventTxLog - txLogs := make([]string, len(evmResp.Logs)) - for i, log := range evmResp.Logs { - value, err := json.Marshal(log) - if err != nil { - return errors.Wrap(err, "failed to encode log") - } - txLogs[i] = string(value) - } - _ = ctx.EventManager().EmitTypedEvent(&evm.EventTxLog{TxLogs: txLogs}) - // Untyped event: "message", used for tendermint subscription ctx.EventManager().EmitEvent( sdk.NewEvent( @@ -690,6 +683,25 @@ func (k *Keeper) EmitEthereumTxEvents( return nil } +// EmitLogEvents emits all types of EVM events applicable to a particular execution case +func (k *Keeper) EmitLogEvents( + ctx sdk.Context, + evmResp *evm.MsgEthereumTxResponse, +) error { + // Typed event: eth.evm.v1.EventTxLog + txLogs := make([]string, len(evmResp.Logs)) + for i, log := range evmResp.Logs { + value, err := json.Marshal(log) + if err != nil { + return errors.Wrap(err, "failed to encode log") + } + txLogs[i] = string(value) + } + _ = ctx.EventManager().EmitTypedEvent(&evm.EventTxLog{TxLogs: txLogs}) + + return nil +} + // updateBlockBloom updates transient block bloom filter func (k *Keeper) updateBlockBloom( ctx sdk.Context, diff --git a/x/evm/precompile/funtoken.go b/x/evm/precompile/funtoken.go index c914df88a..6ed9411eb 100644 --- a/x/evm/precompile/funtoken.go +++ b/x/evm/precompile/funtoken.go @@ -109,11 +109,11 @@ type precompileFunToken struct { // function bankSend(address erc20, uint256 amount, string memory to) external; // ``` func (p precompileFunToken) bankSend( - start OnRunStartResult, + startResult OnRunStartResult, caller gethcommon.Address, readOnly bool, ) (bz []byte, err error) { - ctx, method, args := start.CacheCtx, start.Method, start.Args + ctx, method, args := startResult.CacheCtx, startResult.Method, startResult.Args if err := assertNotReadonlyTx(readOnly, method); err != nil { return nil, err } @@ -123,6 +123,8 @@ func (p precompileFunToken) bankSend( return } + var evmResponses []*evm.MsgEthereumTxResponse + // ERC20 must have FunToken mapping funtokens := p.evmKeeper.FunTokens.Collect( ctx, p.evmKeeper.FunTokens.Indexes.ERC20Addr.ExactMatch(ctx, erc20), @@ -146,10 +148,11 @@ func (p precompileFunToken) bankSend( // Caller transfers ERC20 to the EVM account transferTo := evm.EVM_MODULE_ADDRESS - gotAmount, err := p.evmKeeper.ERC20().Transfer(erc20, caller, transferTo, amount, ctx) + gotAmount, transferResp, err := p.evmKeeper.ERC20().Transfer(erc20, caller, transferTo, amount, ctx) if err != nil { return nil, fmt.Errorf("error in ERC20.transfer from caller to EVM account: %w", err) } + evmResponses = append(evmResponses, transferResp) // EVM account mints FunToken.BankDenom to module account coinToSend := sdk.NewCoin(funtoken.BankDenom, math.NewIntFromBigInt(gotAmount)) @@ -158,17 +161,18 @@ func (p precompileFunToken) bankSend( // owns the ERC20 contract and was the original minter of the ERC20 tokens. // Since we're sending them away and want accurate total supply tracking, the // tokens need to be burned. - _, err = p.evmKeeper.ERC20().Burn(erc20, evm.EVM_MODULE_ADDRESS, gotAmount, ctx) - if err != nil { - err = fmt.Errorf("ERC20.Burn: %w", err) + burnResp, e := p.evmKeeper.ERC20().Burn(erc20, evm.EVM_MODULE_ADDRESS, gotAmount, ctx) + if e != nil { + err = fmt.Errorf("ERC20.Burn: %w", e) return } + evmResponses = append(evmResponses, burnResp) } else { // NOTE: The NibiruBankKeeper needs to reference the current [vm.StateDB] before // any operation that has the potential to use Bank send methods. This will // guarantee that [evmkeeper.Keeper.SetAccBalance] journal changes are // recorded if wei (NIBI) is transferred. - p.evmKeeper.Bank.StateDB = start.StateDB + p.evmKeeper.Bank.StateDB = startResult.StateDB err = p.evmKeeper.Bank.MintCoins(ctx, evm.ModuleName, sdk.NewCoins(coinToSend)) if err != nil { return nil, fmt.Errorf("mint failed for module \"%s\" (%s): contract caller %s: %w", @@ -183,7 +187,7 @@ func (p precompileFunToken) bankSend( // any operation that has the potential to use Bank send methods. This will // guarantee that [evmkeeper.Keeper.SetAccBalance] journal changes are // recorded if wei (NIBI) is transferred. - p.evmKeeper.Bank.StateDB = start.StateDB + p.evmKeeper.Bank.StateDB = startResult.StateDB err = p.evmKeeper.Bank.SendCoinsFromModuleToAccount( ctx, evm.ModuleName, @@ -195,6 +199,11 @@ func (p precompileFunToken) bankSend( evm.ModuleName, evm.EVM_MODULE_ADDRESS.Hex(), caller.Hex(), err, ) } + for _, resp := range evmResponses { + for _, log := range resp.Logs { + startResult.StateDB.AddLog(log.ToEthereum()) + } + } // TODO: UD-DEBUG: feat: Emit EVM events