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: minimize necessary execution on recvpacket checktx #6302

Merged
merged 5 commits into from
May 20, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 38 additions & 1 deletion modules/core/ante/ante.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package ante

import (
errorsmod "cosmossdk.io/errors"

sdk "github.com/cosmos/cosmos-sdk/types"

clienttypes "github.com/cosmos/ibc-go/v8/modules/core/02-client/types"
Expand Down Expand Up @@ -30,10 +32,19 @@ func (rrd RedundantRelayDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula
for _, m := range tx.GetMsgs() {
switch msg := m.(type) {
case *channeltypes.MsgRecvPacket:
response, err := rrd.k.RecvPacket(ctx, msg)
var (
response *channeltypes.MsgRecvPacketResponse
err error
)
if ctx.IsCheckTx() {
Copy link
Member

@damiannolan damiannolan May 13, 2024

Choose a reason for hiding this comment

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

So confusing to have isCheckTx/isReCheckTx and ExecMode() 😅

Just noting for reviewers that here we are in CheckTxMode explicitly because we've checked also !simulate above on L28.

On the v0.50+ line there is union between IsCheckTx=true and ExecMode == execModeSimulate (after cosmos/cosmos-sdk#20346). i.e. when IsCheckTx is true when in execModeSimulate (this is because simulateTx runs on the checkState multistore)

Copy link
Member

Choose a reason for hiding this comment

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

I guess this is where it should be the other way around like you mentioned @colin-axner. When recheckTx=true then checkTx=true... because ¯_(ツ)_/¯

response, err = rrd.recvPacketCheckTx(ctx, msg)
} else {
response, err = rrd.k.RecvPacket(ctx, msg)
}
if err != nil {
return ctx, err
}

if response.Result == channeltypes.NOOP {
redundancies++
}
Expand Down Expand Up @@ -90,3 +101,29 @@ func (rrd RedundantRelayDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula
}
return next(ctx, tx, simulate)
}

// 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
}
26 changes: 25 additions & 1 deletion modules/core/ante/ante_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package ante_test

import (
"fmt"
"testing"

"github.com/stretchr/testify/require"
Expand Down Expand Up @@ -46,7 +47,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 @@ -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 {
Expand Down Expand Up @@ -451,6 +466,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}
},
false,
},
}

for _, tc := range testCases {
Expand Down
Loading