refactor: simplify IBC redundant relay check in given restructure of …

…SDK v0.46 (#1288)

* refactor: simplify ibc redundancy check used as sdk middleware

Instead of having the function checkRedundancy check if the call is on CheckTx or SimulateTx, only call the function on CheckTx or SimulateTx

* add godoc

modules/core/keeper/sdk_middleware.go

@@ -0,0 +1,129 @@
+package keeper
+
+import (
+	"context"
+
+	"github.com/cosmos/cosmos-sdk/types/tx"
+
+	clienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/types"
+	channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
+)
+
+var _ tx.Handler = ibcTxHandler{}
+
+type ibcTxHandler struct {
+	k    *Keeper
+	next tx.Handler
+}
+
+// IBCTxMiddleware implements ibc tx handling middleware
+func IBCTxMiddleware(IBCKeeper *Keeper) tx.Middleware {
+	return func(txh tx.Handler) tx.Handler {
+		return ibcTxHandler{
+			k:    IBCKeeper,
+			next: txh,
+		}
+	}
+}
+
+// perform redundancy checks on IBC relays. If a transaction contains a relay message (`RecvPacket`, `AcknowledgePacket`, `TimeoutPacket/OnClose`)
+// and all the relay messages are redundant, it will be dropped from the mempool. If any non relay message is contained, with the exception of `UpdateClient`,
+// it will be processed as normal. A transaction with only `UpdateClient` message will be processed as normal.
+func (itxh ibcTxHandler) checkRedundancy(ctx context.Context, req tx.Request) error {
+	// keep track of total packet messages and number of redundancies across `RecvPacket`, `AcknowledgePacket`, and `TimeoutPacket/OnClose`
+	redundancies := 0
+	packetMsgs := 0
+
+	for _, m := range req.Tx.GetMsgs() {
+		switch msg := m.(type) {
+		case *channeltypes.MsgRecvPacket:
+			response, err := itxh.k.RecvPacket(ctx, msg)
+			if err != nil {
+				return err
+			}
+
+			if response.Result == channeltypes.NOOP {
+				redundancies += 1
+			}
+			packetMsgs += 1
+
+		case *channeltypes.MsgAcknowledgement:
+			response, err := itxh.k.Acknowledgement(ctx, msg)
+			if err != nil {
+				return err
+			}
+
+			if response.Result == channeltypes.NOOP {
+				redundancies += 1
+			}
+			packetMsgs += 1
+
+		case *channeltypes.MsgTimeout:
+			response, err := itxh.k.Timeout(ctx, msg)
+			if err != nil {
+				return err
+			}
+
+			if response.Result == channeltypes.NOOP {
+				redundancies += 1
+			}
+			packetMsgs += 1
+
+		case *channeltypes.MsgTimeoutOnClose:
+			response, err := itxh.k.TimeoutOnClose(ctx, msg)
+			if err != nil {
+				return err
+
+			}
+
+			if response.Result == channeltypes.NOOP {
+				redundancies += 1
+			}
+			packetMsgs += 1
+
+		case *clienttypes.MsgUpdateClient:
+			if _, err := itxh.k.UpdateClient(ctx, msg); err != nil {
+				return err
+			}
+
+		default:
+			// if the multiMsg tx has a msg that is not a packet msg or update msg, then we will not return error
+			// regardless of if all packet messages are redundant. This ensures that non-packet messages get processed
+			// even if they get batched with redundant packet messages.
+			return nil
+		}
+	}
+
+	// only return error if all packet messages are redundant
+	if redundancies == packetMsgs && packetMsgs > 0 {
+		return channeltypes.ErrRedundantTx
+	}
+
+	return nil
+}
+
+// CheckTx implements tx.Handler.CheckTx. Run redundancy checks on CheckTx to filter any redundant
+// relays in the mempool.
+func (itxh ibcTxHandler) CheckTx(ctx context.Context, req tx.Request, checkReq tx.RequestCheckTx) (tx.Response, tx.ResponseCheckTx, error) {
+	if err := itxh.checkRedundancy(ctx, req); err != nil {
+		return tx.Response{}, tx.ResponseCheckTx{}, err
+	}
+
+	return itxh.next.CheckTx(ctx, req, checkReq)
+}
+
+// DeliverTx implements tx.Handler.DeliverTx. Redundancy checks are not run on DeliverTx since
+// the transaction has already been included in a block.
+func (itxh ibcTxHandler) DeliverTx(ctx context.Context, req tx.Request) (tx.Response, error) {
+	return itxh.next.DeliverTx(ctx, req)
+}
+
+// SimulateTx implements tx.Handler.SimulateTx. Run redundancy checks on SimulateTx to filter any redundant
+// relays in the mempool.
+func (itxh ibcTxHandler) SimulateTx(ctx context.Context, req tx.Request) (tx.Response, error) {
+	if err := itxh.checkRedundancy(ctx, req); err != nil {
+		return tx.Response{}, err
+	}
+
+	return itxh.next.SimulateTx(ctx, req)
+}

modules/core/sdk_middleware/sdk_middleware_test.go → modules/core/keeper/sdk_middleware_test.go

@@ -1,4 +1,4 @@
-package sdk_middleware_test
+package keeper_test
 
 import (
 	"context"
@@ -14,7 +14,7 @@ import (
 	channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types"
 	host "github.com/cosmos/ibc-go/v3/modules/core/24-host"
 	"github.com/cosmos/ibc-go/v3/modules/core/exported"
-	ibcmiddleware "github.com/cosmos/ibc-go/v3/modules/core/middleware"
+	"github.com/cosmos/ibc-go/v3/modules/core/keeper"
 	ibctesting "github.com/cosmos/ibc-go/v3/testing"
 )
 
@@ -436,7 +436,7 @@ func (suite *MiddlewareTestSuite) TestMiddleware() {
 				// commiting it to a block, so that the when check tx runs with the redundant
 				// message they are both in the same block
 				k := suite.chainB.App.GetIBCKeeper()
-				mw := ibcmiddleware.IBCTxMiddleware(k)
+				mw := keeper.IBCTxMiddleware(k)
 				checkCtx := suite.chainB.GetContext().WithIsCheckTx(true)
 				txHandler := middleware.ComposeMiddlewares(noopTxHandler, mw)
 
@@ -462,7 +462,7 @@ func (suite *MiddlewareTestSuite) TestMiddleware() {
 			suite.SetupTest()
 
 			k := suite.chainB.App.GetIBCKeeper()
-			mw := ibcmiddleware.IBCTxMiddleware(k)
+			mw := keeper.IBCTxMiddleware(k)
 
 			msgs := tc.malleate(suite)
 

testing/simapp/sdk_middleware.go

@@ -6,7 +6,6 @@ import (
 	authmiddleware "github.com/cosmos/cosmos-sdk/x/auth/middleware"
 
 	"github.com/cosmos/ibc-go/v3/modules/core/keeper"
-	ibcmiddleware "github.com/cosmos/ibc-go/v3/modules/core/sdk_middleware"
 )
 
 // ComposeMiddlewares composes multiple middlewares on top of a tx.Handler. The
@@ -106,6 +105,6 @@ func NewDefaultTxHandler(options TxHandlerOptions) (tx.Handler, error) {
 		// should be accounted for, should go below this authmiddleware.
 		authmiddleware.ConsumeBlockGasMiddleware,
 		authmiddleware.NewTipMiddleware(options.BankKeeper),
-		ibcmiddleware.IBCTxMiddleware(options.IBCKeeper),
+		keeper.IBCTxMiddleware(options.IBCKeeper),
 	), nil
 }