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

perform a no-op on redundant relay messages #268

Merged
merged 9 commits into from
Jul 20, 2021
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
## [Unreleased]

* (core) [\#227](https://github.com/cosmos/ibc-go/pull/227) Remove sdk.Result from application callbacks
* (core) [\#268](https://github.com/cosmos/ibc-go/pull/268) Perform a no-op on redundant relay messages. Previous behaviour returned an error. Now no state change will occur and no error will be returned.


## [v1.0.0-rc0](https://github.com/cosmos/ibc-go/releases/tag/v1.0.0-rc0) - 2021-07-07
Expand Down
85 changes: 85 additions & 0 deletions modules/core/04-channel/keeper/events.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
package keeper

import (
"encoding/hex"
"fmt"

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

"github.com/cosmos/ibc-go/modules/core/04-channel/types"
"github.com/cosmos/ibc-go/modules/core/exported"
)

// EmitRecvPacketEvent emits a receive packet event. It will be emitted both the first time a packet
// is received for a certain sequence and for all duplicate receives.
func EmitRecvPacketEvent(ctx sdk.Context, packet exported.PacketI, channel types.Channel) {
ctx.EventManager().EmitEvents(sdk.Events{
sdk.NewEvent(
types.EventTypeRecvPacket,
sdk.NewAttribute(types.AttributeKeyData, string(packet.GetData())), // DEPRECATED
sdk.NewAttribute(types.AttributeKeyDataHex, hex.EncodeToString(packet.GetData())),
sdk.NewAttribute(types.AttributeKeyTimeoutHeight, packet.GetTimeoutHeight().String()),
sdk.NewAttribute(types.AttributeKeyTimeoutTimestamp, fmt.Sprintf("%d", packet.GetTimeoutTimestamp())),
sdk.NewAttribute(types.AttributeKeySequence, fmt.Sprintf("%d", packet.GetSequence())),
sdk.NewAttribute(types.AttributeKeySrcPort, packet.GetSourcePort()),
sdk.NewAttribute(types.AttributeKeySrcChannel, packet.GetSourceChannel()),
sdk.NewAttribute(types.AttributeKeyDstPort, packet.GetDestPort()),
sdk.NewAttribute(types.AttributeKeyDstChannel, packet.GetDestChannel()),
sdk.NewAttribute(types.AttributeKeyChannelOrdering, channel.Ordering.String()),
// we only support 1-hop packets now, and that is the most important hop for a relayer
// (is it going to a chain I am connected to)
sdk.NewAttribute(types.AttributeKeyConnection, channel.ConnectionHops[0]),
),
sdk.NewEvent(
sdk.EventTypeMessage,
sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
),
})
}

// EmitAcknowledgePacketEvent emits an acknowledge packet event. It will be emitted both the first time
// a packet is acknowledged for a certain sequence and for all duplicate acknowledgements.
func EmitAcknowledgePacketEvent(ctx sdk.Context, packet exported.PacketI, channel types.Channel) {
ctx.EventManager().EmitEvents(sdk.Events{
sdk.NewEvent(
types.EventTypeAcknowledgePacket,
sdk.NewAttribute(types.AttributeKeyTimeoutHeight, packet.GetTimeoutHeight().String()),
sdk.NewAttribute(types.AttributeKeyTimeoutTimestamp, fmt.Sprintf("%d", packet.GetTimeoutTimestamp())),
sdk.NewAttribute(types.AttributeKeySequence, fmt.Sprintf("%d", packet.GetSequence())),
sdk.NewAttribute(types.AttributeKeySrcPort, packet.GetSourcePort()),
sdk.NewAttribute(types.AttributeKeySrcChannel, packet.GetSourceChannel()),
sdk.NewAttribute(types.AttributeKeyDstPort, packet.GetDestPort()),
sdk.NewAttribute(types.AttributeKeyDstChannel, packet.GetDestChannel()),
sdk.NewAttribute(types.AttributeKeyChannelOrdering, channel.Ordering.String()),
// we only support 1-hop packets now, and that is the most important hop for a relayer
// (is it going to a chain I am connected to)
sdk.NewAttribute(types.AttributeKeyConnection, channel.ConnectionHops[0]),
),
sdk.NewEvent(
sdk.EventTypeMessage,
sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
),
})
}

// EmitTimeoutPacketEvent emits a timeout packet event. It will be emitted both the first time a packet
// is timed out for a certain sequence and for all duplicate timeouts.
func EmitTimeoutPacketEvent(ctx sdk.Context, packet exported.PacketI, channel types.Channel) {
ctx.EventManager().EmitEvents(sdk.Events{
sdk.NewEvent(
types.EventTypeTimeoutPacket,
sdk.NewAttribute(types.AttributeKeyTimeoutHeight, packet.GetTimeoutHeight().String()),
sdk.NewAttribute(types.AttributeKeyTimeoutTimestamp, fmt.Sprintf("%d", packet.GetTimeoutTimestamp())),
sdk.NewAttribute(types.AttributeKeySequence, fmt.Sprintf("%d", packet.GetSequence())),
sdk.NewAttribute(types.AttributeKeySrcPort, packet.GetSourcePort()),
sdk.NewAttribute(types.AttributeKeySrcChannel, packet.GetSourceChannel()),
sdk.NewAttribute(types.AttributeKeyDstPort, packet.GetDestPort()),
sdk.NewAttribute(types.AttributeKeyDstChannel, packet.GetDestChannel()),
sdk.NewAttribute(types.AttributeKeyChannelOrdering, channel.Ordering.String()),
),
sdk.NewEvent(
sdk.EventTypeMessage,
sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
),
})
}
70 changes: 18 additions & 52 deletions modules/core/04-channel/keeper/packet.go
Original file line number Diff line number Diff line change
Expand Up @@ -249,10 +249,11 @@ 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(),
)
EmitRecvPacketEvent(ctx, packet, channel)
// 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 +272,12 @@ 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,
)
EmitRecvPacketEvent(ctx, packet, channel)
// 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 All @@ -300,28 +301,7 @@ func (k Keeper) RecvPacket(
k.Logger(ctx).Info("packet received", "packet", fmt.Sprintf("%v", packet))

