perf: minimize logic on rechecktx for recvpacket (#6280)

* perf: minimize logic on rechecktx for recvpacket

* refactor: rework layout for recvpacket rechecktx

* test: add tests for 04-channel rechecktx func

* test: add tests for core ante handler

* chore: add comment explaining is rechecktx usage

* linter appeasement

* chore: add changelog entry

* Update modules/core/ante/ante.go

Co-authored-by: Carlos Rodriguez <[email protected]>

* imp: use cached ctx for consistency

* refactor: change added test to use expected errors

* lint

---------

Co-authored-by: Carlos Rodriguez <[email protected]>
(cherry picked from commit 56ae97d)

# Conflicts:
#	modules/core/ante/ante.go

CHANGELOG.md

@@ -44,6 +44,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 
 ### Improvements
 * (core/ante) [\#6302](https://github.com/cosmos/ibc-go/pull/6302) Performance: Skip app callbacks during RecvPacket execution in checkTx within the redundant relay ante handler.
+* (core/ante) [\#6280](https://github.com/cosmos/ibc-go/pull/6280) Performance: Skip redundant proof checking in RecvPacket execution in reCheckTx within the redundant relay ante handler.
 
 ### Features
 

modules/core/04-channel/keeper/ante.go

@@ -0,0 +1,24 @@
+package keeper
+
+import (
+	errorsmod "cosmossdk.io/errors"
+
+	sdk "github.com/cosmos/cosmos-sdk/types"
+
+	"github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
+)
+
+// RecvPacketReCheckTx applies replay protection ensuring that when relay messages are
+// re-executed in ReCheckTx, we can appropriately filter out redundant relay transactions.
+func (k *Keeper) RecvPacketReCheckTx(ctx sdk.Context, packet types.Packet) error {
+	channel, found := k.GetChannel(ctx, packet.GetDestPort(), packet.GetDestChannel())
+	if !found {
+		return errorsmod.Wrap(types.ErrChannelNotFound, packet.GetDestChannel())
+	}
+
+	if err := k.applyReplayProtection(ctx, packet, channel); err != nil {
+		return err
+	}
+
+	return nil
+}

modules/core/04-channel/keeper/ante_test.go

@@ -0,0 +1,64 @@
+package keeper_test
+
+import (
+	"github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
+	ibctesting "github.com/cosmos/ibc-go/v8/testing"
+)
+
+func (suite *KeeperTestSuite) TestRecvPacketReCheckTx() {
+	var (
+		path   *ibctesting.Path
+		packet types.Packet
+	)
+
+	testCases := []struct {
+		name     string
+		malleate func()
+		expError error
+	}{
+		{
+			"success",
+			func() {},
+			nil,
+		},
+		{
+			"channel not found",
+			func() {
+				packet.DestinationPort = "invalid-port" //nolint:goconst
+			},
+			types.ErrChannelNotFound,
+		},
+		{
+			"redundant relay",
+			func() {
+				err := suite.chainB.App.GetIBCKeeper().ChannelKeeper.RecvPacketReCheckTx(suite.chainB.GetContext(), packet)
+				suite.Require().NoError(err)
+			},
+			types.ErrNoOpMsg,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		suite.Run(tc.name, func() {
+			suite.SetupTest() // reset
+			path = ibctesting.NewPath(suite.chainA, suite.chainB)
+			path.Setup()
+
+			sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData)
+			suite.Require().NoError(err)
+			packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp)
+
+			tc.malleate()
+
+			err = suite.chainB.App.GetIBCKeeper().ChannelKeeper.RecvPacketReCheckTx(suite.chainB.GetContext(), packet)
+
+			expPass := tc.expError == nil
+			if expPass {
+				suite.Require().NoError(err)
+			} else {
+				suite.Require().ErrorIs(err, tc.expError)
+			}
+		})
+	}
+}

modules/core/04-channel/keeper/packet.go

@@ -192,6 +192,29 @@ func (k Keeper) RecvPacket(
 		return errorsmod.Wrap(err, "couldn't verify counterparty packet commitment")
 	}
 
+	if err := k.applyReplayProtection(ctx, packet, channel); err != nil {
+		return err
+	}
+
+	// log that a packet has been received & executed
+	k.Logger(ctx).Info(
+		"packet received",
+		"sequence", strconv.FormatUint(packet.GetSequence(), 10),
+		"src_port", packet.GetSourcePort(),
+		"src_channel", packet.GetSourceChannel(),
+		"dst_port", packet.GetDestPort(),
+		"dst_channel", packet.GetDestChannel(),
+	)
+
+	// emit an event that the relayer can query for
+	emitRecvPacketEvent(ctx, packet, channel)
+
+	return nil
+}
+
+// applyReplayProtection ensures a packet has not already been received
+// and performs the necessary state changes to ensure it cannot be received again.
+func (k *Keeper) applyReplayProtection(ctx sdk.Context, packet types.Packet, channel types.Channel) error {
 	// REPLAY PROTECTION: The recvStartSequence will prevent historical proofs from allowing replay
 	// attacks on packets processed in previous lifecycles of a channel. After a successful channel
 	// upgrade all packets under the recvStartSequence will have been processed and thus should be
@@ -257,19 +280,6 @@ func (k Keeper) RecvPacket(
 		k.SetNextSequenceRecv(ctx, packet.GetDestPort(), packet.GetDestChannel(), nextSequenceRecv)
 	}
 
-	// log that a packet has been received & executed
-	k.Logger(ctx).Info(
-		"packet received",
-		"sequence", strconv.FormatUint(packet.GetSequence(), 10),
-		"src_port", packet.GetSourcePort(),
-		"src_channel", packet.GetSourceChannel(),
-		"dst_port", packet.GetDestPort(),
-		"dst_channel", packet.GetDestChannel(),
-	)
-
-	// emit an event that the relayer can query for
-	emitRecvPacketEvent(ctx, packet, channel)
-
 	return nil
 }
 

modules/core/ante/ante.go

@@ -37,10 +37,10 @@ func (rrd RedundantRelayDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula
 					err      error
 				)
 				// when we are in ReCheckTx mode, ctx.IsCheckTx() will also return true
-				// there we must start the if statement on ctx.IsReCheckTx() to correctly
+				// therefore we must start the if statement on ctx.IsReCheckTx() to correctly
 				// determine which mode we are in
 				if ctx.IsReCheckTx() {
-					response, err = rrd.k.RecvPacket(ctx, msg)
+					response, err = rrd.recvPacketReCheckTx(ctx, msg)
 				} else {
 					response, err = rrd.recvPacketCheckTx(ctx, msg)
 				}
@@ -130,3 +130,57 @@ func (rrd RedundantRelayDecorator) recvPacketCheckTx(ctx sdk.Context, msg *chann
 
 	return &channeltypes.MsgRecvPacketResponse{Result: channeltypes.SUCCESS}, nil
 }
+<<<<<<< HEAD
+=======
+
+// recvPacketReCheckTx runs a subset of ibc recv packet logic to be used specifically within the RedundantRelayDecorator AnteHandler.
+// It only performs core IBC receiving logic and skips any application logic.
+func (rrd RedundantRelayDecorator) recvPacketReCheckTx(ctx sdk.Context, msg *channeltypes.MsgRecvPacket) (*channeltypes.MsgRecvPacketResponse, error) {
+	// If the packet was already received, perform a no-op
+	// Use a cached context to prevent accidental state changes
+	cacheCtx, writeFn := ctx.CacheContext()
+	err := rrd.k.ChannelKeeper.RecvPacketReCheckTx(cacheCtx, msg.Packet)
+
+	switch err {
+	case nil:
+		writeFn()
+	case channeltypes.ErrNoOpMsg:
+		return &channeltypes.MsgRecvPacketResponse{Result: channeltypes.NOOP}, nil
+	default:
+		return nil, errorsmod.Wrap(err, "receive packet verification failed")
+	}
+
+	return &channeltypes.MsgRecvPacketResponse{Result: channeltypes.SUCCESS}, nil
+}
+
+// updateClientCheckTx runs a subset of ibc client update logic to be used specifically within the RedundantRelayDecorator AnteHandler.
+// The following function performs ibc client message verification for CheckTx only and state updates in both CheckTx and ReCheckTx.
+// Note that misbehaviour checks are omitted.
+func (rrd RedundantRelayDecorator) updateClientCheckTx(ctx sdk.Context, msg *clienttypes.MsgUpdateClient) error {
+	clientMsg, err := clienttypes.UnpackClientMessage(msg.ClientMessage)
+	if err != nil {
+		return err
+	}
+
+	if status := rrd.k.ClientKeeper.GetClientStatus(ctx, msg.ClientId); status != exported.Active {
+		return errorsmod.Wrapf(clienttypes.ErrClientNotActive, "cannot update client (%s) with status %s", msg.ClientId, status)
+	}
+
+	clientModule, found := rrd.k.ClientKeeper.Route(msg.ClientId)
+	if !found {
+		return errorsmod.Wrap(clienttypes.ErrRouteNotFound, msg.ClientId)
+	}
+
+	if !ctx.IsReCheckTx() {
+		if err := clientModule.VerifyClientMessage(ctx, msg.ClientId, clientMsg); err != nil {
+			return err
+		}
+	}
+
+	heights := clientModule.UpdateState(ctx, msg.ClientId, clientMsg)
+
+	ctx.Logger().With("module", "x/"+exported.ModuleName).Debug("ante ibc client update", "consensusHeights", heights)
+
+	return nil
+}
+>>>>>>> 56ae97d8 (perf: minimize logic on rechecktx for recvpacket (#6280))

modules/core/ante/ante_test.go

@@ -175,7 +175,7 @@ func (suite *AnteTestSuite) createUpdateClientMessage() sdk.Msg {
 	return msg
 }
 
-func (suite *AnteTestSuite) TestAnteDecorator() {
+func (suite *AnteTestSuite) TestAnteDecoratorCheckTx() {
 	testCases := []struct {
 		name     string
 		malleate func(suite *AnteTestSuite) []sdk.Msg
@@ -512,3 +512,95 @@ func (suite *AnteTestSuite) TestAnteDecorator() {
 		})
 	}
 }
+
+func (suite *AnteTestSuite) TestAnteDecoratorReCheckTx() {
+	testCases := []struct {
+		name     string
+		malleate func(suite *AnteTestSuite) []sdk.Msg
+		expError error
+	}{
+		{
+			"success on one new RecvPacket message",
+			func(suite *AnteTestSuite) []sdk.Msg {
+				// the RecvPacket message has not been submitted to the chain yet, so it will succeed
+				return []sdk.Msg{suite.createRecvPacketMessage(false)}
+			},
+			nil,
+		},
+		{
+			"success on one redundant and one new RecvPacket message",
+			func(suite *AnteTestSuite) []sdk.Msg {
+				return []sdk.Msg{
+					suite.createRecvPacketMessage(true),
+					suite.createRecvPacketMessage(false),
+				}
+			},
+			nil,
+		},
+		{
+			"success on invalid proof (proof checks occur in checkTx)",
+			func(suite *AnteTestSuite) []sdk.Msg {
+				msg := suite.createRecvPacketMessage(false)
+				msg.ProofCommitment = []byte("invalid-proof")
+				return []sdk.Msg{msg}
+			},
+			nil,
+		},
+		{
+			"success on app callback error, app callbacks are skipped for performance",
+			func(suite *AnteTestSuite) []sdk.Msg {
+				suite.chainB.GetSimApp().IBCMockModule.IBCApp.OnRecvPacket = func(
+					ctx sdk.Context, packet channeltypes.Packet, relayer sdk.AccAddress,
+				) exported.Acknowledgement {
+					panic(fmt.Errorf("failed OnRecvPacket mock callback"))
+				}
+
+				// the RecvPacket message has not been submitted to the chain yet, so it will succeed
+				return []sdk.Msg{suite.createRecvPacketMessage(false)}
+			},
+			nil,
+		},
+		{
+			"no success on one redundant RecvPacket message",
+			func(suite *AnteTestSuite) []sdk.Msg {
+				return []sdk.Msg{suite.createRecvPacketMessage(true)}
+			},
+			channeltypes.ErrRedundantTx,
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+
+		suite.Run(tc.name, func() {
+			// reset suite
+			suite.SetupTest()
+
+			k := suite.chainB.App.GetIBCKeeper()
+			decorator := ante.NewRedundantRelayDecorator(k)
+
+			msgs := tc.malleate(suite)
+
+			deliverCtx := suite.chainB.GetContext().WithIsCheckTx(false)
+			reCheckCtx := suite.chainB.GetContext().WithIsReCheckTx(true)
+
+			// create multimsg tx
+			txBuilder := suite.chainB.TxConfig.NewTxBuilder()
+			err := txBuilder.SetMsgs(msgs...)
+			suite.Require().NoError(err)
+			tx := txBuilder.GetTx()
+
+			next := func(ctx sdk.Context, tx sdk.Tx, simulate bool) (newCtx sdk.Context, err error) { return ctx, nil }
+
+			_, err = decorator.AnteHandle(deliverCtx, tx, false, next)
+			suite.Require().NoError(err, "antedecorator should not error on DeliverTx")
+
+			_, err = decorator.AnteHandle(reCheckCtx, tx, false, next)
+			if tc.expError == nil {
+				suite.Require().NoError(err, "non-strict decorator did not pass as expected")
+			} else {
+				suite.Require().ErrorIs(err, tc.expError, "non-strict antehandler did not return error as expected")
+			}
+		})
+	}
+}