Skip to content
New issue

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

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

Already on GitHub? # to your account

perf: minimize logic on rechecktx for recvpacket #6280

Merged
merged 18 commits into from
May 21, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
041e66c
perf: minimize logic on rechecktx for recvpacket
colin-axner May 8, 2024
c0e4496
refactor: rework layout for recvpacket rechecktx
colin-axner May 13, 2024
9d70011
test: add tests for 04-channel rechecktx func
colin-axner May 13, 2024
3b521c7
test: add tests for core ante handler
colin-axner May 13, 2024
d972605
Merge branch 'main' into colin/6232-perf-rechecktx-recvpacket
colin-axner May 13, 2024
5cf09c7
chore: add comment explaining is rechecktx usage
colin-axner May 16, 2024
41b2dfb
linter appeasement
colin-axner May 16, 2024
2bd15dd
Merge branch 'colin/6232-perf-rechecktx-recvpacket' of github.com:cos…
colin-axner May 16, 2024
5917ec3
Merge branch 'main' of github.com:cosmos/ibc-go into colin/6232-perf-…
colin-axner May 16, 2024
dfcf58b
Merge branch 'main' of github.com:cosmos/ibc-go into colin/6232-perf-…
colin-axner May 20, 2024
252be4b
chore: add changelog entry
colin-axner May 20, 2024
cfeed70
Update modules/core/ante/ante.go
colin-axner May 20, 2024
0e6287c
imp: use cached ctx for consistency
colin-axner May 20, 2024
5bd6933
refactor: change added test to use expected errors
colin-axner May 20, 2024
ec73618
Merge branch 'colin/6232-perf-rechecktx-recvpacket' of github.com:cos…
colin-axner May 20, 2024
a3283b7
lint
colin-axner May 20, 2024
f5abe9d
Merge branch 'main' into colin/6232-perf-rechecktx-recvpacket
colin-axner May 20, 2024
2a83e8d
Merge branch 'main' into colin/6232-perf-rechecktx-recvpacket
colin-axner May 21, 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
22 changes: 22 additions & 0 deletions modules/core/04-channel/keeper/ante.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package keeper

import (
errorsmod "cosmossdk.io/errors"
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"
)

// RecvPacketReCheckTx applies replay protection ensuring that when relay messages are
// re-executed in ReCheckTx, we can appropriately filter out redundant relay transactions.
func (k *Keeper) RecvPacketReCheckTx(ctx sdk.Context, packet types.Packet) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wondering if there would be an alternative way to implement this where we don't need to increase the public API of the channel keeper...

Also, for my understanding: why what we do in ReCheckTx is different from what we do in CheckTx?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Outside of copy pasting the code, you would need to increase the public API of the channel keeper. I do not recommend copy pasting the code due to the codes sensitivity. It'd be nice to limit the channel keeper API, but we need to do some restructuring and make use of the internal package more. I'd say we should start by addressing #19 and moving the respective msg servers to their respective keepers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ReCheckTx is rerun on the mempool after every block commit. It is redundant to recheck proofs and other logic which will not change between commits

channel, found := k.GetChannel(ctx, packet.GetDestPort(), packet.GetDestChannel())
if !found {
return errorsmod.Wrap(types.ErrChannelNotFound, packet.GetDestChannel())
}

if err := k.applyReplayProtection(ctx, packet, channel); err != nil {
return err
}

return nil
}
66 changes: 66 additions & 0 deletions modules/core/04-channel/keeper/ante_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
package keeper_test

import (
"github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"

Check failure on line 4 in modules/core/04-channel/keeper/ante_test.go

View workflow job for this annotation

GitHub Actions / lint

ST1019: package "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types" is being imported more than once (stylecheck)
channeltypes "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types"

Check failure on line 5 in modules/core/04-channel/keeper/ante_test.go

View workflow job for this annotation

GitHub Actions / lint

duplicated-imports: Package "github.com/cosmos/ibc-go/v8/modules/core/04-channel/types" already imported (revive)
ibctesting "github.com/cosmos/ibc-go/v8/testing"
)

func (suite *KeeperTestSuite) TestRecvPacketReCheckTx() {
var (
path *ibctesting.Path
packet types.Packet
)

testCases := []struct {
name string
malleate func()
expError error
}{
{
"success",
func() {},
nil,
},
{
"channel not found",
func() {
packet.DestinationPort = "invalid-port"

Check failure on line 28 in modules/core/04-channel/keeper/ante_test.go

View workflow job for this annotation

GitHub Actions / lint

string `invalid-port` has 3 occurrences, make it a constant (goconst)
},
channeltypes.ErrChannelNotFound,
},
{
"redundant relay",
func() {
err := suite.chainB.App.GetIBCKeeper().ChannelKeeper.RecvPacketReCheckTx(suite.chainB.GetContext(), packet)
suite.Require().NoError(err)
},
channeltypes.ErrNoOpMsg,
},
}

for _, tc := range testCases {
tc := tc
suite.Run(tc.name, func() {
suite.SetupTest() // reset
path = ibctesting.NewPath(suite.chainA, suite.chainB)
path.Setup()

sequence, err := path.EndpointA.SendPacket(defaultTimeoutHeight, disabledTimeoutTimestamp, ibctesting.MockPacketData)
suite.Require().NoError(err)
packet = types.NewPacket(ibctesting.MockPacketData, sequence, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, defaultTimeoutHeight, disabledTimeoutTimestamp)

tc.malleate()

err = suite.chainB.App.GetIBCKeeper().ChannelKeeper.RecvPacketReCheckTx(suite.chainB.GetContext(), packet)

expPass := tc.expError == nil
if expPass {
suite.Require().NoError(err)
} else {
suite.Require().ErrorIs(err, tc.expError)
}
})
}

}
36 changes: 23 additions & 13 deletions modules/core/04-channel/keeper/packet.go
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,29 @@ func (k *Keeper) RecvPacket(
return errorsmod.Wrap(err, "couldn't verify counterparty packet commitment")
}

