From 5cdbb451aa5fe7b559a81dced6aa1c0e6e3344b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?colin=20axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 20 May 2024 15:49:45 +0200 Subject: [PATCH 1/2] perf: minimize necessary execution on recvpacket checktx (#6302) * perf: only perform core ibc logic on recvpacket checktx * try me linter * fix: reorder if and add comment * chore: add changelog entry (cherry picked from commit 0993246ffe6d9776f4165ede02c49b7bc753a213) # Conflicts: # CHANGELOG.md # modules/core/ante/ante.go # modules/core/ante/ante_test.go --- CHANGELOG.md | 9 ++++ modules/core/ante/ante.go | 76 ++++++++++++++++++++++++++++++++++ modules/core/ante/ante_test.go | 49 +++++++++++++++++++++- 3 files changed, 133 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e24a819f75f..663e32fe559 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,7 +44,16 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements +<<<<<<< HEAD * (apps/27-interchain-accounts) [\#6144](https://github.com/cosmos/ibc-go/pull/6144) Emit an event signalling that the host submodule is disabled. +======= +* (apps/27-interchain-accounts) [\#5533](https://github.com/cosmos/ibc-go/pull/5533) ICA host sets the host connection ID on `OnChanOpenTry`, so that ICA controller implementations are not obliged to set the value on `OnChanOpenInit` if they are not able. +* (core/02-client, core/03-connection, apps/27-interchain-accounts) [\#6256](https://github.com/cosmos/ibc-go/pull/6256) Add length checking of array fields in messages. +* (apps/27-interchain-accounts, apps/tranfer, apps/29-fee) [\#6253](https://github.com/cosmos/ibc-go/pull/6253) Allow channel handshake to succeed if fee middleware is wired up on one side, but not the other. +* (apps/transfer) [\#6268](https://github.com/cosmos/ibc-go/pull/6268) Use memo strings instead of JSON keys in `AllowedPacketData` of transfer authorization. +* (core/ante) [\#6278](https://github.com/cosmos/ibc-go/pull/6278) Performance: Exclude pruning from tendermint client updates in ante handler executions. +* (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. +>>>>>>> 0993246f (perf: minimize necessary execution on recvpacket checktx (#6302)) ### Features diff --git a/modules/core/ante/ante.go b/modules/core/ante/ante.go index 7b8cdb326de..9cc42200a7c 100644 --- a/modules/core/ante/ante.go +++ b/modules/core/ante/ante.go @@ -30,10 +30,26 @@ func (rrd RedundantRelayDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula for _, m := range tx.GetMsgs() { switch msg := m.(type) { case *channeltypes.MsgRecvPacket: +<<<<<<< HEAD response, err := rrd.k.RecvPacket(sdk.WrapSDKContext(ctx), msg) +======= + var ( + response *channeltypes.MsgRecvPacketResponse + 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 + // determine which mode we are in + if ctx.IsReCheckTx() { + response, err = rrd.k.RecvPacket(ctx, msg) + } else { + response, err = rrd.recvPacketCheckTx(ctx, msg) + } +>>>>>>> 0993246f (perf: minimize necessary execution on recvpacket checktx (#6302)) if err != nil { return ctx, err } + if response.Result == channeltypes.NOOP { redundancies++ } @@ -90,3 +106,63 @@ func (rrd RedundantRelayDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula } return next(ctx, tx, simulate) } +<<<<<<< HEAD +======= + +// recvPacketCheckTx 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) recvPacketCheckTx(ctx sdk.Context, msg *channeltypes.MsgRecvPacket) (*channeltypes.MsgRecvPacketResponse, error) { + // grab channel capability + _, capability, err := rrd.k.ChannelKeeper.LookupModuleByChannel(ctx, msg.Packet.DestinationPort, msg.Packet.DestinationChannel) + if err != nil { + return nil, errorsmod.Wrap(err, "could not retrieve module from port-id") + } + + // 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.RecvPacket(cacheCtx, capability, msg.Packet, msg.ProofCommitment, msg.ProofHeight) + + 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 +} +>>>>>>> 0993246f (perf: minimize necessary execution on recvpacket checktx (#6302)) diff --git a/modules/core/ante/ante_test.go b/modules/core/ante/ante_test.go index aca0fefd9a7..fe6bbe51020 100644 --- a/modules/core/ante/ante_test.go +++ b/modules/core/ante/ante_test.go @@ -1,18 +1,38 @@ package ante_test import ( + "fmt" "testing" +<<<<<<< HEAD +======= + "github.com/stretchr/testify/require" + testifysuite "github.com/stretchr/testify/suite" + + codectypes "github.com/cosmos/cosmos-sdk/codec/types" +>>>>>>> 0993246f (perf: minimize necessary execution on recvpacket checktx (#6302)) sdk "github.com/cosmos/cosmos-sdk/types" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" +<<<<<<< HEAD clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" host "github.com/cosmos/ibc-go/v7/modules/core/24-host" "github.com/cosmos/ibc-go/v7/modules/core/ante" "github.com/cosmos/ibc-go/v7/modules/core/exported" ibctesting "github.com/cosmos/ibc-go/v7/testing" +======= + capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types" + clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" + channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types" + commitmenttypes "github.com/cosmos/ibc-go/v8/modules/core/23-commitment/types" + host "github.com/cosmos/ibc-go/v8/modules/core/24-host" + "github.com/cosmos/ibc-go/v8/modules/core/ante" + "github.com/cosmos/ibc-go/v8/modules/core/exported" + ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" + ibctesting "github.com/cosmos/ibc-go/v8/testing" +>>>>>>> 0993246f (perf: minimize necessary execution on recvpacket checktx (#6302)) ) type AnteTestSuite struct { @@ -45,7 +65,7 @@ func TestAnteTestSuite(t *testing.T) { } // createRecvPacketMessage creates a RecvPacket message for a packet sent from chain A to chain B. -func (suite *AnteTestSuite) createRecvPacketMessage(isRedundant bool) sdk.Msg { +func (suite *AnteTestSuite) createRecvPacketMessage(isRedundant bool) *channeltypes.MsgRecvPacket { sequence, err := suite.path.EndpointA.SendPacket(clienttypes.NewHeight(2, 0), 0, ibctesting.MockPacketData) suite.Require().NoError(err) @@ -341,6 +361,20 @@ func (suite *AnteTestSuite) TestAnteDecorator() { }, true, }, + { + "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 { @@ -424,7 +458,11 @@ func (suite *AnteTestSuite) TestAnteDecorator() { channeltypes.NewMsgRecvPacket(packet, []byte("proof"), clienttypes.NewHeight(1, 1), "signer"), } }, +<<<<<<< HEAD false, +======= + commitmenttypes.ErrInvalidProof, +>>>>>>> 0993246f (perf: minimize necessary execution on recvpacket checktx (#6302)) }, { "no success on one new message and one redundant message in the same block", @@ -450,6 +488,15 @@ func (suite *AnteTestSuite) TestAnteDecorator() { }, false, }, + { + "no success on recvPacket checkTx, no capability found", + func(suite *AnteTestSuite) []sdk.Msg { + msg := suite.createRecvPacketMessage(false) + msg.Packet.DestinationPort = "invalid-port" + return []sdk.Msg{msg} + }, + capabilitytypes.ErrCapabilityNotFound, + }, } for _, tc := range testCases { From 37c8564fd02c35a7f864dc59816eb6a46a40ae70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 20 May 2024 16:31:22 +0200 Subject: [PATCH 2/2] fix: merge conflicts --- CHANGELOG.md | 8 ------ modules/core/ante/ante.go | 45 +++------------------------------- modules/core/ante/ante_test.go | 41 ++----------------------------- 3 files changed, 6 insertions(+), 88 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 663e32fe559..88e96cfdf06 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,16 +44,8 @@ Ref: https://keepachangelog.com/en/1.0.0/ ### Improvements -<<<<<<< HEAD * (apps/27-interchain-accounts) [\#6144](https://github.com/cosmos/ibc-go/pull/6144) Emit an event signalling that the host submodule is disabled. -======= -* (apps/27-interchain-accounts) [\#5533](https://github.com/cosmos/ibc-go/pull/5533) ICA host sets the host connection ID on `OnChanOpenTry`, so that ICA controller implementations are not obliged to set the value on `OnChanOpenInit` if they are not able. -* (core/02-client, core/03-connection, apps/27-interchain-accounts) [\#6256](https://github.com/cosmos/ibc-go/pull/6256) Add length checking of array fields in messages. -* (apps/27-interchain-accounts, apps/tranfer, apps/29-fee) [\#6253](https://github.com/cosmos/ibc-go/pull/6253) Allow channel handshake to succeed if fee middleware is wired up on one side, but not the other. -* (apps/transfer) [\#6268](https://github.com/cosmos/ibc-go/pull/6268) Use memo strings instead of JSON keys in `AllowedPacketData` of transfer authorization. -* (core/ante) [\#6278](https://github.com/cosmos/ibc-go/pull/6278) Performance: Exclude pruning from tendermint client updates in ante handler executions. * (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. ->>>>>>> 0993246f (perf: minimize necessary execution on recvpacket checktx (#6302)) ### Features diff --git a/modules/core/ante/ante.go b/modules/core/ante/ante.go index 9cc42200a7c..a4d852ecaa1 100644 --- a/modules/core/ante/ante.go +++ b/modules/core/ante/ante.go @@ -2,6 +2,7 @@ package ante import ( sdk "github.com/cosmos/cosmos-sdk/types" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" @@ -30,9 +31,6 @@ func (rrd RedundantRelayDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula for _, m := range tx.GetMsgs() { switch msg := m.(type) { case *channeltypes.MsgRecvPacket: -<<<<<<< HEAD - response, err := rrd.k.RecvPacket(sdk.WrapSDKContext(ctx), msg) -======= var ( response *channeltypes.MsgRecvPacketResponse err error @@ -41,11 +39,10 @@ func (rrd RedundantRelayDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula // there 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.k.RecvPacket(sdk.WrapSDKContext(ctx), msg) } else { response, err = rrd.recvPacketCheckTx(ctx, msg) } ->>>>>>> 0993246f (perf: minimize necessary execution on recvpacket checktx (#6302)) if err != nil { return ctx, err } @@ -106,8 +103,6 @@ func (rrd RedundantRelayDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula } return next(ctx, tx, simulate) } -<<<<<<< HEAD -======= // recvPacketCheckTx 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. @@ -115,7 +110,7 @@ func (rrd RedundantRelayDecorator) recvPacketCheckTx(ctx sdk.Context, msg *chann // grab channel capability _, capability, err := rrd.k.ChannelKeeper.LookupModuleByChannel(ctx, msg.Packet.DestinationPort, msg.Packet.DestinationChannel) if err != nil { - return nil, errorsmod.Wrap(err, "could not retrieve module from port-id") + return nil, sdkerrors.Wrap(err, "could not retrieve module from port-id") } // If the packet was already received, perform a no-op @@ -129,40 +124,8 @@ func (rrd RedundantRelayDecorator) recvPacketCheckTx(ctx sdk.Context, msg *chann case channeltypes.ErrNoOpMsg: return &channeltypes.MsgRecvPacketResponse{Result: channeltypes.NOOP}, nil default: - return nil, errorsmod.Wrap(err, "receive packet verification failed") + return nil, sdkerrors.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 -} ->>>>>>> 0993246f (perf: minimize necessary execution on recvpacket checktx (#6302)) diff --git a/modules/core/ante/ante_test.go b/modules/core/ante/ante_test.go index fe6bbe51020..b51857acea4 100644 --- a/modules/core/ante/ante_test.go +++ b/modules/core/ante/ante_test.go @@ -1,38 +1,19 @@ package ante_test import ( - "fmt" "testing" -<<<<<<< HEAD -======= "github.com/stretchr/testify/require" - testifysuite "github.com/stretchr/testify/suite" + "github.com/stretchr/testify/suite" - codectypes "github.com/cosmos/cosmos-sdk/codec/types" ->>>>>>> 0993246f (perf: minimize necessary execution on recvpacket checktx (#6302)) sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/stretchr/testify/require" - "github.com/stretchr/testify/suite" -<<<<<<< HEAD clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" host "github.com/cosmos/ibc-go/v7/modules/core/24-host" "github.com/cosmos/ibc-go/v7/modules/core/ante" "github.com/cosmos/ibc-go/v7/modules/core/exported" ibctesting "github.com/cosmos/ibc-go/v7/testing" -======= - capabilitytypes "github.com/cosmos/ibc-go/modules/capability/types" - clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" - channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types" - commitmenttypes "github.com/cosmos/ibc-go/v8/modules/core/23-commitment/types" - host "github.com/cosmos/ibc-go/v8/modules/core/24-host" - "github.com/cosmos/ibc-go/v8/modules/core/ante" - "github.com/cosmos/ibc-go/v8/modules/core/exported" - ibctm "github.com/cosmos/ibc-go/v8/modules/light-clients/07-tendermint" - ibctesting "github.com/cosmos/ibc-go/v8/testing" ->>>>>>> 0993246f (perf: minimize necessary execution on recvpacket checktx (#6302)) ) type AnteTestSuite struct { @@ -361,20 +342,6 @@ func (suite *AnteTestSuite) TestAnteDecorator() { }, true, }, - { - "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 { @@ -458,11 +425,7 @@ func (suite *AnteTestSuite) TestAnteDecorator() { channeltypes.NewMsgRecvPacket(packet, []byte("proof"), clienttypes.NewHeight(1, 1), "signer"), } }, -<<<<<<< HEAD false, -======= - commitmenttypes.ErrInvalidProof, ->>>>>>> 0993246f (perf: minimize necessary execution on recvpacket checktx (#6302)) }, { "no success on one new message and one redundant message in the same block", @@ -495,7 +458,7 @@ func (suite *AnteTestSuite) TestAnteDecorator() { msg.Packet.DestinationPort = "invalid-port" return []sdk.Msg{msg} }, - capabilitytypes.ErrCapabilityNotFound, + false, }, }