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

perform a no-op on redundant relay messages #268

Merged
merged 9 commits into from
Jul 20, 2021
Merged

perform a no-op on redundant relay messages #268

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
0ffbcec
create initial changes for delivertx handling
colin-axner Jul 15, 2021
990d6a8
handle closed channel no-ops, fix tests
colin-axner Jul 19, 2021
59ad180
self review nits
colin-axner Jul 19, 2021
f45469f
Merge branch 'main' into colin/245-delivertx-redundant-relay
colin-axner Jul 19, 2021
31a55f4
add changelog
colin-axner Jul 19, 2021
beb3d46
Merge branch 'colin/245-delivertx-redundant-relay' of github.com:cosm…
colin-axner Jul 19, 2021
d7443a5
fix merge conflict
colin-axner Jul 19, 2021
560ad17
add events for no op messages
colin-axner Jul 20, 2021
c23ae39
add back comment
colin-axner Jul 20, 2021
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
23 changes: 13 additions & 10 deletions modules/core/04-channel/keeper/packet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -249,10 +249,10 @@ func (k Keeper) RecvPacket(
// check if the packet receipt has been received already for unordered channels
_, found := k.GetPacketReceipt(ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
if found {
return sdkerrors.Wrapf(
types.ErrPacketReceived,
"packet sequence (%d)", packet.GetSequence(),
)
// This error indicates that the packet has already been relayed. Core IBC will
// treat this error as a no-op in order to prevent an entire relay transaction
// from failing and consuming unnecessary fees.
return types.ErrPacketReceived
}

// All verification complete, update state
Expand All @@ -271,12 +271,11 @@ func (k Keeper) RecvPacket(
)
}

// helpful error message for relayers
if packet.GetSequence() < nextSequenceRecv {
return sdkerrors.Wrapf(
types.ErrPacketReceived,
"packet sequence (%d), next sequence receive (%d)", packet.GetSequence(), nextSequenceRecv,
)
// This error indicates that the packet has already been relayed. Core IBC will
// treat this error as a no-op in order to prevent an entire relay transaction
// from failing and consuming unnecessary fees.
return types.ErrPacketReceived
}

if packet.GetSequence() != nextSequenceRecv {
Expand Down Expand Up @@ -480,7 +479,11 @@ func (k Keeper) AcknowledgePacket(
commitment := k.GetPacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())

if len(commitment) == 0 {
return sdkerrors.Wrapf(types.ErrPacketCommitmentNotFound, "packet with sequence (%d) has been acknowledged, or timed out. In rare cases, the packet referenced was never sent, likely due to the relayer being misconfigured", packet.GetSequence())
// This error indicates that the acknowledgement has already been relayed
// or there is a misconfigured relayer attempting to prove an acknowledgement
// for a packet never sent. Core IBC will treat this error as a no-op in order to
// prevent an entire relay transaction from failing and consuming unnecessary fees.
return types.ErrPacketCommitmentNotFound
}

packetCommitment := types.CommitPacket(k.cdc, packet)
Expand Down
14 changes: 13 additions & 1 deletion modules/core/04-channel/keeper/timeout.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,11 @@ func (k Keeper) TimeoutPacket(
commitment := k.GetPacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())

if len(commitment) == 0 {
return sdkerrors.Wrapf(types.ErrPacketCommitmentNotFound, "packet with sequence (%d) has been acknowledged or timed out. In rare cases, the packet referenced was never sent, likely due to the relayer being misconfigured", packet.GetSequence())
// This error indicates that the timeout has already been relayed
// or there is a misconfigured relayer attempting to prove a timeout
// for a packet never sent. Core IBC will treat this error as a no-op in order to
// prevent an entire relay transaction from failing and consuming unnecessary fees.
return types.ErrPacketCommitmentNotFound
}

packetCommitment := types.CommitPacket(k.cdc, packet)
Expand Down Expand Up @@ -222,6 +226,14 @@ func (k Keeper) TimeoutOnClose(

commitment := k.GetPacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())

if len(commitment) == 0 {
// This error indicates that the timeout has already been relayed
// or there is a misconfigured relayer attempting to prove a timeout
// for a packet never sent. Core IBC will treat this error as a no-op in order to
// prevent an entire relay transaction from failing and consuming unnecessary fees.
return types.ErrPacketCommitmentNotFound
}

packetCommitment := types.CommitPacket(k.cdc, packet)

// verify we sent the packet and haven't cleared it out yet
Expand Down
43 changes: 38 additions & 5 deletions modules/core/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -492,13 +492,21 @@ func (k Keeper) RecvPacket(goCtx context.Context, msg *channeltypes.MsgRecvPacke
}

// Perform TAO verification
if err := k.ChannelKeeper.RecvPacket(ctx, cap, msg.Packet, msg.ProofCommitment, msg.ProofHeight); err != nil {
// If the packet was already received we want to perform a no-op. We use a cached context to prevent
// accidental state changes
cacheCtx, writeFn := ctx.CacheContext()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this seems like the best solution to me. I need to return before performing the application callbacks, but I also don't want to skip some of the earlier IBC checks in case the sender misconfigured their msg (used wrong channel id or something like that). I also don't want to allow state changes to slip through

if err := k.ChannelKeeper.RecvPacket(cacheCtx, cap, msg.Packet, msg.ProofCommitment, msg.ProofHeight); err != nil {
// packet already received
if err == channeltypes.ErrPacketReceived {
return &channeltypes.MsgRecvPacketResponse{}, nil // no-op
}
return nil, sdkerrors.Wrap(err, "receive packet verification failed")
}
writeFn()

// Perform application logic callback
// Cache context so that we may discard state changes from callback if the acknowledgement is unsuccessful.
cacheCtx, writeFn := ctx.CacheContext()
cacheCtx, writeFn = ctx.CacheContext()
ack := cbs.OnRecvPacket(cacheCtx, msg.Packet, relayer)
// This doesn't cause duplicate events to be emitted.
// NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context.
Expand Down Expand Up @@ -556,9 +564,18 @@ func (k Keeper) Timeout(goCtx context.Context, msg *channeltypes.MsgTimeout) (*c
}

// Perform TAO verification
if err := k.ChannelKeeper.TimeoutPacket(ctx, msg.Packet, msg.ProofUnreceived, msg.ProofHeight, msg.NextSequenceRecv); err != nil {
// If the timeout was already received we want to perform a no-op. We use a cached context to prevent
// accidental state changes
cacheCtx, writeFn := ctx.CacheContext()
if err := k.ChannelKeeper.TimeoutPacket(cacheCtx, msg.Packet, msg.ProofUnreceived, msg.ProofHeight, msg.NextSequenceRecv); err != nil {
// timeout already received
// NOTE: ordered channels will error on a closed channel upon replay
colin-axner marked this conversation as resolved.
Show resolved Hide resolved
if err == channeltypes.ErrPacketCommitmentNotFound {
return &channeltypes.MsgTimeoutResponse{}, nil // no-op
}
return nil, sdkerrors.Wrap(err, "timeout packet verification failed")
}
writeFn()

// Perform application logic callback
err = cbs.OnTimeoutPacket(ctx, msg.Packet, relayer)
Expand Down Expand Up @@ -610,9 +627,17 @@ func (k Keeper) TimeoutOnClose(goCtx context.Context, msg *channeltypes.MsgTimeo
}

// Perform TAO verification
if err := k.ChannelKeeper.TimeoutOnClose(ctx, cap, msg.Packet, msg.ProofUnreceived, msg.ProofClose, msg.ProofHeight, msg.NextSequenceRecv); err != nil {
// If the timeout was already received we want to perform a no-op. We use a cached context to prevent
// accidental state changes
cacheCtx, writeFn := ctx.CacheContext()
if err := k.ChannelKeeper.TimeoutOnClose(cacheCtx, cap, msg.Packet, msg.ProofUnreceived, msg.ProofClose, msg.ProofHeight, msg.NextSequenceRecv); err != nil {
// timeout already received
if err == channeltypes.ErrPacketCommitmentNotFound {
return &channeltypes.MsgTimeoutOnCloseResponse{}, nil // no-op
}
return nil, sdkerrors.Wrap(err, "timeout on close packet verification failed")
}
writeFn()

// Perform application logic callback
// NOTE: MsgTimeout and MsgTimeoutOnClose use the same "OnTimeoutPacket"
Expand Down Expand Up @@ -666,9 +691,17 @@ func (k Keeper) Acknowledgement(goCtx context.Context, msg *channeltypes.MsgAckn
}

// Perform TAO verification
if err := k.ChannelKeeper.AcknowledgePacket(ctx, cap, msg.Packet, msg.Acknowledgement, msg.ProofAcked, msg.ProofHeight); err != nil {
// If the acknowledgement was already received we want to perform a no-op. We use a cached context to prevent
// accidental state changes
cacheCtx, writeFn := ctx.CacheContext()
if err := k.ChannelKeeper.AcknowledgePacket(cacheCtx, cap, msg.Packet, msg.Acknowledgement, msg.ProofAcked, msg.ProofHeight); err != nil {
// acknowledgement already received
if err == channeltypes.ErrPacketCommitmentNotFound {
return &channeltypes.MsgAcknowledgementResponse{}, nil // no-op
}
return nil, sdkerrors.Wrap(err, "acknowledge packet verification failed")
}
writeFn()

// Perform application logic callback
err = cbs.OnAcknowledgementPacket(ctx, msg.Packet, msg.Acknowledgement, relayer)
Expand Down
45 changes: 25 additions & 20 deletions modules/core/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ func (suite *KeeperTestSuite) TestHandleRecvPacket() {
suite.coordinator.Setup(path)
packet = channeltypes.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, 0)
}, false, false},
{"ORDERED: packet already received (replay)", func() {
{"successful no-op: ORDERED - packet already received (replay)", func() {
path.SetChannelOrdered()
suite.coordinator.Setup(path)
packet = channeltypes.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, 0)
Expand All @@ -151,8 +151,8 @@ func (suite *KeeperTestSuite) TestHandleRecvPacket() {

err = path.EndpointB.RecvPacket(packet)
suite.Require().NoError(err)
}, false, false},
{"UNORDERED: packet already received (replay)", func() {
}, true, true},
{"successful no-op: UNORDERED - packet already received (replay)", func() {
suite.coordinator.Setup(path)
packet = channeltypes.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, 0)

Expand All @@ -161,7 +161,7 @@ func (suite *KeeperTestSuite) TestHandleRecvPacket() {

err = path.EndpointB.RecvPacket(packet)
suite.Require().NoError(err)
}, false, false},
}, true, true},
}

for _, tc := range testCases {
Expand All @@ -185,9 +185,9 @@ func (suite *KeeperTestSuite) TestHandleRecvPacket() {
if tc.expPass {
suite.Require().NoError(err)

// replay should fail since state changes occur
// replay should not fail since it will be treated as a no-op
_, err := keeper.Keeper.RecvPacket(*suite.chainB.App.GetIBCKeeper(), sdk.WrapSDKContext(suite.chainB.GetContext()), msg)
suite.Require().Error(err)
suite.Require().NoError(err)

// check that callback state was handled correctly
_, exists := suite.chainB.GetSimApp().ScopedIBCMockKeeper.GetCapability(suite.chainB.GetContext(), ibctesting.MockCanaryCapabilityName)
Expand Down Expand Up @@ -293,7 +293,7 @@ func (suite *KeeperTestSuite) TestHandleAcknowledgePacket() {
err := path.EndpointA.SendPacket(packet)
suite.Require().NoError(err)
}, false},
{"ORDERED: packet already acknowledged (replay)", func() {
{"successful no-op: ORDERED - packet already acknowledged (replay)", func() {
suite.coordinator.Setup(path)
packet = channeltypes.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, 0)

Expand All @@ -305,8 +305,8 @@ func (suite *KeeperTestSuite) TestHandleAcknowledgePacket() {

err = path.EndpointA.AcknowledgePacket(packet, ibctesting.MockAcknowledgement)
suite.Require().NoError(err)
}, false},
{"UNORDERED: packet already acknowledged (replay)", func() {
}, true},
{"successful no-op: UNORDERED - packet already acknowledged (replay)", func() {
suite.coordinator.Setup(path)

packet = channeltypes.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, 0)
Expand All @@ -319,7 +319,7 @@ func (suite *KeeperTestSuite) TestHandleAcknowledgePacket() {

err = path.EndpointA.AcknowledgePacket(packet, ibctesting.MockAcknowledgement)
suite.Require().NoError(err)
}, false},
}, true},
}

for _, tc := range testCases {
Expand All @@ -341,9 +341,9 @@ func (suite *KeeperTestSuite) TestHandleAcknowledgePacket() {
if tc.expPass {
suite.Require().NoError(err)

// replay should an error
// replay should not error as it is treated as a no-op
_, err := keeper.Keeper.Acknowledgement(*suite.chainA.App.GetIBCKeeper(), sdk.WrapSDKContext(suite.chainA.GetContext()), msg)
suite.Require().Error(err)
suite.Require().NoError(err)

// verify packet commitment was deleted on source chain
has := suite.chainA.App.GetIBCKeeper().ChannelKeeper.HasPacketCommitment(suite.chainA.GetContext(), packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())
Expand Down Expand Up @@ -441,11 +441,11 @@ func (suite *KeeperTestSuite) TestHandleTimeoutPacket() {

packetKey = host.NextSequenceRecvKey(packet.GetDestPort(), packet.GetDestChannel())
}, false},
{"UNORDERED: packet not sent", func() {
{"successful no-op: UNORDERED - packet not sent", func() {
suite.coordinator.Setup(path)
packet = channeltypes.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, 0)
packetKey = host.PacketReceiptKey(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
}, false},
}, true},
}

for _, tc := range testCases {
Expand All @@ -466,9 +466,14 @@ func (suite *KeeperTestSuite) TestHandleTimeoutPacket() {
if tc.expPass {
suite.Require().NoError(err)

// replay should return an error
if path.EndpointA.ChannelConfig.Order == channeltypes.ORDERED {
// replay should return an error as replayed transaction error on the channel being closed
_, err := keeper.Keeper.Timeout(*suite.chainA.App.GetIBCKeeper(), sdk.WrapSDKContext(suite.chainA.GetContext()), msg)
suite.Require().Error(err)
}
// replay should not return an error as it is treated as a no-op
_, err := keeper.Keeper.Timeout(*suite.chainA.App.GetIBCKeeper(), sdk.WrapSDKContext(suite.chainA.GetContext()), msg)
suite.Require().Error(err)
suite.Require().NoError(err)

// verify packet commitment was deleted on source chain
has := suite.chainA.App.GetIBCKeeper().ChannelKeeper.HasPacketCommitment(suite.chainA.GetContext(), packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())
Expand Down Expand Up @@ -577,14 +582,14 @@ func (suite *KeeperTestSuite) TestHandleTimeoutOnClosePacket() {

packetKey = host.NextSequenceRecvKey(packet.GetDestPort(), packet.GetDestChannel())
}, false},
{"UNORDERED: packet not sent", func() {
{"successful no-op: UNORDERED - packet not sent", func() {
suite.coordinator.Setup(path)
packet = channeltypes.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, 0)
packetKey = host.PacketAcknowledgementKey(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())

// close counterparty channel
path.EndpointB.SetChannelClosed()
}, false},
}, true},
{"ORDERED: channel not closed", func() {
path.SetChannelOrdered()
suite.coordinator.Setup(path)
Expand Down Expand Up @@ -622,9 +627,9 @@ func (suite *KeeperTestSuite) TestHandleTimeoutOnClosePacket() {
if tc.expPass {
suite.Require().NoError(err)

// replay should return an error
// replay should not return an error as it will be treated as a no-op
_, err := keeper.Keeper.TimeoutOnClose(*suite.chainA.App.GetIBCKeeper(), sdk.WrapSDKContext(suite.chainA.GetContext()), msg)
suite.Require().Error(err)
suite.Require().NoError(err)

// verify packet commitment was deleted on source chain
has := suite.chainA.App.GetIBCKeeper().ChannelKeeper.HasPacketCommitment(suite.chainA.GetContext(), packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence())
Expand Down