From ac8f7a28c2dda2c40dec46c1a69097cb73bc1fb9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Tue, 21 May 2024 11:28:16 +0200 Subject: [PATCH] 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 * imp: use cached ctx for consistency * refactor: change added test to use expected errors * lint --------- Co-authored-by: Carlos Rodriguez (cherry picked from commit 56ae97d806339cd83cb346502bae20775a2b7ca7) # Conflicts: # modules/core/ante/ante.go --- CHANGELOG.md | 1 + modules/core/04-channel/keeper/ante.go | 24 ++++++ modules/core/04-channel/keeper/ante_test.go | 64 ++++++++++++++ modules/core/04-channel/keeper/packet.go | 36 +++++--- modules/core/ante/ante.go | 58 ++++++++++++- modules/core/ante/ante_test.go | 94 ++++++++++++++++++++- 6 files changed, 261 insertions(+), 16 deletions(-) create mode 100644 modules/core/04-channel/keeper/ante.go create mode 100644 modules/core/04-channel/keeper/ante_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 730ad9d261a..71a0727f32d 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/modules/core/04-channel/keeper/ante.go b/modules/core/04-channel/keeper/ante.go new file mode 100644 index 00000000000..ac387569c68 --- /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/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 +} 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..4bc62ff2ae6 --- /dev/null +++ b/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) + } + }) + } +} diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go index f6a68c1e123..46212b0a284 100644 --- a/modules/core/04-channel/keeper/packet.go +++ b/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 } diff --git a/modules/core/ante/ante.go b/modules/core/ante/ante.go index 40d9bc40e63..c6872c758fb 100644 --- a/modules/core/ante/ante.go +++ b/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)) diff --git a/modules/core/ante/ante_test.go b/modules/core/ante/ante_test.go index 1a9fd8e62ed..3de738dcc89 100644 --- a/modules/core/ante/ante_test.go +++ b/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") + } + }) + } +}