diff --git a/CHANGELOG.md b/CHANGELOG.md index 2daf3e4c38f..471703349bd 100644 --- 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 new file mode 100644 index 00000000000..13c92246f02 --- /dev/null +++ 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 index 571b7ae3b53..5cfc7e3c14f 100644 --- 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 index d1cb11370c3..9ef7798140d 100644 --- 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 index caf4e037cf8..93fa9eca4fe 100644 --- 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 index 460e6097632..7cdb88cdc05 100644 --- 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) diff --git a/modules/core/04-channel/types/errors.go b/modules/core/04-channel/types/errors.go index 74043730bf0..8149136ed56 100644 --- a/modules/core/04-channel/types/errors.go +++ b/modules/core/04-channel/types/errors.go @@ -33,4 +33,7 @@ var ( // Antehandler error ErrRedundantTx = sdkerrors.Register(SubModuleName, 22, "packet messages are redundant") + + // Perform a no-op on the current Msg + ErrNoOpMsg = sdkerrors.Register(SubModuleName, 23, "message is redundant, no-op will be performed") ) diff --git a/modules/core/keeper/msg_server.go b/modules/core/keeper/msg_server.go index ab9e583385c..69bde98b687 100644 --- a/modules/core/keeper/msg_server.go +++ b/modules/core/keeper/msg_server.go @@ -492,15 +492,29 @@ 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, perform a no-op + // Use a cached context to prevent accidental state changes + cacheCtx, writeFn := ctx.CacheContext() + err = k.ChannelKeeper.RecvPacket(cacheCtx, cap, msg.Packet, msg.ProofCommitment, msg.ProofHeight) + + // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. + ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) + + switch err { + case nil: + writeFn() + case channeltypes.ErrNoOpMsg: + return &channeltypes.MsgRecvPacketResponse{}, nil // no-op + default: return nil, sdkerrors.Wrap(err, "receive packet verification failed") } // 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. // Events from callback are emitted regardless of acknowledgement success ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) @@ -556,7 +570,21 @@ 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() + err = k.ChannelKeeper.TimeoutPacket(cacheCtx, msg.Packet, msg.ProofUnreceived, msg.ProofHeight, msg.NextSequenceRecv) + + // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. + ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) + + switch err { + case nil: + writeFn() + case channeltypes.ErrNoOpMsg: + return &channeltypes.MsgTimeoutResponse{}, nil // no-op + default: return nil, sdkerrors.Wrap(err, "timeout packet verification failed") } @@ -610,11 +638,26 @@ 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() + err = k.ChannelKeeper.TimeoutOnClose(cacheCtx, cap, msg.Packet, msg.ProofUnreceived, msg.ProofClose, msg.ProofHeight, msg.NextSequenceRecv) + + // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. + ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) + + switch err { + case nil: + writeFn() + case channeltypes.ErrNoOpMsg: + return &channeltypes.MsgTimeoutOnCloseResponse{}, nil // no-op + default: return nil, sdkerrors.Wrap(err, "timeout on close packet verification failed") } // Perform application logic callback + // // NOTE: MsgTimeout and MsgTimeoutOnClose use the same "OnTimeoutPacket" // application logic callback. err = cbs.OnTimeoutPacket(ctx, msg.Packet, relayer) @@ -666,7 +709,21 @@ 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, perform a no-op + // Use a cached context to prevent accidental state changes + cacheCtx, writeFn := ctx.CacheContext() + err = k.ChannelKeeper.AcknowledgePacket(cacheCtx, cap, msg.Packet, msg.Acknowledgement, msg.ProofAcked, msg.ProofHeight) + + // NOTE: The context returned by CacheContext() refers to a new EventManager, so it needs to explicitly set events to the original context. + ctx.EventManager().EmitEvents(cacheCtx.EventManager().Events()) + + switch err { + case nil: + writeFn() + case channeltypes.ErrNoOpMsg: + return &channeltypes.MsgAcknowledgementResponse{}, nil // no-op + default: return nil, sdkerrors.Wrap(err, "acknowledge packet verification failed") } diff --git a/modules/core/keeper/msg_server_test.go b/modules/core/keeper/msg_server_test.go index a337c1fb323..b28dc263689 100644 --- a/modules/core/keeper/msg_server_test.go +++ b/modules/core/keeper/msg_server_test.go @@ -141,7 +141,8 @@ 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() { + // mock will panic if application callback is called twice on the same packet 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) @@ -151,8 +152,9 @@ func (suite *KeeperTestSuite) TestHandleRecvPacket() { err = path.EndpointB.RecvPacket(packet) suite.Require().NoError(err) - }, false, false}, - {"UNORDERED: packet already received (replay)", func() { + }, true, false}, + {"successful no-op: UNORDERED - packet already received (replay)", func() { + // mock will panic if application callback is called twice on the same packet 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) @@ -161,7 +163,7 @@ func (suite *KeeperTestSuite) TestHandleRecvPacket() { err = path.EndpointB.RecvPacket(packet) suite.Require().NoError(err) - }, false, false}, + }, true, false}, } for _, tc := range testCases { @@ -174,9 +176,16 @@ func (suite *KeeperTestSuite) TestHandleRecvPacket() { tc.malleate() + var ( + proof []byte + proofHeight clienttypes.Height + ) + // get proof of packet commitment from chainA packetKey := host.PacketCommitmentKey(packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) - proof, proofHeight := suite.chainA.QueryProof(packetKey) + if path.EndpointA.ChannelID != "" { + proof, proofHeight = path.EndpointA.QueryProof(packetKey) + } msg := channeltypes.NewMsgRecvPacket(packet, proof, proofHeight, suite.chainB.SenderAccount.GetAddress().String()) @@ -185,12 +194,12 @@ 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) + _, exists := suite.chainB.GetSimApp().ScopedIBCMockKeeper.GetCapability(suite.chainB.GetContext(), ibctesting.GetMockRecvCanaryCapabilityName(packet)) if tc.expRevert { suite.Require().False(exists, "capability exists in store even after callback reverted") } else { @@ -293,7 +302,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) @@ -305,8 +314,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) @@ -319,7 +328,7 @@ func (suite *KeeperTestSuite) TestHandleAcknowledgePacket() { err = path.EndpointA.AcknowledgePacket(packet, ibctesting.MockAcknowledgement) suite.Require().NoError(err) - }, false}, + }, true}, } for _, tc := range testCases { @@ -331,8 +340,14 @@ func (suite *KeeperTestSuite) TestHandleAcknowledgePacket() { tc.malleate() + var ( + proof []byte + proofHeight clienttypes.Height + ) packetKey := host.PacketAcknowledgementKey(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence()) - proof, proofHeight := suite.chainB.QueryProof(packetKey) + if path.EndpointB.ChannelID != "" { + proof, proofHeight = path.EndpointB.QueryProof(packetKey) + } msg := channeltypes.NewMsgAcknowledgement(packet, ibcmock.MockAcknowledgement.Acknowledgement(), proof, proofHeight, suite.chainA.SenderAccount.GetAddress().String()) @@ -341,14 +356,13 @@ func (suite *KeeperTestSuite) TestHandleAcknowledgePacket() { if tc.expPass { suite.Require().NoError(err) - // replay should an error - _, err := keeper.Keeper.Acknowledgement(*suite.chainA.App.GetIBCKeeper(), sdk.WrapSDKContext(suite.chainA.GetContext()), msg) - suite.Require().Error(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()) suite.Require().False(has) + // 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().NoError(err) } else { suite.Require().Error(err) } @@ -441,11 +455,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) + packet = channeltypes.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, clienttypes.NewHeight(0, 1), 0) packetKey = host.PacketReceiptKey(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence()) - }, false}, + }, true}, } for _, tc := range testCases { @@ -457,7 +471,13 @@ func (suite *KeeperTestSuite) TestHandleTimeoutPacket() { tc.malleate() - proof, proofHeight := suite.chainB.QueryProof(packetKey) + var ( + proof []byte + proofHeight clienttypes.Height + ) + if path.EndpointB.ChannelID != "" { + proof, proofHeight = path.EndpointB.QueryProof(packetKey) + } msg := channeltypes.NewMsgTimeout(packet, 1, proof, proofHeight, suite.chainA.SenderAccount.GetAddress().String()) @@ -466,9 +486,9 @@ func (suite *KeeperTestSuite) TestHandleTimeoutPacket() { if tc.expPass { suite.Require().NoError(err) - // replay should return an error + // 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()) @@ -577,14 +597,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) + packet = channeltypes.NewPacket(ibctesting.MockPacketData, 1, path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, path.EndpointB.ChannelConfig.PortID, path.EndpointB.ChannelID, clienttypes.NewHeight(0, 1), 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) @@ -622,9 +642,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()) diff --git a/testing/mock/mock.go b/testing/mock/mock.go index 9edf5d3536a..fe527e1c22d 100644 --- a/testing/mock/mock.go +++ b/testing/mock/mock.go @@ -3,6 +3,7 @@ package mock import ( "bytes" "encoding/json" + "strconv" "github.com/cosmos/cosmos-sdk/client" "github.com/cosmos/cosmos-sdk/codec" @@ -27,12 +28,14 @@ const ( ) var ( - MockAcknowledgement = channeltypes.NewResultAcknowledgement([]byte("mock acknowledgement")) - MockFailAcknowledgement = channeltypes.NewErrorAcknowledgement("mock failed acknowledgement") - MockPacketData = []byte("mock packet data") - MockFailPacketData = []byte("mock failed packet data") - MockAsyncPacketData = []byte("mock async packet data") - MockCanaryCapabilityName = "mock canary capability name" + MockAcknowledgement = channeltypes.NewResultAcknowledgement([]byte("mock acknowledgement")) + MockFailAcknowledgement = channeltypes.NewErrorAcknowledgement("mock failed acknowledgement") + MockPacketData = []byte("mock packet data") + MockFailPacketData = []byte("mock failed packet data") + MockAsyncPacketData = []byte("mock async packet data") + MockRecvCanaryCapabilityName = "mock receive canary capability name" + MockAckCanaryCapabilityName = "mock acknowledgement canary capability name" + MockTimeoutCanaryCapabilityName = "mock timeout canary capability name" ) var _ porttypes.IBCModule = AppModule{} @@ -194,7 +197,12 @@ func (am AppModule) OnChanCloseConfirm(sdk.Context, string, string) error { // OnRecvPacket implements the IBCModule interface. func (am AppModule) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, relayer sdk.AccAddress) exported.Acknowledgement { // set state by claiming capability to check if revert happens return - am.scopedKeeper.NewCapability(ctx, MockCanaryCapabilityName) + _, err := am.scopedKeeper.NewCapability(ctx, MockRecvCanaryCapabilityName+strconv.Itoa(int(packet.GetSequence()))) + if err != nil { + // application callback called twice on same packet sequence + // must never occur + panic(err) + } if bytes.Equal(MockPacketData, packet.GetData()) { return MockAcknowledgement } else if bytes.Equal(MockAsyncPacketData, packet.GetData()) { @@ -205,11 +213,25 @@ func (am AppModule) OnRecvPacket(ctx sdk.Context, packet channeltypes.Packet, re } // OnAcknowledgementPacket implements the IBCModule interface. -func (am AppModule) OnAcknowledgementPacket(sdk.Context, channeltypes.Packet, []byte, sdk.AccAddress) error { +func (am AppModule) OnAcknowledgementPacket(ctx sdk.Context, packet channeltypes.Packet, _ []byte, _ sdk.AccAddress) error { + _, err := am.scopedKeeper.NewCapability(ctx, MockAckCanaryCapabilityName+strconv.Itoa(int(packet.GetSequence()))) + if err != nil { + // application callback called twice on same packet sequence + // must never occur + panic(err) + } + return nil } // OnTimeoutPacket implements the IBCModule interface. -func (am AppModule) OnTimeoutPacket(sdk.Context, channeltypes.Packet, sdk.AccAddress) error { +func (am AppModule) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet, _ sdk.AccAddress) error { + _, err := am.scopedKeeper.NewCapability(ctx, MockTimeoutCanaryCapabilityName+strconv.Itoa(int(packet.GetSequence()))) + if err != nil { + // application callback called twice on same packet sequence + // must never occur + panic(err) + } + return nil } diff --git a/testing/values.go b/testing/values.go index 5ba3ab1dd29..7a66b683826 100644 --- a/testing/values.go +++ b/testing/values.go @@ -5,12 +5,14 @@ package ibctesting import ( + "strconv" "time" sdk "github.com/cosmos/cosmos-sdk/types" ibctransfertypes "github.com/cosmos/ibc-go/modules/apps/transfer/types" connectiontypes "github.com/cosmos/ibc-go/modules/core/03-connection/types" + channeltypes "github.com/cosmos/ibc-go/modules/core/04-channel/types" commitmenttypes "github.com/cosmos/ibc-go/modules/core/23-commitment/types" ibctmtypes "github.com/cosmos/ibc-go/modules/light-clients/07-tendermint/types" "github.com/cosmos/ibc-go/testing/mock" @@ -50,10 +52,14 @@ var ( ConnectionVersion = connectiontypes.ExportedVersionsToProto(connectiontypes.GetCompatibleVersions())[0] - MockAcknowledgement = mock.MockAcknowledgement.Acknowledgement() - MockPacketData = mock.MockPacketData - MockFailPacketData = mock.MockFailPacketData - MockCanaryCapabilityName = mock.MockCanaryCapabilityName + MockAcknowledgement = mock.MockAcknowledgement.Acknowledgement() + MockPacketData = mock.MockPacketData + MockFailPacketData = mock.MockFailPacketData + MockRecvCanaryCapabilityName = mock.MockRecvCanaryCapabilityName prefix = commitmenttypes.NewMerklePrefix([]byte("ibc")) ) + +func GetMockRecvCanaryCapabilityName(packet channeltypes.Packet) string { + return MockRecvCanaryCapabilityName + strconv.Itoa(int(packet.GetSequence())) +}