// emit an event that the relayer can query for
ctx.EventManager().EmitEvents(sdk.Events{
sdk.NewEvent(
types.EventTypeRecvPacket,
sdk.NewAttribute(types.AttributeKeyData, string(packet.GetData())), // DEPRECATED
sdk.NewAttribute(types.AttributeKeyDataHex, hex.EncodeToString(packet.GetData())),
sdk.NewAttribute(types.AttributeKeyTimeoutHeight, packet.GetTimeoutHeight().String()),
sdk.NewAttribute(types.AttributeKeyTimeoutTimestamp, fmt.Sprintf("%d", packet.GetTimeoutTimestamp())),
sdk.NewAttribute(types.AttributeKeySequence, fmt.Sprintf("%d", packet.GetSequence())),
sdk.NewAttribute(types.AttributeKeySrcPort, packet.GetSourcePort()),
sdk.NewAttribute(types.AttributeKeySrcChannel, packet.GetSourceChannel()),
sdk.NewAttribute(types.AttributeKeyDstPort, packet.GetDestPort()),
sdk.NewAttribute(types.AttributeKeyDstChannel, packet.GetDestChannel()),
sdk.NewAttribute(types.AttributeKeyChannelOrdering, channel.Ordering.String()),
// we only support 1-hop packets now, and that is the most important hop for a relayer
// (is it going to a chain I am connected to)
sdk.NewAttribute(types.AttributeKeyConnection, channel.ConnectionHops[0]),
),
sdk.NewEvent(
sdk.EventTypeMessage,
sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
),
})
EmitRecvPacketEvent(ctx, packet, channel)

return nil
}
Expand Down Expand Up @@ -480,7 +460,12 @@ 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())
EmitAcknowledgePacketEvent(ctx, packet, channel)
// 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 Expand Up @@ -530,26 +515,7 @@ func (k Keeper) AcknowledgePacket(
k.Logger(ctx).Info("packet acknowledged", "packet", fmt.Sprintf("%v", packet))

// emit an event marking that we have processed the acknowledgement
ctx.EventManager().EmitEvents(sdk.Events{
sdk.NewEvent(
types.EventTypeAcknowledgePacket,
sdk.NewAttribute(types.AttributeKeyTimeoutHeight, packet.GetTimeoutHeight().String()),
sdk.NewAttribute(types.AttributeKeyTimeoutTimestamp, fmt.Sprintf("%d", packet.GetTimeoutTimestamp())),
sdk.NewAttribute(types.AttributeKeySequence, fmt.Sprintf("%d", packet.GetSequence())),
sdk.NewAttribute(types.AttributeKeySrcPort, packet.GetSourcePort()),
sdk.NewAttribute(types.AttributeKeySrcChannel, packet.GetSourceChannel()),
sdk.NewAttribute(types.AttributeKeyDstPort, packet.GetDestPort()),
sdk.NewAttribute(types.AttributeKeyDstChannel, packet.GetDestChannel()),
sdk.NewAttribute(types.AttributeKeyChannelOrdering, channel.Ordering.String()),
// we only support 1-hop packets now, and that is the most important hop for a relayer
// (is it going to a chain I am connected to)
sdk.NewAttribute(types.AttributeKeyConnection, channel.ConnectionHops[0]),
),
sdk.NewEvent(
sdk.EventTypeMessage,
sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
),
})
EmitAcknowledgePacketEvent(ctx, packet, channel)

return nil
}
20 changes: 10 additions & 10 deletions modules/core/04-channel/keeper/packet_test.go
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
48 changes: 23 additions & 25 deletions modules/core/04-channel/keeper/timeout.go
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,19 @@ 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())
EmitTimeoutPacketEvent(ctx, packet, channel)
// 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 @@ -155,23 +160,7 @@ func (k Keeper) TimeoutExecuted(
k.Logger(ctx).Info("packet timed-out", "packet", fmt.Sprintf("%v", packet))

// emit an event marking that we have processed the timeout
ctx.EventManager().EmitEvents(sdk.Events{
sdk.NewEvent(
types.EventTypeTimeoutPacket,
sdk.NewAttribute(types.AttributeKeyTimeoutHeight, packet.GetTimeoutHeight().String()),
sdk.NewAttribute(types.AttributeKeyTimeoutTimestamp, fmt.Sprintf("%d", packet.GetTimeoutTimestamp())),
sdk.NewAttribute(types.AttributeKeySequence, fmt.Sprintf("%d", packet.GetSequence())),
sdk.NewAttribute(types.AttributeKeySrcPort, packet.GetSourcePort()),
sdk.NewAttribute(types.AttributeKeySrcChannel, packet.GetSourceChannel()),
sdk.NewAttribute(types.AttributeKeyDstPort, packet.GetDestPort()),
sdk.NewAttribute(types.AttributeKeyDstChannel, packet.GetDestChannel()),
sdk.NewAttribute(types.AttributeKeyChannelOrdering, channel.Ordering.String()),
),
sdk.NewEvent(
sdk.EventTypeMessage,
sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
),
})
EmitTimeoutPacketEvent(ctx, packet, channel)

return nil
}
Expand Down Expand Up @@ -222,6 +211,15 @@ func (k Keeper) TimeoutOnClose(

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

if len(commitment) == 0 {
EmitTimeoutPacketEvent(ctx, packet, channel)
// 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
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
Loading