perform a no-op on redundant relay messages (#268)

* create initial changes for delivertx handling * handle closed channel no-ops, fix tests * self review nits * add changelog * add events for no op messages * add back comment
cosmos · Jul 20, 2021 · 4a83b05 · 4a83b05
1 parent 76ad03e
commit 4a83b05
Show file tree

Hide file tree

Showing 11 changed files with 307 additions and 143 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -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

diff --git a/modules/core/04-channel/keeper/events.go b/modules/core/04-channel/keeper/events.go
@@ -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),
+		),
+	})
+}
diff --git a/modules/core/04-channel/keeper/packet.go b/modules/core/04-channel/keeper/packet.go
@@ -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
@@ -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 {
@@ -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
 }
@@ -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)
@@ -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
 }
diff --git a/modules/core/04-channel/keeper/packet_test.go b/modules/core/04-channel/keeper/packet_test.go
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)
@@ -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)

diff --git a/modules/core/04-channel/keeper/timeout.go b/modules/core/04-channel/keeper/timeout.go
@@ -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(),
-		)
-	}
-
 	// NOTE: TimeoutPacket is called by the AnteHandler which acts upon the packet.Route(),
 	// so the capability authentication can be omitted here
 
@@ -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)
@@ -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
 }
@@ -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

diff --git a/modules/core/04-channel/keeper/timeout_test.go b/modules/core/04-channel/keeper/timeout_test.go
@@ -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()
 
@@ -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)
@@ -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() {
@@ -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()
 
@@ -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)