fix(evm): proper tx gas refund outside of CallContract

CHANGELOG.md

@@ -55,6 +55,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - [#2127](https://github.com/NibiruChain/nibiru/pull/2127) - fix(vesting): disabled built in auth/vesting module functionality
 - [#2129](https://github.com/NibiruChain/nibiru/pull/2129) - fix(evm): issue with infinite recursion in erc20 funtoken contracts
 - [#2130](https://github.com/NibiruChain/nibiru/pull/2130) - fix(evm): proper nonce management in statedb
+- [#2132](https://github.com/NibiruChain/nibiru/pull/2132) - fix(evm): proper tx gas refund
 - [#2134](https://github.com/NibiruChain/nibiru/pull/2134) - fix(evm): query of NIBI should use bank state, not the StateDB
 - [#2139](https://github.com/NibiruChain/nibiru/pull/2139) - fix(evm): erc20 born funtoken: properly burn bank coins after converting coin back to erc20 
 - [#2140](https://github.com/NibiruChain/nibiru/pull/2140) - fix(bank): bank keeper extension now charges gas for the bank operations

eth/rpc/backend/backend_suite_test.go

@@ -5,6 +5,7 @@ import (
 	"crypto/ecdsa"
 	"fmt"
 	"math/big"
+	"sync"
 	"testing"
 	"time"
 
@@ -33,6 +34,9 @@ import (
 	"github.com/NibiruChain/nibiru/v2/x/common/testutil/testnetwork"
 )
 
+// testMutex is used to synchronize the tests which are broadcasting transactions concurrently
+var testMutex sync.Mutex
+
 var (
 	recipient    = evmtest.NewEthPrivAcc().EthAddr
 	amountToSend = evm.NativeToWei(big.NewInt(1))
@@ -95,7 +99,7 @@ func (s *BackendSuite) SetupSuite() {
 	// Send Transfer TX and use the results in the tests
 	s.Require().NoError(err)
 	transferTxHash = s.SendNibiViaEthTransfer(recipient, amountToSend, true)
-	blockNumber, blockHash := WaitForReceipt(s, transferTxHash)
+	blockNumber, blockHash, _ := WaitForReceipt(s, transferTxHash)
 	s.Require().NotNil(blockNumber)
 	s.Require().NotNil(blockHash)
 	transferTxBlockNumber = rpc.NewBlockNumber(blockNumber)
@@ -104,7 +108,7 @@ func (s *BackendSuite) SetupSuite() {
 	// Deploy test erc20 contract
 	deployContractTxHash, contractAddress := s.DeployTestContract(true)
 	testContractAddress = contractAddress
-	blockNumber, blockHash = WaitForReceipt(s, deployContractTxHash)
+	blockNumber, blockHash, _ = WaitForReceipt(s, deployContractTxHash)
 	s.Require().NotNil(blockNumber)
 	s.Require().NotNil(blockHash)
 	deployContractBlockNumber = rpc.NewBlockNumber(blockNumber)
@@ -167,35 +171,41 @@ func SendTransaction(s *BackendSuite, tx *gethcore.LegacyTx, waitForNextBlock bo
 	return txHash
 }
 
-func WaitForReceipt(s *BackendSuite, txHash gethcommon.Hash) (*big.Int, *gethcommon.Hash) {
+// WaitForReceipt waits for a transaction to be included in a block, returns block number, block hash and receipt
+func WaitForReceipt(s *BackendSuite, txHash gethcommon.Hash) (*big.Int, *gethcommon.Hash, *backend.TransactionReceipt) {
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 	defer cancel()
 
 	for {
 		receipt, err := s.backend.GetTransactionReceipt(txHash)
 		if err != nil {
-			return nil, nil
+			return nil, nil, nil
 		}
 		if receipt != nil {
-			return receipt.BlockNumber, &receipt.BlockHash
+			return receipt.BlockNumber, &receipt.BlockHash, receipt
 		}
 		select {
 		case <-ctx.Done():
 			fmt.Println("Timeout reached, transaction not included in a block yet.")
-			return nil, nil
+			return nil, nil, nil
 		default:
 			time.Sleep(1 * time.Second)
 		}
 	}
 }
 
-// getCurrentNonce returns the current nonce of the funded account
-func (s *BackendSuite) getCurrentNonce(address gethcommon.Address) uint64 {
-	bn, err := s.backend.BlockNumber()
+// getUnibiBalance returns the balance of an address in unibi
+func (s *BackendSuite) getUnibiBalance(address gethcommon.Address) *big.Int {
+	latestBlock := rpc.EthLatestBlockNumber
+	latestBlockOrHash := rpc.BlockNumberOrHash{BlockNumber: &latestBlock}
+	balance, err := s.backend.GetBalance(address, latestBlockOrHash)
 	s.Require().NoError(err)
-	currentHeight := rpc.BlockNumber(bn)
+	return evm.WeiToNative(balance.ToInt())
+}
 
-	nonce, err := s.backend.GetTransactionCount(address, currentHeight)
+// getCurrentNonce returns the current nonce of the funded account
+func (s *BackendSuite) getCurrentNonce(address gethcommon.Address) uint64 {
+	nonce, err := s.backend.GetTransactionCount(s.fundedAccEthAddr, rpc.EthPendingBlockNumber)
 	s.Require().NoError(err)
 
 	return uint64(*nonce)

eth/rpc/backend/gas_used_test.go

@@ -0,0 +1,188 @@
+package backend_test
+
+import (
+	"math/big"
+
+	"github.com/ethereum/go-ethereum/common/hexutil"
+	gethcore "github.com/ethereum/go-ethereum/core/types"
+
+	"github.com/NibiruChain/nibiru/v2/eth"
+	"github.com/NibiruChain/nibiru/v2/eth/rpc"
+	"github.com/NibiruChain/nibiru/v2/x/common/testutil"
+	"github.com/NibiruChain/nibiru/v2/x/evm"
+	"github.com/NibiruChain/nibiru/v2/x/evm/embeds"
+	"github.com/NibiruChain/nibiru/v2/x/evm/evmtest"
+	"github.com/NibiruChain/nibiru/v2/x/evm/precompile"
+)
+
+// TestGasUsedTransfers verifies that gas used is correctly calculated for simple transfers.
+// Test creates 2 eth transfer txs that are supposed to be included in the same block.
+// It checks that gas used is the same for both txs and the total block gas is greater than the sum of 2 gas used.
+func (s *BackendSuite) TestGasUsedTransfers() {
+	// Test is broadcasting txs. Lock to avoid nonce conflicts.
+	testMutex.Lock()
+	defer testMutex.Unlock()
+
+	// Start with new block
+	s.Require().NoError(s.network.WaitForNextBlock())
+	balanceBefore := s.getUnibiBalance(s.fundedAccEthAddr)
+
+	// Send 2 similar transfers
+	randomEthAddr := evmtest.NewEthPrivAcc().EthAddr
+	txHash1 := s.SendNibiViaEthTransfer(randomEthAddr, amountToSend, false)
+	txHash2 := s.SendNibiViaEthTransfer(randomEthAddr, amountToSend, false)
+
+	blockNumber1, _, receipt1 := WaitForReceipt(s, txHash1)
+	blockNumber2, _, receipt2 := WaitForReceipt(s, txHash2)
+
+	s.Require().NotNil(receipt1)
+	s.Require().NotNil(receipt2)
+
+	s.Require().Equal(gethcore.ReceiptStatusSuccessful, receipt1.Status)
+	s.Require().Equal(gethcore.ReceiptStatusSuccessful, receipt2.Status)
+
+	// Expect txs are included into one block
+	s.Require().Equal(blockNumber1, blockNumber2)
+
+	// Ensure that gas used is the same for both transactions
+	s.Require().Equal(receipt1.GasUsed, receipt2.GasUsed)
+
+	// Get block receipt and check gas used
+	block, err := s.backend.GetBlockByNumber(rpc.NewBlockNumber(blockNumber1), false)
+	s.Require().NoError(err)
+	s.Require().NotNil(block)
+	s.Require().NotNil(block["gasUsed"])
+	s.Require().GreaterOrEqual(block["gasUsed"].(*hexutil.Big).ToInt().Uint64(), receipt1.GasUsed+receipt2.GasUsed)
+
+	// Balance after should be equal to balance before minus gas used and amount sent
+	balanceAfter := s.getUnibiBalance(s.fundedAccEthAddr)
+	s.Require().Equal(
+		receipt1.GasUsed+receipt2.GasUsed+2,
+		balanceBefore.Uint64()-balanceAfter.Uint64(),
+	)
+}
+
+// TestGasUsedFunTokens verifies that gas used is correctly calculated for precompile "sendToBank" txs.
+// Test creates 3 txs: 2 successful and one failing.
+// Successful txs gas should be refunded and failing tx should consume 100% of the gas limit.
+// It also checks that txs are included in the same block and block gas is greater or equals
+// to the total gas used by txs.
+func (s *BackendSuite) TestGasUsedFunTokens() {
+	// Test is broadcasting txs. Lock to avoid nonce conflicts.
+	testMutex.Lock()
+	defer testMutex.Unlock()
+
+	// Create funtoken from erc20
+	erc20Addr, err := eth.NewEIP55AddrFromStr(testContractAddress.String())
+	s.Require().NoError(err)
+
+	_, err = s.backend.GetTransactionCount(s.fundedAccEthAddr, rpc.EthPendingBlockNumber)
+	s.Require().NoError(err)
+
+	txResp, err := s.network.BroadcastMsgs(s.node.Address, &evm.MsgCreateFunToken{
+		Sender:    s.node.Address.String(),
+		FromErc20: &erc20Addr,
+	})
+	s.Require().NoError(err)
+	s.Require().NotNil(txResp)
+	s.Require().NoError(s.network.WaitForNextBlock())
+
+	randomNibiAddress := testutil.AccAddress()
+	packedArgsPass, err := embeds.SmartContract_FunToken.ABI.Pack(
+		"sendToBank",
+		erc20Addr.Address,
+		big.NewInt(1),
+		randomNibiAddress.String(),
+	)
+	s.Require().NoError(err)
+
+	nonce, err := s.backend.GetTransactionCount(s.fundedAccEthAddr, rpc.EthPendingBlockNumber)
+	s.Require().NoError(err)
+
+	balanceBefore := s.getUnibiBalance(s.fundedAccEthAddr)
+
+	txHash1 := SendTransaction(
+		s,
+		&gethcore.LegacyTx{
+			Nonce:    uint64(*nonce),
+			To:       &precompile.PrecompileAddr_FunToken,
+			Data:     packedArgsPass,
+			Gas:      1_500_000,
+			GasPrice: big.NewInt(1),
+		},
+		false,
+	)
+
+	packedArgsFail, err := embeds.SmartContract_FunToken.ABI.Pack(
+		"sendToBank",
+		erc20Addr.Address,
+		big.NewInt(1),
+		"invalidAddress",
+	)
+	s.Require().NoError(err)
+	txHash2 := SendTransaction( // should fail due to invalid recipient address
+		s,
+		&gethcore.LegacyTx{
+			Nonce:    uint64(*nonce + 1),
+			To:       &precompile.PrecompileAddr_FunToken,
+			Data:     packedArgsFail,
+			Gas:      1_500_000,
+			GasPrice: big.NewInt(1),
+		},
+		false,
+	)
+	txHash3 := SendTransaction(
+		s,
+		&gethcore.LegacyTx{
+			Nonce:    uint64(*nonce + 2),
+			To:       &precompile.PrecompileAddr_FunToken,
+			Data:     packedArgsPass,
+			Gas:      1_500_000,
+			GasPrice: big.NewInt(1),
+		},
+		false,
+	)
+	blockNumber1, _, receipt1 := WaitForReceipt(s, txHash1)
+	blockNumber2, _, receipt2 := WaitForReceipt(s, txHash2)
+	blockNumber3, _, receipt3 := WaitForReceipt(s, txHash3)
+
+	s.Require().NotNil(receipt1)
+	s.Require().NotNil(receipt2)
+	s.Require().NotNil(receipt3)
+
+	s.Require().NotNil(blockNumber1)
+	s.Require().NotNil(blockNumber2)
+	s.Require().NotNil(blockNumber3)
+
+	// TXs should have been included in the same block
+	s.Require().Equal(blockNumber1, blockNumber2)
+	s.Require().Equal(blockNumber2, blockNumber3)
+
+	// 1 and 3 should pass and 2 should fail
+	s.Require().Equal(gethcore.ReceiptStatusSuccessful, receipt1.Status)
+	s.Require().Equal(gethcore.ReceiptStatusFailed, receipt2.Status)
+	s.Require().Equal(gethcore.ReceiptStatusSuccessful, receipt3.Status)
+
+	// TX 1 and 3 should have gas used lower than specified gas limit
+	s.Require().Less(receipt1.GasUsed, uint64(500_000))
+	s.Require().Less(receipt3.GasUsed, uint64(500_000))
+
+	// TX 2 should have gas used equal to specified gas limit as it failed
+	s.Require().Equal(uint64(1_500_000), receipt2.GasUsed)
+
+	block, err := s.backend.GetBlockByNumber(rpc.NewBlockNumber(blockNumber1), false)
+	s.Require().NoError(err)
+	s.Require().NotNil(block)
+	s.Require().NotNil(block["gasUsed"])
+	s.Require().GreaterOrEqual(
+		block["gasUsed"].(*hexutil.Big).ToInt().Uint64(),
+		receipt1.GasUsed+receipt2.GasUsed+receipt3.GasUsed,
+	)
+
+	// Balance after should be equal to balance before minus gas used
+	balanceAfter := s.getUnibiBalance(s.fundedAccEthAddr)
+	s.Require().Equal(
+		receipt1.GasUsed+receipt2.GasUsed+receipt3.GasUsed,
+		balanceBefore.Uint64()-balanceAfter.Uint64(),
+	)
+}

eth/rpc/backend/nonce_test.go

@@ -13,6 +13,10 @@ import (
 // TestNonceIncrementWithMultipleMsgsTx tests that the nonce is incremented correctly
 // when multiple messages are included in a single transaction.
 func (s *BackendSuite) TestNonceIncrementWithMultipleMsgsTx() {
+	// Test is broadcasting txs. Lock to avoid nonce conflicts.
+	testMutex.Lock()
+	defer testMutex.Unlock()
+
 	nonce := s.getCurrentNonce(s.fundedAccEthAddr)
 
 	// Create series of 3 tx messages. Expecting nonce to be incremented by 3
@@ -38,7 +42,7 @@ func (s *BackendSuite) TestNonceIncrementWithMultipleMsgsTx() {
 
 	// Assert all transactions included in block
 	for _, tx := range []gethcore.Transaction{creationTx, firstTransferTx, secondTransferTx} {
-		blockNum, blockHash := WaitForReceipt(s, tx.Hash())
+		blockNum, blockHash, _ := WaitForReceipt(s, tx.Hash())
 		s.Require().NotNil(blockNum)
 		s.Require().NotNil(blockHash)
 	}

x/evm/keeper/call_contract.go

@@ -108,14 +108,11 @@ func (k Keeper) CallContractWithInput(
 		ctx, evmMsg, evm.NewNoOpTracer(), commit, evmCfg, txConfig, true,
 	)
 	if err != nil {
-		// We don't know the actual gas used, so consuming the gas limit
-		k.ResetGasMeterAndConsumeGas(ctx, gasLimit)
 		err = errors.Wrap(err, "failed to apply ethereum core message")
 		return
 	}
 
 	if evmResp.Failed() {
-		k.ResetGasMeterAndConsumeGas(ctx, evmResp.GasUsed)
 		if strings.Contains(evmResp.VmError, vm.ErrOutOfGas.Error()) {
 			err = fmt.Errorf("gas required exceeds allowance (%d)", gasLimit)
 			return
@@ -130,12 +127,6 @@ func (k Keeper) CallContractWithInput(
 
 	// Success, update block gas used and bloom filter
 	if commit {
-		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, blockGasUsed)
 		k.updateBlockBloom(ctx, evmResp, uint64(txConfig.LogIndex))
 		// TODO: remove after migrating logs
 		//err = k.EmitLogEvents(ctx, evmResp)

x/evm/keeper/funtoken_from_coin.go

@@ -78,12 +78,18 @@
 	bytecodeForCall := append(embeds.SmartContract_ERC20Minter.Bytecode, packedArgs...)
 
 	// nil address for contract creation
-	_, _, err = k.CallContractWithInput(
+	evmResp, _, err := k.CallContractWithInput(
 		ctx, evm.EVM_MODULE_ADDRESS, nil, true, bytecodeForCall, Erc20GasLimitDeploy,
 	)
 	if err != nil {
+		k.ResetGasMeterAndConsumeGas(ctx, ctx.GasMeter().Limit())
 		return gethcommon.Address{}, errors.Wrap(err, "failed to deploy ERC20 contract")
 	}
+	blockGasUsed, errBlockGasUsed := k.AddToBlockGasUsed(ctx, evmResp.GasUsed)
+	if errBlockGasUsed != nil {
+		return gethcommon.Address{}, errors.Wrap(errBlockGasUsed, "error adding transient gas used")
+	}
+	k.ResetGasMeterAndConsumeGas(ctx, blockGasUsed)
 
 	return erc20Addr, nil
 }

x/evm/keeper/funtoken_from_coin_test.go

@@ -255,6 +255,8 @@ func (s *FunTokenFromCoinSuite) TestConvertCoinToEvmAndBack() {
 	s.Require().NoError(err)
 	s.Require().Equal("0", balance.String())
 
+	deps.ResetGasMeter()
+
 	s.T().Log("sad: Convert more erc-20 to back to bank coin, insufficient funds")
 	_, err = deps.EvmKeeper.CallContract(
 		deps.Ctx,

x/evm/keeper/keeper.go

@@ -104,7 +104,7 @@
 ) (blockGasUsed uint64, err error) {
 	// Either k.EvmState.BlockGasUsed.GetOr() or k.EvmState.BlockGasUsed.Set()
 	// also consume gas and could panic.
-	defer HandleOutOfGasPanic(&err, "")
+	defer HandleOutOfGasPanic(&err, "")()
 
 	blockGasUsed = k.EvmState.BlockGasUsed.GetOr(ctx, 0) + gasUsed
 	if blockGasUsed < gasUsed {
@@ -147,6 +147,7 @@
 		if r := recover(); r != nil {
 			switch r.(type) {
 			case sdk.ErrorOutOfGas:
+
 				*err = vm.ErrOutOfGas
 			default:
 				panic(r)