feat: allow PostTxProcessing to run on failures and persist data

tests/integration/x/vm/test_state_transition.go

@@ -1,12 +1,14 @@
 package vm
 
 import (
+	"errors"
 	"fmt"
 	"math"
 	"math/big"
 
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/core"
+	ethtypes "github.com/ethereum/go-ethereum/core/types"
 	gethtypes "github.com/ethereum/go-ethereum/core/types"
 	"github.com/ethereum/go-ethereum/params"
 
@@ -624,6 +626,173 @@ func (s *KeeperTestSuite) TestApplyTransaction() {
 	}
 }
 
+type testHooks struct {
+	postProcessing func(ctx sdk.Context, sender common.Address, msg core.Message, receipt *ethtypes.Receipt) error
+}
+
+func (h *testHooks) PostTxProcessing(ctx sdk.Context, sender common.Address, msg core.Message, receipt *ethtypes.Receipt) error {
+	return h.postProcessing(ctx, sender, msg, receipt)
+}
+
+func (s *KeeperTestSuite) TestApplyTransactionWithTxPostProcessing() {
+	s.EnableFeemarket = true
+	defer func() { s.EnableFeemarket = false }()
+
+	testCases := []struct {
+		name  string
+		setup func(s *KeeperTestSuite)
+		do    func(s *KeeperTestSuite)
+		after func(s *KeeperTestSuite)
+	}{
+		{
+			"pass - evm tx succeeds, post processing is called, the balance is changed",
+			func(s *KeeperTestSuite) {
+				s.Network.App.GetEVMKeeper().SetHooks(
+					keeper.NewMultiEvmHooks(
+						&testHooks{
+							postProcessing: func(ctx sdk.Context, sender common.Address, msg core.Message, receipt *ethtypes.Receipt) error {
+								return nil
+							},
+						},
+					),
+				)
+			},
+			func(s *KeeperTestSuite) {
+				senderBefore := s.Network.App.GetBankKeeper().GetBalance(s.Network.GetContext(), s.Keyring.GetAccAddr(0), "aatom").Amount
+				recipientBefore := s.Network.App.GetBankKeeper().GetBalance(s.Network.GetContext(), s.Keyring.GetAccAddr(1), "aatom").Amount
+
+				// Generate a transfer tx message
+				sender := s.Keyring.GetKey(0)
+				recipient := s.Keyring.GetAddr(1)
+				transferAmt := big.NewInt(100)
+
+				tx, err := s.Factory.GenerateSignedEthTx(sender.Priv, types.EvmTxArgs{
+					To:     &recipient,
+					Amount: transferAmt,
+				})
+				s.Require().NoError(err)
+
+				ethMsg := tx.GetMsgs()[0].(*types.MsgEthereumTx)
+				res, err := s.Network.App.GetEVMKeeper().ApplyTransaction(s.Network.GetContext(), ethMsg.AsTransaction())
+				s.Require().NoError(err)
+				s.Require().False(res.Failed())
+
+				senderAfter := s.Network.App.GetBankKeeper().GetBalance(s.Network.GetContext(), s.Keyring.GetAccAddr(0), "aatom").Amount
+				recipientAfter := s.Network.App.GetBankKeeper().GetBalance(s.Network.GetContext(), s.Keyring.GetAccAddr(1), "aatom").Amount
+				s.Require().Equal(senderBefore.Sub(sdkmath.NewIntFromBigInt(transferAmt)), senderAfter)
+				s.Require().Equal(recipientBefore.Add(sdkmath.NewIntFromBigInt(transferAmt)), recipientAfter)
+			},
+			func(s *KeeperTestSuite) {},
+		},
+		{
+			"pass - evm tx succeeds, post processing is called but fails, the balance is unchanged",
+			func(s *KeeperTestSuite) {
+				s.Network.App.GetEVMKeeper().SetHooks(
+					keeper.NewMultiEvmHooks(
+						&testHooks{
+							postProcessing: func(ctx sdk.Context, sender common.Address, msg core.Message, receipt *ethtypes.Receipt) error {
+								return errors.New("post processing failed :(")
+							},
+						},
+					),
+				)
+			},
+			func(s *KeeperTestSuite) {
+				senderBefore := s.Network.App.GetBankKeeper().GetBalance(s.Network.GetContext(), s.Keyring.GetAccAddr(0), "aatom").Amount
+				recipientBefore := s.Network.App.GetBankKeeper().GetBalance(s.Network.GetContext(), s.Keyring.GetAccAddr(1), "aatom").Amount
+
+				// Generate a transfer tx message
+				sender := s.Keyring.GetKey(0)
+				recipient := s.Keyring.GetAddr(1)
+				transferAmt := big.NewInt(100)
+
+				tx, err := s.Factory.GenerateSignedEthTx(sender.Priv, types.EvmTxArgs{
+					To:     &recipient,
+					Amount: transferAmt,
+				})
+				s.Require().NoError(err)
+
+				ethMsg := tx.GetMsgs()[0].(*types.MsgEthereumTx)
+				res, err := s.Network.App.GetEVMKeeper().ApplyTransaction(s.Network.GetContext(), ethMsg.AsTransaction())
+				s.Require().NoError(err)
+				s.Require().True(res.Failed())
+
+				senderAfter := s.Network.App.GetBankKeeper().GetBalance(s.Network.GetContext(), s.Keyring.GetAccAddr(0), "aatom").Amount
+				recipientAfter := s.Network.App.GetBankKeeper().GetBalance(s.Network.GetContext(), s.Keyring.GetAccAddr(1), "aatom").Amount
+				s.Require().Equal(senderBefore, senderAfter)
+				s.Require().Equal(recipientBefore, recipientAfter)
+			},
+			func(s *KeeperTestSuite) {},
+		},
+		{
+			"evm tx fails, post processing is called and persisted, the balance is not changed",
+			func(s *KeeperTestSuite) {
+				s.Network.App.GetEVMKeeper().SetHooks(
+					keeper.NewMultiEvmHooks(
+						&testHooks{
+							postProcessing: func(ctx sdk.Context, sender common.Address, msg core.Message, receipt *ethtypes.Receipt) error {
+								return s.Network.App.GetMintKeeper().MintCoins(
+									ctx, sdk.NewCoins(sdk.NewCoin("arandomcoin", sdkmath.NewInt(100))),
+								)
+							},
+						},
+					),
+				)
+			},
+			func(s *KeeperTestSuite) {
+				senderBefore := s.Network.App.GetBankKeeper().GetBalance(s.Network.GetContext(), s.Keyring.GetAccAddr(0), "aatom").Amount
+				recipientBefore := s.Network.App.GetBankKeeper().GetBalance(s.Network.GetContext(), s.Keyring.GetAccAddr(1), "aatom").Amount
+
+				// Generate a transfer tx message
+				sender := s.Keyring.GetKey(0)
+				recipient := s.Keyring.GetAddr(1)
+				transferAmt := senderBefore.Add(sdkmath.NewInt(100)) // transfer more than the balance
+
+				tx, err := s.Factory.GenerateSignedEthTx(sender.Priv, types.EvmTxArgs{
+					To:     &recipient,
+					Amount: transferAmt.BigInt(),
+				})
+				s.Require().NoError(err)
+
+				ethMsg := tx.GetMsgs()[0].(*types.MsgEthereumTx)
+				res, err := s.Network.App.GetEVMKeeper().ApplyTransaction(s.Network.GetContext(), ethMsg.AsTransaction())
+				s.Require().NoError(err)
+				s.Require().True(res.Failed())
+
+				senderAfter := s.Network.App.GetBankKeeper().GetBalance(s.Network.GetContext(), s.Keyring.GetAccAddr(0), "aatom").Amount
+				recipientAfter := s.Network.App.GetBankKeeper().GetBalance(s.Network.GetContext(), s.Keyring.GetAccAddr(1), "aatom").Amount
+				s.Require().Equal(senderBefore, senderAfter)
+				s.Require().Equal(recipientBefore, recipientAfter)
+			},
+			func(s *KeeperTestSuite) {
+				// check if the mint module has "arandomcoin" in its balance, it was minted in the post processing, proving that the post processing was called
+				// and that it can persist state even when the tx fails
+				balance := s.Network.App.GetBankKeeper().GetBalance(s.Network.GetContext(), s.Network.App.GetAccountKeeper().GetModuleAddress("mint"), "arandomcoin")
+				s.Require().Equal(sdkmath.NewInt(100), balance.Amount)
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		s.Run(fmt.Sprintf("Case %s", tc.name), func() {
+			s.SetupTest()
+
+			tc.setup(s)
+
+			// set bounded cosmos block gas limit
+			ctx := s.Network.GetContext().WithBlockGasMeter(storetypes.NewGasMeter(1e6))
+			err := s.Network.App.GetBankKeeper().MintCoins(ctx, "mint", sdk.NewCoins(sdk.NewCoin("aatom", sdkmath.NewInt(3e18))))
+			s.Require().NoError(err)
+			err = s.Network.App.GetBankKeeper().SendCoinsFromModuleToModule(ctx, "mint", "fee_collector", sdk.NewCoins(sdk.NewCoin("aatom", sdkmath.NewInt(3e18))))
+			s.Require().NoError(err)
+
+			tc.do(s)
+
+			tc.after(s)
+		})
+	}
+}
+
 func (s *KeeperTestSuite) TestApplyMessage() {
 	s.EnableFeemarket = true
 	defer func() { s.EnableFeemarket = false }()

x/vm/keeper/keeper.go

@@ -217,6 +217,11 @@ func (k *Keeper) PostTxProcessing(
 	return k.hooks.PostTxProcessing(ctx, sender, msg, receipt)
 }
 
+// HasHooks returns true if hooks are set
+func (k *Keeper) HasHooks() bool {
+	return k.hooks != nil
+}
+
 // ----------------------------------------------------------------------------
 // Log
 // ----------------------------------------------------------------------------

x/vm/keeper/state_transition.go

@@ -193,36 +193,54 @@ func (k *Keeper) ApplyTransaction(ctx sdk.Context, tx *ethtypes.Transaction) (*t
 	ethLogs := types.LogsToEthereum(res.Logs)
 	_, bloomReceipt := k.initializeBloomFromLogs(ctx, ethLogs)
 
-	if !res.Failed() {
-		var contractAddr common.Address
-		if msg.To == nil {
-			contractAddr = crypto.CreateAddress(msg.From, msg.Nonce)
-		}
-
-		receipt := &ethtypes.Receipt{
-			Type:              tx.Type(),
-			PostState:         nil,
-			CumulativeGasUsed: calculateCumulativeGasFromEthResponse(ctx.GasMeter(), res),
-			Bloom:             bloomReceipt,
-			Logs:              ethLogs,
-			TxHash:            txConfig.TxHash,
-			ContractAddress:   contractAddr,
-			GasUsed:           res.GasUsed,
-			BlockHash:         txConfig.BlockHash,
-			BlockNumber:       big.NewInt(ctx.BlockHeight()),
-			TransactionIndex:  txConfig.TxIndex,
-		}
+	var contractAddr common.Address
+	if msg.To == nil {
+		contractAddr = crypto.CreateAddress(msg.From, msg.Nonce)
+	}
+
+	receipt := &ethtypes.Receipt{
+		Type:              tx.Type(),
+		PostState:         nil,
+		CumulativeGasUsed: calculateCumulativeGasFromEthResponse(ctx.GasMeter(), res),
+		Bloom:             bloomReceipt,
+		Logs:              ethLogs,
+		TxHash:            txConfig.TxHash,
+		ContractAddress:   contractAddr,
+		GasUsed:           res.GasUsed,
+		BlockHash:         txConfig.BlockHash,
+		BlockNumber:       big.NewInt(ctx.BlockHeight()),
+		TransactionIndex:  txConfig.TxIndex,
+	}
+
+	if res.Failed() {
+		receipt.Status = ethtypes.ReceiptStatusFailed
+
+		// If the tx failed we discard the old context and create a new one, so
+		// PostTxProcessing can persist data even if the tx fails.
+		tmpCtx, commitFn = ctx.CacheContext()
+	} else {
+		receipt.Status = ethtypes.ReceiptStatusSuccessful
+	}
 
-		signerAddr, err := signer.Sender(tx)
-		if err != nil {
-			return nil, errorsmod.Wrap(err, "failed to extract sender address from ethereum transaction")
-		}
+	signerAddr, err := signer.Sender(tx)
+	if err != nil {
+		return nil, errorsmod.Wrap(err, "failed to extract sender address from ethereum transaction")
+	}
 
-		eventsLen := len(tmpCtx.EventManager().Events())
+	eventsLen := len(tmpCtx.EventManager().Events())
 
+	// Only call PostTxProcessing if there are hooks set, to avoid calling commitFn unnecessarily
+	if !k.HasHooks() {
+		// If there are no hooks, we can commit the state immediately if the tx is successful
+		if commitFn != nil && !res.Failed() {
+			commitFn()
+		}
+	} else {
 		// Note: PostTxProcessing hooks currently do not charge for gas
-		// and function similar to EndBlockers in abci, but for EVM transactions
-		if err = k.PostTxProcessing(tmpCtx, signerAddr, *msg, receipt); err != nil {
+		// and function similar to EndBlockers in abci, but for EVM transactions.
+		// It will persist data even if the tx fails.
+		err = k.PostTxProcessing(tmpCtx, signerAddr, *msg, receipt)
+		if err != nil {
 			// If hooks returns an error, revert the whole tx.
 			res.VmError = errorsmod.Wrap(err, "failed to execute post transaction processing").Error()
 			k.Logger(ctx).Error("tx post processing failed", "error", err)
@@ -231,11 +249,20 @@ func (k *Keeper) ApplyTransaction(ctx sdk.Context, tx *ethtypes.Transaction) (*t
 			res.Logs = nil
 			receipt.Logs = nil
 			receipt.Bloom = ethtypes.Bloom{} // Clear bloom filter
-		} else if commitFn != nil {
-			commitFn()
+		} else {
+			if commitFn != nil {
+				commitFn()
+			}
 
 			// Since the post-processing can alter the log, we need to update the result
-			res.Logs = types.NewLogsFromEth(receipt.Logs)
+			if res.Failed() {
+				res.Logs = nil
+				receipt.Logs = nil
+				receipt.Bloom = ethtypes.Bloom{}
+			} else {
+				res.Logs = types.NewLogsFromEth(receipt.Logs)
+			}
+
 			events := tmpCtx.EventManager().Events()
 			if len(events) > eventsLen {
 				ctx.EventManager().EmitEvents(events[eventsLen:])