Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: exclude pruning from tendermint update client in ante handler execution #6278

Merged
merged 12 commits into from
May 20, 2024
Merged
15 changes: 15 additions & 0 deletions modules/core/ante/ante_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package ante_test

import (
"errors"
"testing"

"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -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(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 {
Expand Down
5 changes: 5 additions & 0 deletions modules/core/keeper/msg_server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()) && ctx.ExecMode() != sdk.ExecModeSimulate {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For backport to v7.5 we should just add a new func which plumbs through the simulate bool from ante. We can do similar to the checkTxUpdateClient method I added on the ante handler itself.

There is no ability to check runTxModeSimulate on the v0.47 line (before it was changed to "execMode")

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.
Expand Down
5 changes: 4 additions & 1 deletion modules/light-clients/07-tendermint/update.go
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,10 @@ func (cs ClientState) UpdateState(ctx sdk.Context, cdc codec.BinaryCodec, client
return []exported.Height{}
}

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 {
Expand Down
55 changes: 55 additions & 0 deletions modules/light-clients/07-tendermint/update_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -560,6 +560,61 @@ func (suite *TendermintTestSuite) TestUpdateState() {
}
}

func (suite *TendermintTestSuite) TestUpdateStateCheckTx() {
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
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)
Expand Down
Loading