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.

Sign up for GitHub

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

Jump to bottom

perf: minimize necessary execution on recvpacket checktx #6302

Merged
merged 5 commits into from
May 20, 2024
Merged

perf: minimize necessary execution on recvpacket checktx #6302

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a94d49e
perf: only perform core ibc logic on recvpacket checktx
colin-axner May 13, 2024
cff8305
try me linter
colin-axner May 13, 2024
16ca629
fix: reorder if and add comment
colin-axner May 16, 2024
7c685b3
Merge branch 'main' of github.com:cosmos/ibc-go into colin/6232-recvp…
colin-axner May 20, 2024
95c955f
chore: add changelog entry
colin-axner May 20, 2024
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
View file
Open in desktop
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 •
edited
Loading

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
}
24 changes: 24 additions & 0 deletions modules/core/ante/ante_test.go
View file
Open in desktop
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 @@ -342,6 +343,20 @@
},
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 @@
},
false,
},
{
"no success on recvPacket checkTx, no capability found",
func(suite *AnteTestSuite) []sdk.Msg {
msg := suite.createRecvPacketMessage(false).(*channeltypes.MsgRecvPacket)

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

View workflow job for this annotation

GitHub Actions / lint 

unchecked-type-assertion: type cast result is unchecked in suite.createRecvPacketMessage(false).(*channeltypes.MsgRecvPacket) - type assertion will panic if not matched (revive)
msg.Packet.DestinationPort = "invalid-port"
return []sdk.Msg{msg}
},
false,
},
}

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