From edeef9353681e7ffc219f30ba775de6f97198941 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 8 May 2024 16:19:05 +0200 Subject: [PATCH 1/8] performance: exit early on recvpacket to exclude app callbacks --- modules/core/ante/ante_test.go | 15 +++++++++++++++ modules/core/keeper/msg_server.go | 5 +++++ 2 files changed, 20 insertions(+) diff --git a/modules/core/ante/ante_test.go b/modules/core/ante/ante_test.go index 8f5071e13fe..150235c3181 100644 --- a/modules/core/ante/ante_test.go +++ b/modules/core/ante/ante_test.go @@ -1,6 +1,7 @@ package ante_test import ( + "fmt" "testing" "github.com/stretchr/testify/require" @@ -342,6 +343,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)} + }, + true, + }, { "no success on one redundant RecvPacket message", func(suite *AnteTestSuite) []sdk.Msg { diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index a30e96d1ed6..9084091a6e9 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -488,6 +488,11 @@ func (k *Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPack return nil, errorsmod.Wrap(err, "receive packet verification failed") } + // performance: return early for the redundant relayer ante handler + if ctx.IsCheckTx() || ctx.IsReCheckTx() { + return &channeltypes.MsgRecvPacketResponse{Result: channeltypes.SUCCESS}, nil + } + // Perform application logic callback // // Cache context so that we may discard state changes from callback if the acknowledgement is unsuccessful. From 9b9adbe11fbb982f5da50946c5ecaff73ac06ac4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 8 May 2024 16:42:48 +0200 Subject: [PATCH 2/8] perf: skip pruning on check tx and recheck tx --- modules/light-clients/07-tendermint/update.go | 5 +- .../07-tendermint/update_test.go | 55 +++++++++++++++++++ 2 files changed, 59 insertions(+), 1 deletion(-) diff --git a/modules/light-clients/07-tendermint/update.go b/modules/light-clients/07-tendermint/update.go index e01786a79d1..10a2ee2968f 100644 --- a/modules/light-clients/07-tendermint/update.go +++ b/modules/light-clients/07-tendermint/update.go @@ -137,7 +137,10 @@ func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, client panic(fmt.Errorf("expected type %T, got %T", &Header{}, clientMsg)) } - cs.pruneOldestConsensusState(ctx, cdc, clientStore) + // don't do prune logic in CheckTx + if !ctx.IsCheckTx() && !ctx.IsReCheckTx() { + cs.pruneOldestConsensusState(ctx, cdc, clientStore) + } // check for duplicate update if _, found := GetConsensusState(clientStore, cdc, header.GetHeight()); found { diff --git a/modules/light-clients/07-tendermint/update_test.go b/modules/light-clients/07-tendermint/update_test.go index b28b928babb..d81194a8dbf 100644 --- a/modules/light-clients/07-tendermint/update_test.go +++ b/modules/light-clients/07-tendermint/update_test.go @@ -557,6 +557,61 @@ func (suite *TendermintTestSuite) TestUpdateState() { } } +func (suite *TendermintTestSuite) TestUpdateStateCheckTx() { + path := ibctesting.NewPath(suite.chainA, suite.chainB) + path.SetupClients() + + createClientMessage := func() exported.ClientMessage { + trustedHeight, ok := path.EndpointA.GetClientLatestHeight().(clienttypes.Height) + suite.Require().True(ok) + header, err := path.EndpointB.Chain.IBCClientHeader(path.EndpointB.Chain.LatestCommittedHeader, trustedHeight) + suite.Require().NoError(err) + return header + } + + // get the first height as it will be pruned first. + var pruneHeight exported.Height + getFirstHeightCb := func(height exported.Height) bool { + pruneHeight = height + return true + } + ctx := path.EndpointA.Chain.GetContext() + clientStore := path.EndpointA.Chain.App.GetIBCKeeper().ClientKeeper.ClientStore(ctx, path.EndpointA.ClientID) + ibctm.IterateConsensusStateAscending(clientStore, getFirstHeightCb) + + // Increment the time by a week + suite.coordinator.IncrementTimeBy(7 * 24 * time.Hour) + + lightClientModule, found := suite.chainA.App.GetIBCKeeper().ClientKeeper.Route(path.EndpointA.ClientID) + suite.Require().True(found) + + ctx = path.EndpointA.Chain.GetContext().WithIsCheckTx(true) + lightClientModule.UpdateState(ctx, path.EndpointA.ClientID, createClientMessage()) + + // Increment the time by another week, then update the client. + // This will cause the first two consensus states to become expired. + suite.coordinator.IncrementTimeBy(7 * 24 * time.Hour) + ctx = path.EndpointA.Chain.GetContext().WithIsCheckTx(true) + lightClientModule.UpdateState(ctx, path.EndpointA.ClientID, createClientMessage()) + + // check that the first expired consensus state got deleted along with all associated metadata + consState, ok := path.EndpointA.Chain.GetConsensusState(path.EndpointA.ClientID, pruneHeight) + suite.Require().NotNil(consState, "expired consensus state pruned") + suite.Require().True(ok) + + // check processed time metadata is pruned + processTime, ok := ibctm.GetProcessedTime(clientStore, pruneHeight) + suite.Require().NotEqual(uint64(0), processTime, "processed time metadata pruned") + suite.Require().True(ok) + processHeight, ok := ibctm.GetProcessedHeight(clientStore, pruneHeight) + suite.Require().NotNil(processHeight, "processed height metadata pruned") + suite.Require().True(ok) + + // check iteration key metadata is pruned + consKey := ibctm.GetIterationKey(clientStore, pruneHeight) + suite.Require().NotNil(consKey, "iteration key pruned") +} + func (suite *TendermintTestSuite) TestPruneConsensusState() { // create path and setup clients path := ibctesting.NewPath(suite.chainA, suite.chainB) From b795527dc9ae4818d50d869e847ea9269196e16f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Wed, 8 May 2024 17:30:48 +0200 Subject: [PATCH 3/8] change fmt.Errorf to errors.New --- modules/core/ante/ante_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/core/ante/ante_test.go b/modules/core/ante/ante_test.go index 150235c3181..5eff72629de 100644 --- a/modules/core/ante/ante_test.go +++ b/modules/core/ante/ante_test.go @@ -1,7 +1,7 @@ package ante_test import ( - "fmt" + "errors" "testing" "github.com/stretchr/testify/require" @@ -349,7 +349,7 @@ func (suite *AnteTestSuite) TestAnteDecorator() { 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")) + panic(errors.New("failed OnRecvPacket mock callback")) } // the RecvPacket message has not been submitted to the chain yet, so it will succeed From 6c55babc13a2e683915a1eaa59d9fdbb93bf0786 Mon Sep 17 00:00:00 2001 From: Damian Nolan Date: Wed, 8 May 2024 18:14:37 +0200 Subject: [PATCH 4/8] fix: check execMode simulate in conditional --- modules/core/keeper/msg_server.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index 9084091a6e9..479198e4817 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -489,7 +489,7 @@ func (k *Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPack } // performance: return early for the redundant relayer ante handler - if ctx.IsCheckTx() || ctx.IsReCheckTx() { + if (ctx.IsCheckTx() || ctx.IsReCheckTx()) && ctx.ExecMode() != sdk.ExecModeSimulate { return &channeltypes.MsgRecvPacketResponse{Result: channeltypes.SUCCESS}, nil } From d93162345c0634af0aa079f25e03253106a3ac2e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 13 May 2024 15:31:46 +0200 Subject: [PATCH 5/8] revert: recv packet changes --- modules/core/ante/ante_test.go | 15 --------------- modules/core/keeper/msg_server.go | 5 ----- 2 files changed, 20 deletions(-) diff --git a/modules/core/ante/ante_test.go b/modules/core/ante/ante_test.go index 5eff72629de..8f5071e13fe 100644 --- a/modules/core/ante/ante_test.go +++ b/modules/core/ante/ante_test.go @@ -1,7 +1,6 @@ package ante_test import ( - "errors" "testing" "github.com/stretchr/testify/require" @@ -343,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(errors.New("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)} - }, - true, - }, { "no success on one redundant RecvPacket message", func(suite *AnteTestSuite) []sdk.Msg { diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index 479198e4817..a30e96d1ed6 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -488,11 +488,6 @@ func (k *Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPack return nil, errorsmod.Wrap(err, "receive packet verification failed") } - // performance: return early for the redundant relayer ante handler - if (ctx.IsCheckTx() || ctx.IsReCheckTx()) && ctx.ExecMode() != sdk.ExecModeSimulate { - return &channeltypes.MsgRecvPacketResponse{Result: channeltypes.SUCCESS}, nil - } - // Perform application logic callback // // Cache context so that we may discard state changes from callback if the acknowledgement is unsuccessful. From 62b5ca72bfed077a7fe1f3ae5b5a6cb4fdb9818c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 13 May 2024 15:49:35 +0200 Subject: [PATCH 6/8] fix: account for simulation in pruning check --- modules/light-clients/07-tendermint/update.go | 5 +- .../07-tendermint/update_test.go | 47 +++++++++++++------ 2 files changed, 36 insertions(+), 16 deletions(-) diff --git a/modules/light-clients/07-tendermint/update.go b/modules/light-clients/07-tendermint/update.go index b2e675b9833..9a710f9e686 100644 --- a/modules/light-clients/07-tendermint/update.go +++ b/modules/light-clients/07-tendermint/update.go @@ -139,8 +139,9 @@ func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, client return []exported.Height{} } - // don't do prune logic in CheckTx - if !ctx.IsCheckTx() && !ctx.IsReCheckTx() { + // performance: do not prune in checkTx + // simulation must prune for accurate gas estimation + if (!ctx.IsCheckTx() && !ctx.IsReCheckTx()) || ctx.ExecMode() == sdk.ExecModeSimulate { cs.pruneOldestConsensusState(ctx, cdc, clientStore) } diff --git a/modules/light-clients/07-tendermint/update_test.go b/modules/light-clients/07-tendermint/update_test.go index 02a9879b85c..3947e94d387 100644 --- a/modules/light-clients/07-tendermint/update_test.go +++ b/modules/light-clients/07-tendermint/update_test.go @@ -5,6 +5,8 @@ import ( storetypes "cosmossdk.io/store/types" + sdk "github.com/cosmos/cosmos-sdk/types" + cmttypes "github.com/cometbft/cometbft/types" clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" @@ -597,22 +599,39 @@ func (suite *TendermintTestSuite) TestUpdateStateCheckTx() { ctx = path.EndpointA.Chain.GetContext().WithIsCheckTx(true) lightClientModule.UpdateState(ctx, path.EndpointA.ClientID, createClientMessage()) - // check that the first expired consensus state got deleted along with all associated metadata - consState, ok := path.EndpointA.Chain.GetConsensusState(path.EndpointA.ClientID, pruneHeight) - suite.Require().NotNil(consState, "expired consensus state pruned") - suite.Require().True(ok) + assertPrune := func(pruned bool) { + // check consensus states and associated metadata + consState, ok := path.EndpointA.Chain.GetConsensusState(path.EndpointA.ClientID, pruneHeight) + suite.Require().Equal(!pruned, ok) + + processTime, ok := ibctm.GetProcessedTime(clientStore, pruneHeight) + suite.Require().Equal(!pruned, ok) + + processHeight, ok := ibctm.GetProcessedHeight(clientStore, pruneHeight) + suite.Require().Equal(!pruned, ok) + + consKey := ibctm.GetIterationKey(clientStore, pruneHeight) + + if pruned { + suite.Require().Nil(consState, "expired consensus state not pruned") + suite.Require().Empty(processTime, "processed time metadata not pruned") + suite.Require().Nil(processHeight, "processed height metadata not pruned") + suite.Require().Nil(consKey, "iteration key not pruned") + } else { + suite.Require().NotNil(consState, "expired consensus state pruned") + suite.Require().NotEqual(uint64(0), processTime, "processed time metadata pruned") + suite.Require().NotNil(processHeight, "processed height metadata pruned") + suite.Require().NotNil(consKey, "iteration key pruned") + } + } - // check processed time metadata is pruned - processTime, ok := ibctm.GetProcessedTime(clientStore, pruneHeight) - suite.Require().NotEqual(uint64(0), processTime, "processed time metadata pruned") - suite.Require().True(ok) - processHeight, ok := ibctm.GetProcessedHeight(clientStore, pruneHeight) - suite.Require().NotNil(processHeight, "processed height metadata pruned") - suite.Require().True(ok) + assertPrune(false) - // check iteration key metadata is pruned - consKey := ibctm.GetIterationKey(clientStore, pruneHeight) - suite.Require().NotNil(consKey, "iteration key pruned") + // simulation mode must prune to calculate gas correctly + ctx = path.EndpointA.Chain.GetContext().WithExecMode(sdk.ExecModeSimulate) + lightClientModule.UpdateState(ctx, path.EndpointA.ClientID, createClientMessage()) + + assertPrune(true) } func (suite *TendermintTestSuite) TestPruneConsensusState() { From b8ee7e327add158e31e61f5ca31bafb3a83a9dee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Colin=20Axn=C3=A9r?= <25233464+colin-axner@users.noreply.github.com> Date: Mon, 13 May 2024 15:58:00 +0200 Subject: [PATCH 7/8] fix: reuse checkTx ctx --- modules/light-clients/07-tendermint/update_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/light-clients/07-tendermint/update_test.go b/modules/light-clients/07-tendermint/update_test.go index 3947e94d387..178ffa6195b 100644 --- a/modules/light-clients/07-tendermint/update_test.go +++ b/modules/light-clients/07-tendermint/update_test.go @@ -628,7 +628,7 @@ func (suite *TendermintTestSuite) TestUpdateStateCheckTx() { assertPrune(false) // simulation mode must prune to calculate gas correctly - ctx = path.EndpointA.Chain.GetContext().WithExecMode(sdk.ExecModeSimulate) + ctx = ctx.WithExecMode(sdk.ExecModeSimulate) lightClientModule.UpdateState(ctx, path.EndpointA.ClientID, createClientMessage()) assertPrune(true) From 7f0324736d360ce82f400d42d914fdd135fabbba 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:13:13 +0200 Subject: [PATCH 8/8] chore: add changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5506ddc1cd6..64e4b71993a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,6 +65,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (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. ### Features