Fix issue in tracer not catching correct error in state_transition.go

update with suggestions from coderabbitai

x/evm/keeper/abci.go

@@ -36,7 +36,7 @@ func (k *Keeper) BeginBlock(ctx sdk.Context) error {
 			ToCosmosStartBlockEvent(
 				k,
 				ctx,
-				evmBlockConfig.CoinBase.Bytes(),
+				evmBlockConfig.CoinBase,
 				ctx.BlockHeader(),
 			),
 		)

x/evm/keeper/keeper_firehose.go

@@ -5,6 +5,7 @@ import (
 	cmtproto "github.com/cometbft/cometbft/proto/tendermint/types"
 	cosmostypes "github.com/cometbft/cometbft/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/ethereum/go-ethereum/common"
 	ethtypes "github.com/ethereum/go-ethereum/core/types"
 	"github.com/evmos/ethermint/x/evm/tracing"
 	"github.com/evmos/ethermint/x/evm/types"
@@ -23,7 +24,7 @@ func BlocksBloom(k *Keeper, ctx sdk.Context) *big.Int {
 	return bloom
 }
 
-func ToCosmosStartBlockEvent(k *Keeper, ctx sdk.Context, coinbaseBytes []byte, blockHeader cmtproto.Header) tracing.CosmosStartBlockEvent {
+func ToCosmosStartBlockEvent(k *Keeper, ctx sdk.Context, coinbaseAddr common.Address, blockHeader cmtproto.Header) tracing.CosmosStartBlockEvent {
 	// ignore the errors as we are sure that the block header is valid
 	h, _ := cosmostypes.HeaderFromProto(&blockHeader)
 	h.ValidatorsHash = ctx.CometInfo().GetValidatorsHash()
@@ -46,7 +47,7 @@ func ToCosmosStartBlockEvent(k *Keeper, ctx sdk.Context, coinbaseBytes []byte, b
 		CosmosHeader: &h,
 		BaseFee:      baseFee,
 		GasLimit:     gasLimit,
-		Coinbase:     coinbaseBytes,
+		Coinbase:     coinbaseAddr,
 		Finalized:    finalizedHeader,
 	}
 }

x/evm/keeper/state_transition.go

@@ -17,6 +17,7 @@ package keeper
 
 import (
 	"bytes"
+	"errors"
 	"fmt"
 	"math/big"
 	"sort"
@@ -40,7 +41,7 @@ import (
 	"github.com/ethereum/go-ethereum/params"
 )
 
-var configOverridesErrDesc = "failed to apply state override"
+var ErrConfigOverrides = errors.New("failed to apply state override")
 
 // NewEVM generates a go-ethereum VM from the provided Message fields and the chain parameters
 // (ChainConfig and module Params). It additionally sets the validator operator address as the
@@ -189,6 +190,20 @@ func (k *Keeper) ApplyTransaction(ctx sdk.Context, msgEth *types.MsgEthereumTx)
 	// pass true to commit the StateDB
 	res, applyMessageErr := k.ApplyMessageWithConfig(tmpCtx, msg, cfg, true)
 	if applyMessageErr != nil {
+
+		// Any of these errors will not impact the evm state / execution flow
+		if errorsmod.IsOf(applyMessageErr, types.ErrCreateDisabled, types.ErrCallDisabled, ErrConfigOverrides) {
+			return nil, errorsmod.Wrap(applyMessageErr, "failed to apply ethereum core message, issue with create, call or config overrides")
+		}
+
+		// Call onTxEnd tracer hook with an empty receipt
+		if cfg.Tracer != nil && cfg.Tracer.OnTxEnd != nil {
+			cfg.Tracer.OnTxEnd(
+				nil,
+				applyMessageErr,
+			)
+		}
+
 		return nil, errorsmod.Wrap(applyMessageErr, "failed to apply ethereum core message")
 	}
 
@@ -234,19 +249,10 @@ func (k *Keeper) ApplyTransaction(ctx sdk.Context, msgEth *types.MsgEthereumTx)
 	}
 
 	defer func() {
-		// These errors are not vm related, so they should not be passed to the vm tracer
-		if errorsmod.IsOf(applyMessageErr, types.ErrCreateDisabled, types.ErrCallDisabled) {
-			return
-		}
-
-		if applyMessageErr != nil && applyMessageErr.Error() != configOverridesErrDesc {
-			return
-		}
-
 		if cfg.Tracer != nil && cfg.Tracer.OnTxEnd != nil {
 			cfg.Tracer.OnTxEnd(
 				receipt,
-				applyMessageErr,
+				errors.New(res.VmError),
 			)
 		}
 	}()
@@ -347,7 +353,7 @@ func (k *Keeper) ApplyMessageWithConfig(
 	var evm *vm.EVM
 	if cfg.Overrides != nil {
 		if err := cfg.Overrides.Apply(stateDB); err != nil {
-			return nil, errorsmod.Wrap(err, configOverridesErrDesc)
+			return nil, errorsmod.Wrap(ErrConfigOverrides, err.Error())
 		}
 	}
 
@@ -412,7 +418,8 @@ func (k *Keeper) ApplyMessageWithConfig(
 	stateDB.Prepare(rules, msg.From, cfg.CoinBase, msg.To, vm.DefaultActivePrecompiles(rules), msg.AccessList)
 
 	if contractCreation {
-		// FIXME: also why do we want to set the nonce in the statedb twice here?
+		// Why do we want to set the nonce in the statedb twice here?
+
 		// take over the nonce management from evm:
 		// - reset sender's nonce to msg.Nonce() before calling evm.
 		// - increase sender's nonce by one no matter the result.

x/evm/tracers/firehose.go

@@ -710,7 +710,6 @@ func (f *Firehose) OnTxEnd(receipt *types.Receipt, err error) {
 	firehoseInfo("trx ending (tracer=%s, isolated=%t, error=%s)", f.tracerID, f.transactionIsolated, errorView(err))
 	f.ensureInBlockAndInTrx()
 
-	// TODO: we do nothing with the error here ???
 	// Shouldn't there be something that checks the type of error and sets the status of the transaction accordingly?
 	trxTrace := f.completeTransaction(receipt)
 
@@ -1681,7 +1680,7 @@ func newBlockHeaderFromChainHeader(h *types.Header, td *pbeth.BigInt) *pbeth.Blo
 }
 
 // FIXME: Create a unit test that is going to fail as soon as any header is added in
-func newBlockHeaderFromCosmosChainHeader(h *cosmostypes.Header, coinbase []byte, gasLimit uint64, baseFee *big.Int) *pbeth.BlockHeader {
+func newBlockHeaderFromCosmosChainHeader(h *cosmostypes.Header, coinbase common.Address, gasLimit uint64, baseFee *big.Int) *pbeth.BlockHeader {
 	difficulty := firehoseBigIntFromNative(new(big.Int).SetInt64(0))
 
 	parentBlockHash := h.LastBlockID.Hash
@@ -1700,7 +1699,7 @@ func newBlockHeaderFromCosmosChainHeader(h *cosmostypes.Header, coinbase []byte,
 		ParentHash:       parentBlockHash,
 		Number:           uint64(h.Height),
 		UncleHash:        types.EmptyUncleHash.Bytes(), // No uncles in Tendermint
-		Coinbase:         coinbase,
+		Coinbase:         coinbase.Bytes(),
 		StateRoot:        h.AppHash,
 		TransactionsRoot: transactionRoot,
 		ReceiptRoot:      types.EmptyRootHash.Bytes(),
@@ -2420,7 +2419,7 @@ func (m Memory) GetPtr(offset, size int64) []byte {
 		return nil
 	}
 
-	if len(m) >= (int(offset) + int(size)) {
+	if offset >= 0 && size >= 0 && int64(len(m)) >= offset+size {
 		return m[offset : offset+size]
 	}
 
@@ -2431,6 +2430,10 @@ func (m Memory) GetPtr(offset, size int64) []byte {
 	// executed so the memory will be of the correct size.
 	//
 	// In this situation, we must pad with zeroes when the memory is not big enough.
+	if offset >= int64(len(m)) {
+		return make([]byte, size)
+	}
+
 	reminder := m[offset:]
 	return append(reminder, make([]byte, int(size)-len(reminder))...)
 }

x/evm/tracers/firehose_test.go

@@ -36,7 +36,7 @@ func TestFirehoseCallStack_Push(t *testing.T) {
 		actions []actionRunner
 	}{
 		{
-			"push/pop emtpy", []actionRunner{
+			"push/pop empty", []actionRunner{
 				push(&pbeth.Call{}),
 				pop(),
 				check(func(t *testing.T, s *CallStack) {
@@ -84,7 +84,7 @@ func Test_validateKnownTransactionTypes(t *testing.T) {
 	}{
 		{"legacy", 0, true, nil},
 		{"access_list", 1, true, nil},
-		{"inexistant", 255, false, nil},
+		{"nonexistent", 255, false, nil},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
@@ -352,7 +352,7 @@ func TestFirehose_reorderIsolatedTransactionsAndOrdinals(t *testing.T) {
 			goldenUpdate := os.Getenv("GOLDEN_UPDATE") == "true"
 			goldenPath := tt.expectedBlockFile
 
-			if !goldenUpdate && !fileExits(t, goldenPath) {
+			if !goldenUpdate && !fileExists(t, goldenPath) {
 				t.Fatalf("the golden file %q does not exist, re-run with 'GOLDEN_UPDATE=true go test ./... -run %q' to generate the intial version", goldenPath, t.Name())
 			}
 
@@ -367,7 +367,8 @@ func TestFirehose_reorderIsolatedTransactionsAndOrdinals(t *testing.T) {
 			require.NoError(t, err)
 
 			expectedBlock := &pbeth.Block{}
-			protojson.Unmarshal(expected, expectedBlock)
+			err = protojson.Unmarshal(expected, expectedBlock)
+			require.NoError(t, err)
 
 			if !proto.Equal(expectedBlock, f.block) {
 				assert.Equal(t, expectedBlock, f.block, "Run 'GOLDEN_UPDATE=true go test ./... -run %q' to update golden file", t.Name())
@@ -434,7 +435,7 @@ var b = big.NewInt
 var empty, from, to = common.HexToAddress("00"), common.HexToAddress("01"), common.HexToAddress("02")
 var hex2Hash = common.HexToHash
 
-func fileExits(t *testing.T, path string) bool {
+func fileExists(t *testing.T, path string) bool {
 	t.Helper()
 	stat, err := os.Stat(path)
 	return err == nil && !stat.IsDir()
@@ -484,6 +485,7 @@ func TestMemory_GetPtr(t *testing.T) {
 		{"memory is just a bit too small", Memory([]byte{1, 2, 3}), args{0, 4}, []byte{1, 2, 3, 0}},
 		{"memory is flushed with request", Memory([]byte{1, 2, 3, 4}), args{0, 4}, []byte{1, 2, 3, 4}},
 		{"memory is just a bit too big", Memory([]byte{1, 2, 3, 4, 5}), args{0, 4}, []byte{1, 2, 3, 4}},
+		{"offset beyond memory length", Memory([]byte{1, 2, 3}), args{5, 2}, []byte{0, 0}},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

x/evm/tracing/hooks.go

@@ -33,7 +33,7 @@ type CosmosStartBlockEvent struct {
 	CosmosHeader *types.Header
 	BaseFee      *big.Int
 	GasLimit     uint64
-	Coinbase     []byte
+	Coinbase     common.Address
 	Finalized    *ethtypes.Header
 }