diff --git a/CHANGELOG.md b/CHANGELOG.md index f3db8c09cc0..d66487636b1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -45,6 +45,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 diff --git a/modules/core/04-channel/keeper/ante.go b/modules/core/04-channel/keeper/ante.go new file mode 100644 index 00000000000..b996f623b33 --- /dev/null +++ b/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/v7/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 +} diff --git a/modules/core/04-channel/keeper/ante_test.go b/modules/core/04-channel/keeper/ante_test.go new file mode 100644 index 00000000000..e391443bed1 --- /dev/null +++ b/modules/core/04-channel/keeper/ante_test.go @@ -0,0 +1,64 @@ +package keeper_test + +import ( + "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" + ibctesting "github.com/cosmos/ibc-go/v7/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) + suite.coordinator.Setup(path) + + 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) + } + }) + } +} diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go index 69a3f77aa11..827f324d9ac 100644 --- a/modules/core/04-channel/keeper/packet.go +++ b/modules/core/04-channel/keeper/packet.go @@ -204,6 +204,29 @@ func (k Keeper) RecvPacket( return sdkerrors.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 exported.PacketI, channel types.Channel) error { switch channel.Ordering { case types.UNORDERED: // check if the packet receipt has been received already for unordered channels @@ -257,19 +280,6 @@ func (k Keeper) RecvPacket( } - // 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 } diff --git a/modules/core/ante/ante.go b/modules/core/ante/ante.go index a4d852ecaa1..0f2855d3017 100644 --- a/modules/core/ante/ante.go +++ b/modules/core/ante/ante.go @@ -36,10 +36,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(sdk.WrapSDKContext(ctx), msg) + response, err = rrd.recvPacketReCheckTx(ctx, msg) } else { response, err = rrd.recvPacketCheckTx(ctx, msg) } @@ -129,3 +129,23 @@ func (rrd RedundantRelayDecorator) recvPacketCheckTx(ctx sdk.Context, msg *chann return &channeltypes.MsgRecvPacketResponse{Result: channeltypes.SUCCESS}, nil } + +// 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, sdkerrors.Wrap(err, "receive packet verification failed") + } + + return &channeltypes.MsgRecvPacketResponse{Result: channeltypes.SUCCESS}, nil +} diff --git a/modules/core/ante/ante_test.go b/modules/core/ante/ante_test.go index b51857acea4..fd280b6a333 100644 --- a/modules/core/ante/ante_test.go +++ b/modules/core/ante/ante_test.go @@ -174,7 +174,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 @@ -497,3 +497,81 @@ 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, + }, + { + "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") + } + }) + } +}