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

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 2 commits
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.ErrNoOpMsg
}

// 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.ErrNoOpMsg
}

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.ErrNoOpMsg
}

packetCommitment := types.CommitPacket(k.cdc, packet)
Expand Down
20 changes: 10 additions & 10 deletions modules/core/04-channel/keeper/packet_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,8 +267,8 @@ func (suite *KeeperTestSuite) TestRecvPacket() {
// attempts to receive packet 2 without receiving packet 1
channelCap = suite.chainB.GetChannelCapability(path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID)
}, true},
{"packet already relayed ORDERED channel", func() {
expError = types.ErrPacketReceived
{"packet already relayed ORDERED channel (no-op)", func() {
expError = types.ErrNoOpMsg

path.SetChannelOrdered()
suite.coordinator.Setup(path)
Expand All @@ -281,8 +281,8 @@ func (suite *KeeperTestSuite) TestRecvPacket() {
err = path.EndpointB.RecvPacket(packet.(types.Packet))
suite.Require().NoError(err)
}, false},
{"packet already relayed UNORDERED channel", func() {
expError = types.ErrPacketReceived
{"packet already relayed UNORDERED channel (no-op)", func() {
expError = types.ErrNoOpMsg

// setup uses an UNORDERED channel
suite.coordinator.Setup(path)
Expand Down Expand Up @@ -428,7 +428,7 @@ func (suite *KeeperTestSuite) TestRecvPacket() {
path.EndpointB.UpdateClient()
}, false},
{"receipt already stored", func() {
expError = types.ErrPacketReceived
expError = types.ErrNoOpMsg
suite.coordinator.Setup(path)

packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
Expand Down Expand Up @@ -617,8 +617,8 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() {

channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
}, true},
{"packet already acknowledged ordered channel", func() {
expError = types.ErrPacketCommitmentNotFound
{"packet already acknowledged ordered channel (no-op)", func() {
expError = types.ErrNoOpMsg

path.SetChannelOrdered()
suite.coordinator.Setup(path)
Expand All @@ -636,8 +636,8 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() {
err = path.EndpointA.AcknowledgePacket(packet, ack.Acknowledgement())
suite.Require().NoError(err)
}, false},
{"packet already acknowledged unordered channel", func() {
expError = types.ErrPacketCommitmentNotFound
{"packet already acknowledged unordered channel (no-op)", func() {
expError = types.ErrNoOpMsg

// setup uses an UNORDERED channel
suite.coordinator.Setup(path)
Expand Down Expand Up @@ -738,7 +738,7 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() {
channelCap = suite.chainA.GetChannelCapability(path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID)
}, false},
{"packet hasn't been sent", func() {
expError = types.ErrPacketCommitmentNotFound
expError = types.ErrNoOpMsg

// packet commitment never written
suite.coordinator.Setup(path)
Expand Down
28 changes: 20 additions & 8 deletions modules/core/04-channel/keeper/timeout.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,6 @@ func (k Keeper) TimeoutPacket(
)
}

if channel.State != types.OPEN {
return sdkerrors.Wrapf(
types.ErrInvalidChannelState,
"channel state is not OPEN (got %s)", channel.State.String(),
)
}

Comment on lines -37 to -43
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If a packet is timed out on an ordered channel, all future timeout packets need to be timed out via timeout on close (since the channel is closed). I want this "channel closed" error message to be sent to relayers only when the relay isn't redundant (packet commitment doesn't exist). Thus I moved it below the redundant check

Copy link
Member

Choose a reason for hiding this comment

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

This is so if there's a redundant timeout for the first timed out packet on an ORDERED channel, the relayer gets a no-op rather than this error?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's correct. When I did the change though I was thinking more packets would return a no op here. I'm fine to revert this in order to keep consistency with the spec. Thoughts?

// NOTE: TimeoutPacket is called by the AnteHandler which acts upon the packet.Route(),
// so the capability authentication can be omitted here

Expand Down Expand Up @@ -81,7 +74,18 @@ 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.ErrNoOpMsg
}

if channel.State != types.OPEN {
return sdkerrors.Wrapf(
types.ErrInvalidChannelState,
"channel state is not OPEN (got %s)", channel.State.String(),
)
}

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.ErrNoOpMsg
}

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

// verify we sent the packet and haven't cleared it out yet
Expand Down
24 changes: 15 additions & 9 deletions modules/core/04-channel/keeper/timeout_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() {
path.EndpointA.UpdateClient()
}, true},
{"packet already timed out: ORDERED", func() {
expError = types.ErrInvalidChannelState
expError = types.ErrNoOpMsg
ordered = true
path.SetChannelOrdered()

Expand All @@ -62,7 +62,7 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() {
suite.Require().NoError(err)
}, false},
{"packet already timed out: UNORDERED", func() {
expError = types.ErrPacketCommitmentNotFound
expError = types.ErrNoOpMsg
ordered = false

suite.coordinator.Setup(path)
Expand All @@ -83,9 +83,13 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() {
{"channel not open", func() {
expError = types.ErrInvalidChannelState
suite.coordinator.Setup(path)
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, timeoutHeight, disabledTimeoutTimestamp)
packet = types.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, path.EndpointA.GetClientState().GetLatestHeight().Increment().(clienttypes.Height), disabledTimeoutTimestamp)
err := path.EndpointA.SendPacket(packet)
suite.Require().NoError(err)
// need to update chainA's client representing chainB to prove missing ack
path.EndpointA.UpdateClient()

err := path.EndpointA.SetChannelClosed()
err = path.EndpointA.SetChannelClosed()
suite.Require().NoError(err)
}, false},
{"packet destination port ≠ channel counterparty port", func() {
Expand Down Expand Up @@ -130,7 +134,7 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() {
path.EndpointA.UpdateClient()
}, false},
{"packet hasn't been sent", func() {
expError = types.ErrPacketCommitmentNotFound
expError = types.ErrNoOpMsg
ordered = true
path.SetChannelOrdered()

Expand Down Expand Up @@ -182,10 +186,12 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() {
orderedPacketKey := host.NextSequenceRecvKey(packet.GetDestPort(), packet.GetDestChannel())
unorderedPacketKey := host.PacketReceiptKey(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())

if ordered {
proof, proofHeight = suite.chainB.QueryProof(orderedPacketKey)
} else {
proof, proofHeight = suite.chainB.QueryProof(unorderedPacketKey)
if path.EndpointB.ConnectionID != "" {
if ordered {
proof, proofHeight = path.EndpointB.QueryProof(orderedPacketKey)
} else {
proof, proofHeight = path.EndpointB.QueryProof(unorderedPacketKey)
}
}

err := suite.chainA.App.GetIBCKeeper().ChannelKeeper.TimeoutPacket(suite.chainA.GetContext(), packet, proof, proofHeight, nextSeqRecv)
Expand Down
3 changes: 3 additions & 0 deletions modules/core/04-channel/types/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,7 @@ var (

// ORDERED channel error
ErrPacketSequenceOutOfOrder = sdkerrors.Register(SubModuleName, 21, "packet sequence is out of order")

// Perform a no-op on the current Msg
ErrNoOpMsg = sdkerrors.Register(SubModuleName, 22, "message is redundant, perform no-op")
)
48 changes: 43 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,23 @@ 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
// 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.ErrNoOpMsg {
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 +566,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, perform a no-op
// 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
if err == channeltypes.ErrNoOpMsg {
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,11 +629,21 @@ 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, perform a no-op
// 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.ErrNoOpMsg {
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"
// application logic callback.
err = cbs.OnTimeoutPacket(ctx, msg.Packet, relayer)
Expand Down Expand Up @@ -666,9 +695,18 @@ 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
// 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.ErrNoOpMsg {
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
Loading