Reject Redundant Tx Antedecorator (#235)

* writeup simple antedecorator

* only do antehandler on checkTx

* Update modules/core/04-channel/ante.go

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

* enable 2 antehandler strategies, and write tests

* remove strict decorator and pass on non-packet/update type

* move ante logic into its own package

* changelog

Co-authored-by: colin axnér <25233464+colin-axner@users.noreply.github.com>

CHANGELOG.md

@@ -84,6 +84,7 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (core/04-channel) [\#197](https://github.com/cosmos/ibc-go/pull/197) Introduced a `packet_ack_hex` attribute to emit the hex-encoded acknowledgement in events. This allows for raw binary (proto-encoded message) to be sent over events and decoded correctly on relayer. Original `packet_ack` is DEPRECATED. All relayers and IBC event consumers are encouraged to switch to `packet_ack_hex` as soon as possible.
 * (modules/light-clients/07-tendermint) [\#125](https://github.com/cosmos/ibc-go/pull/125) Implement efficient iteration of consensus states and pruning of earliest expired consensus state on UpdateClient.
 * (modules/light-clients/07-tendermint) [\#141](https://github.com/cosmos/ibc-go/pull/141) Return early in case there's a duplicate update call to save Gas.
+* (modules/core/ante) [\#235](https://github.com/cosmos/ibc-go/pull/235) Introduces a new IBC Antedecorator that will reject transactions that only contain redundant packet messages (and accompany UpdateClient msgs). This will prevent relayers from wasting fees by submitting messages for packets that have already been processed by previous relayer(s). The Antedecorator is only applied on CheckTx and RecheckTx and is therefore optional for each node.
 
 ### Features
 

modules/core/04-channel/types/errors.go

@@ -30,4 +30,7 @@ var (
 
 	// ORDERED channel error
 	ErrPacketSequenceOutOfOrder = sdkerrors.Register(SubModuleName, 21, "packet sequence is out of order")
+
+	// Antehandler error
+	ErrRedundantTx = sdkerrors.Register(SubModuleName, 22, "packet messages are redundant")
 )

modules/core/ante/ante.go

@@ -0,0 +1,72 @@
+package ante
+
+import (
+	sdk "github.com/cosmos/cosmos-sdk/types"
+	clienttypes "github.com/cosmos/ibc-go/modules/core/02-client/types"
+	channelkeeper "github.com/cosmos/ibc-go/modules/core/04-channel/keeper"
+	channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types"
+)
+
+type AnteDecorator struct {
+	k channelkeeper.Keeper
+}
+
+func NewAnteDecorator(k channelkeeper.Keeper) AnteDecorator {
+	return AnteDecorator{k: k}
+}
+
+// AnteDecorator returns an error if a multiMsg tx only contains packet messages (Recv, Ack, Timeout) and additional update messages and all packet messages
+// are redundant. If the transaction is just a single UpdateClient message, or the multimsg transaction contains some other message type, then the antedecorator returns no error
+// and continues processing to ensure these transactions are included.
+// This will ensure that relayers do not waste fees on multiMsg transactions when another relayer has already submitted all packets, by rejecting the tx at the mempool layer.
+func (ad AnteDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, simulate bool, next sdk.AnteHandler) (sdk.Context, error) {
+	// do not run redundancy check on DeliverTx or simulate
+	if (ctx.IsCheckTx() || ctx.IsReCheckTx()) && !simulate {
+		// keep track of total packet messages and number of redundancies across `RecvPacket`, `AcknowledgePacket`, and `TimeoutPacket/OnClose`
+		redundancies := 0
+		packetMsgs := 0
+		for _, m := range tx.GetMsgs() {
+			switch msg := m.(type) {
+			case *channeltypes.MsgRecvPacket:
+				if _, found := ad.k.GetPacketReceipt(ctx, msg.Packet.GetDestPort(), msg.Packet.GetDestChannel(), msg.Packet.GetSequence()); found {
+					redundancies += 1
+				}
+				packetMsgs += 1
+
+			case *channeltypes.MsgAcknowledgement:
+				if commitment := ad.k.GetPacketCommitment(ctx, msg.Packet.GetSourcePort(), msg.Packet.GetSourceChannel(), msg.Packet.GetSequence()); len(commitment) == 0 {
+					redundancies += 1
+				}
+				packetMsgs += 1
+
+			case *channeltypes.MsgTimeout:
+				if commitment := ad.k.GetPacketCommitment(ctx, msg.Packet.GetSourcePort(), msg.Packet.GetSourceChannel(), msg.Packet.GetSequence()); len(commitment) == 0 {
+					redundancies += 1
+				}
+				packetMsgs += 1
+
+			case *channeltypes.MsgTimeoutOnClose:
+				if commitment := ad.k.GetPacketCommitment(ctx, msg.Packet.GetSourcePort(), msg.Packet.GetSourceChannel(), msg.Packet.GetSequence()); len(commitment) == 0 {
+					redundancies += 1
+				}
+				packetMsgs += 1
+
+			case *clienttypes.MsgUpdateClient:
+				// do nothing here, as we want to avoid updating clients if it is batched with only redundant messages
+
+			default:
+				// if the multiMsg tx has a msg that is not a packet msg or update msg, then we will not return error
+				// regardless of if all packet messages are redundant. This ensures that non-packet messages get processed
+				// even if they get batched with redundant packet messages.
+				return next(ctx, tx, simulate)
+			}
+
+		}
+
+		// only return error if all packet messages are redundant
+		if redundancies == packetMsgs && packetMsgs > 0 {
+			return ctx, channeltypes.ErrRedundantTx
+		}
+	}
+	return next(ctx, tx, simulate)
+}