Skip to content

Commit

Permalink
perf: minimize necessary execution on recvpacket checktx (#6302)
Browse files Browse the repository at this point in the history
* 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 0993246)

# Conflicts:
#	CHANGELOG.md
#	modules/core/ante/ante.go
#	modules/core/ante/ante_test.go
  • Loading branch information
colin-axner authored and mergify[bot] committed May 20, 2024
1 parent 48b8858 commit 2416048
Show file tree
Hide file tree
Showing 3 changed files with 134 additions and 1 deletion.
10 changes: 10 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,16 @@ Ref: https://keepachangelog.com/en/1.0.0/

### Improvements

<<<<<<< HEAD
=======
* (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

### Bug Fixes
Expand Down
76 changes: 76 additions & 0 deletions modules/core/ante/ante.go
Original file line number Diff line number Diff line change
Expand Up @@ -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++
}
Expand Down Expand Up @@ -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))
49 changes: 48 additions & 1 deletion modules/core/ante/ante_test.go
Original file line number Diff line number Diff line change
@@ -1,18 +1,38 @@
package ante_test

import (
"fmt"
"testing"

<<<<<<< HEAD

Check failure on line 7 in modules/core/ante/ante_test.go

View workflow job for this annotation

GitHub Actions / build (amd64)

expected 'STRING', found '<<'

Check failure on line 7 in modules/core/ante/ante_test.go

View workflow job for this annotation

GitHub Actions / build (arm)

expected 'STRING', found '<<'

Check failure on line 7 in modules/core/ante/ante_test.go

View workflow job for this annotation

GitHub Actions / build (arm64)

expected 'STRING', found '<<'
=======
"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 {
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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",
Expand All @@ -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 {
Expand Down

0 comments on commit 2416048

Please sign in to comment.