if err := k.applyReplayProtection(ctx, packet, channel); err != nil {
return err
}

// log that a packet has been received & executed
k.Logger(ctx).Info(
"packet received",
"sequence", strconv.FormatUint(packet.GetSequence(), 10),
"src_port", packet.GetSourcePort(),
"src_channel", packet.GetSourceChannel(),
"dst_port", packet.GetDestPort(),
"dst_channel", packet.GetDestChannel(),
)

// emit an event that the relayer can query for
emitRecvPacketEvent(ctx, packet, channel)

return nil
}

// applyReplayProtection ensures a packet has not already been received
// and performs the necessary state changes to ensure it cannot be received again.
func (k *Keeper) applyReplayProtection(ctx sdk.Context, packet types.Packet, channel types.Channel) error {
// REPLAY PROTECTION: The recvStartSequence will prevent historical proofs from allowing replay
// attacks on packets processed in previous lifecycles of a channel. After a successful channel
// upgrade all packets under the recvStartSequence will have been processed and thus should be
Expand Down Expand Up @@ -253,19 +276,6 @@ func (k *Keeper) RecvPacket(
k.SetNextSequenceRecv(ctx, packet.GetDestPort(), packet.GetDestChannel(), nextSequenceRecv)
}

// log that a packet has been received & executed
k.Logger(ctx).Info(
"packet received",
"sequence", strconv.FormatUint(packet.GetSequence(), 10),
"src_port", packet.GetSourcePort(),
"src_channel", packet.GetSourceChannel(),
"dst_port", packet.GetDestPort(),
"dst_channel", packet.GetDestChannel(),
)

// emit an event that the relayer can query for
emitRecvPacketEvent(ctx, packet, channel)

return nil
}

Expand Down
28 changes: 27 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.IsReCheckTx() {
response, err = rrd.recvPacketReCheckTx(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,18 @@ func (rrd RedundantRelayDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simula
}
return next(ctx, tx, simulate)
}

// recvPacketReCheckTx 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) recvPacketReCheckTx(ctx sdk.Context, msg *channeltypes.MsgRecvPacket) (*channeltypes.MsgRecvPacketResponse, error) {
err := rrd.k.ChannelKeeper.RecvPacketReCheckTx(ctx, msg.Packet)

switch err {
case nil:
return &channeltypes.MsgRecvPacketResponse{Result: channeltypes.SUCCESS}, nil
case channeltypes.ErrNoOpMsg:
return &channeltypes.MsgRecvPacketResponse{Result: channeltypes.NOOP}, nil
default:
return nil, errorsmod.Wrap(err, "receive packet verification failed")
}
}
97 changes: 95 additions & 2 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 (
"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 @@ -174,7 +175,7 @@ func (suite *AnteTestSuite) createUpdateClientMessage() sdk.Msg {
return msg
}

func (suite *AnteTestSuite) TestAnteDecorator() {
func (suite *AnteTestSuite) TestAnteDecoratorCheckTx() {
testCases := []struct {
name string
malleate func(suite *AnteTestSuite) []sdk.Msg
Expand Down Expand Up @@ -488,3 +489,95 @@ func (suite *AnteTestSuite) TestAnteDecorator() {
})
}
}

func (suite *AnteTestSuite) TestAnteDecoratorReCheckTx() {
testCases := []struct {
name string
malleate func(suite *AnteTestSuite) []sdk.Msg
expPass bool
}{
{
"success on one new RecvPacket message",
func(suite *AnteTestSuite) []sdk.Msg {
// the RecvPacket message has not been submitted to the chain yet, so it will succeed
return []sdk.Msg{suite.createRecvPacketMessage(false)}
},
true,
},
{
"success on one redundant and one new RecvPacket message",
func(suite *AnteTestSuite) []sdk.Msg {
return []sdk.Msg{
suite.createRecvPacketMessage(true),
suite.createRecvPacketMessage(false),
}
},
true,
},
{
"success on invalid proof (proof checks occur in checkTx)",
func(suite *AnteTestSuite) []sdk.Msg {
msg := suite.createRecvPacketMessage(false)
msg.ProofCommitment = []byte("invalid-proof")
return []sdk.Msg{msg}
},
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 {
return []sdk.Msg{suite.createRecvPacketMessage(true)}
},
false,
},
}

for _, tc := range testCases {
tc := tc

suite.Run(tc.name, func() {
// reset suite
suite.SetupTest()

k := suite.chainB.App.GetIBCKeeper()
decorator := ante.NewRedundantRelayDecorator(k)

msgs := tc.malleate(suite)

deliverCtx := suite.chainB.GetContext().WithIsCheckTx(false)
reCheckCtx := suite.chainB.GetContext().WithIsReCheckTx(true)

// create multimsg tx
txBuilder := suite.chainB.TxConfig.NewTxBuilder()
err := txBuilder.SetMsgs(msgs...)
suite.Require().NoError(err)
tx := txBuilder.GetTx()

next := func(ctx sdk.Context, tx sdk.Tx, simulate bool) (newCtx sdk.Context, err error) { return ctx, nil }

_, err = decorator.AnteHandle(deliverCtx, tx, false, next)
suite.Require().NoError(err, "antedecorator should not error on DeliverTx")

_, err = decorator.AnteHandle(reCheckCtx, tx, false, next)
if tc.expPass {
suite.Require().NoError(err, "non-strict decorator did not pass as expected")
} else {
suite.Require().Error(err, "non-strict antehandler did not return error as expected")
}
})
}
}
Loading