From 010bfc6a171eba4c2aba3f36f364febb87990ef1 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:23:39 +0200 Subject: [PATCH 1/2] perf: exclude pruning from tendermint update client in ante handler execution (#6278) * performance: exit early on recvpacket to exclude app callbacks * perf: skip pruning on check tx and recheck tx * change fmt.Errorf to errors.New * fix: check execMode simulate in conditional * revert: recv packet changes * fix: account for simulation in pruning check * fix: reuse checkTx ctx * chore: add changelog --------- Co-authored-by: Damian Nolan (cherry picked from commit 67b23cd8ff542730dd51bedae46bea352a654fc0) # Conflicts: # modules/light-clients/07-tendermint/update_test.go --- CHANGELOG.md | 1 + modules/light-clients/07-tendermint/update.go | 6 +- .../07-tendermint/update_test.go | 78 +++++++++++++++++++ 3 files changed, 84 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 08c804e0f8d..49383a39f70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -63,6 +63,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ * (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/27-interchain-accounts) [\#6251](https://github.com/cosmos/ibc-go/pull/6251) Use `UNORDERED` as the default ordering for new ICA channels. * (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 diff --git a/modules/light-clients/07-tendermint/update.go b/modules/light-clients/07-tendermint/update.go index 59c050cc903..5c5fbca840c 100644 --- a/modules/light-clients/07-tendermint/update.go +++ b/modules/light-clients/07-tendermint/update.go @@ -138,7 +138,11 @@ func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, client return []exported.Height{} } - cs.pruneOldestConsensusState(ctx, cdc, clientStore) + // 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) + } // 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 1fe8c75e541..477845c3965 100644 --- a/modules/light-clients/07-tendermint/update_test.go +++ b/modules/light-clients/07-tendermint/update_test.go @@ -5,7 +5,13 @@ import ( storetypes "cosmossdk.io/store/types" +<<<<<<< HEAD tmtypes "github.com/cometbft/cometbft/types" +======= + sdk "github.com/cosmos/cosmos-sdk/types" + + cmttypes "github.com/cometbft/cometbft/types" +>>>>>>> 67b23cd8 (perf: exclude pruning from tendermint update client in ante handler execution (#6278)) clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" commitmenttypes "github.com/cosmos/ibc-go/v8/modules/core/23-commitment/types" @@ -523,6 +529,78 @@ 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()) + + 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") + } + } + + assertPrune(false) + + // simulation mode must prune to calculate gas correctly + ctx = ctx.WithExecMode(sdk.ExecModeSimulate) + lightClientModule.UpdateState(ctx, path.EndpointA.ClientID, createClientMessage()) + + assertPrune(true) +} + func (suite *TendermintTestSuite) TestPruneConsensusState() { // create path and setup clients path := ibctesting.NewPath(suite.chainA, suite.chainB) From 22029b07df90b19ca3f31a3385786a3ec2f61edb 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:50:25 +0200 Subject: [PATCH 2/2] fix: merge conflicts --- .../07-tendermint/update_test.go | 23 +++++++------------ 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/modules/light-clients/07-tendermint/update_test.go b/modules/light-clients/07-tendermint/update_test.go index 477845c3965..1176a053b9d 100644 --- a/modules/light-clients/07-tendermint/update_test.go +++ b/modules/light-clients/07-tendermint/update_test.go @@ -5,13 +5,9 @@ import ( storetypes "cosmossdk.io/store/types" -<<<<<<< HEAD - tmtypes "github.com/cometbft/cometbft/types" -======= sdk "github.com/cosmos/cosmos-sdk/types" - cmttypes "github.com/cometbft/cometbft/types" ->>>>>>> 67b23cd8 (perf: exclude pruning from tendermint update client in ante handler execution (#6278)) + tmtypes "github.com/cometbft/cometbft/types" clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types" commitmenttypes "github.com/cosmos/ibc-go/v8/modules/core/23-commitment/types" @@ -531,12 +527,10 @@ func (suite *TendermintTestSuite) TestUpdateState() { func (suite *TendermintTestSuite) TestUpdateStateCheckTx() { path := ibctesting.NewPath(suite.chainA, suite.chainB) - path.SetupClients() + suite.coordinator.SetupClients(path) 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) + header, err := suite.chainA.ConstructUpdateTMClientHeader(suite.chainB, path.EndpointA.ClientID) suite.Require().NoError(err) return header } @@ -554,17 +548,16 @@ func (suite *TendermintTestSuite) TestUpdateStateCheckTx() { // 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) - + cs := path.EndpointA.GetClientState() ctx = path.EndpointA.Chain.GetContext().WithIsCheckTx(true) - lightClientModule.UpdateState(ctx, path.EndpointA.ClientID, createClientMessage()) + + cs.UpdateState(ctx, suite.chainA.GetSimApp().AppCodec(), clientStore, 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()) + cs.UpdateState(ctx, suite.chainA.GetSimApp().AppCodec(), clientStore, createClientMessage()) assertPrune := func(pruned bool) { // check consensus states and associated metadata @@ -596,7 +589,7 @@ func (suite *TendermintTestSuite) TestUpdateStateCheckTx() { // simulation mode must prune to calculate gas correctly ctx = ctx.WithExecMode(sdk.ExecModeSimulate) - lightClientModule.UpdateState(ctx, path.EndpointA.ClientID, createClientMessage()) + cs.UpdateState(ctx, suite.chainA.GetSimApp().AppCodec(), clientStore, createClientMessage()) assertPrune(true) }