From 654b2fdd1019f490b003df6f21de137fcce76480 Mon Sep 17 00:00:00 2001 From: colin axner <25233464+colin-axner@users.noreply.github.com> Date: Sat, 30 May 2020 18:18:43 -0700 Subject: [PATCH] x/ibc: packet commitment deletion after ack verification (#6292) * add deletion of packet commitments to AcknowledgePacket and remove CleanupPacket * update tests * add replay test for unordered channels * add cleanup back, update tests * remove nolint * add timeoutonclose handling and fix * add check for deleted packets * Update x/ibc/04-channel/keeper/keeper.go Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> * add ackexecuted and update tests * add timeoutexecuted back and update test * move events to executed funcs * update handler * update godoc Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com> --- x/ibc/02-client/alias.go | 78 +++++---- x/ibc/02-client/handler.go | 2 +- x/ibc/04-channel/keeper/handshake_test.go | 204 +++++++++++----------- x/ibc/04-channel/keeper/keeper.go | 18 ++ x/ibc/04-channel/keeper/keeper_test.go | 4 +- x/ibc/04-channel/keeper/packet.go | 114 ++++++++---- x/ibc/04-channel/keeper/packet_test.go | 121 ++++++++++++- x/ibc/04-channel/keeper/timeout.go | 86 +++++---- x/ibc/04-channel/keeper/timeout_test.go | 59 +++++-- x/ibc/handler.go | 29 ++- 10 files changed, 488 insertions(+), 227 deletions(-) diff --git a/x/ibc/02-client/alias.go b/x/ibc/02-client/alias.go index bf959628e..2536d6601 100644 --- a/x/ibc/02-client/alias.go +++ b/x/ibc/02-client/alias.go @@ -2,8 +2,8 @@ package client // autogenerated code using github.com/rigelrozanski/multitool // aliases generated for the following subdirectories: -// ALIASGEN: github.com/cosmos/cosmos-sdk/x/ibc/02-client/keeper // ALIASGEN: github.com/cosmos/cosmos-sdk/x/ibc/02-client/types +// ALIASGEN: github.com/cosmos/cosmos-sdk/x/ibc/02-client/keeper import ( "github.com/cosmos/cosmos-sdk/x/ibc/02-client/keeper" @@ -11,45 +11,61 @@ import ( ) const ( - AttributeKeyClientID = types.AttributeKeyClientID - AttrbuteKeyClientType = types.AttributeKeyClientType - SubModuleName = types.SubModuleName - RouterKey = types.RouterKey - QuerierRoute = types.QuerierRoute - QueryAllClients = types.QueryAllClients - QueryClientState = types.QueryClientState - QueryConsensusState = types.QueryConsensusState + AttributeKeyClientID = types.AttributeKeyClientID + AttributeKeyClientType = types.AttributeKeyClientType + SubModuleName = types.SubModuleName + RouterKey = types.RouterKey + QuerierRoute = types.QuerierRoute + QueryAllClients = types.QueryAllClients + QueryClientState = types.QueryClientState + QueryConsensusState = types.QueryConsensusState ) var ( // functions aliases + RegisterCodec = types.RegisterCodec + SetSubModuleCodec = types.SetSubModuleCodec + NewClientConsensusStates = types.NewClientConsensusStates + NewGenesisState = types.NewGenesisState + DefaultGenesisState = types.DefaultGenesisState + NewQueryAllClientsParams = types.NewQueryAllClientsParams + NewClientStateResponse = types.NewClientStateResponse + NewConsensusStateResponse = types.NewConsensusStateResponse NewKeeper = keeper.NewKeeper QuerierClients = keeper.QuerierClients - RegisterCodec = types.RegisterCodec - ErrClientExists = types.ErrClientExists - ErrClientNotFound = types.ErrClientNotFound - ErrClientFrozen = types.ErrClientFrozen - ErrConsensusStateNotFound = types.ErrConsensusStateNotFound - ErrInvalidConsensus = types.ErrInvalidConsensus - ErrClientTypeNotFound = types.ErrClientTypeNotFound - ErrInvalidClientType = types.ErrInvalidClientType - ErrRootNotFound = types.ErrRootNotFound - ErrInvalidHeader = types.ErrInvalidHeader - ErrInvalidEvidence = types.ErrInvalidEvidence - DefaultGenesisState = types.DefaultGenesisState - NewGenesisState = types.NewGenesisState - NewClientConsensusStates = types.NewClientConsensusStates // variable aliases - SubModuleCdc = types.SubModuleCdc - EventTypeCreateClient = types.EventTypeCreateClient - EventTypeUpdateClient = types.EventTypeUpdateClient - AttributeValueCategory = types.AttributeValueCategory + SubModuleCdc = types.SubModuleCdc + ErrClientExists = types.ErrClientExists + ErrClientNotFound = types.ErrClientNotFound + ErrClientFrozen = types.ErrClientFrozen + ErrConsensusStateNotFound = types.ErrConsensusStateNotFound + ErrInvalidConsensus = types.ErrInvalidConsensus + ErrClientTypeNotFound = types.ErrClientTypeNotFound + ErrInvalidClientType = types.ErrInvalidClientType + ErrRootNotFound = types.ErrRootNotFound + ErrInvalidHeader = types.ErrInvalidHeader + ErrInvalidEvidence = types.ErrInvalidEvidence + ErrFailedClientConsensusStateVerification = types.ErrFailedClientConsensusStateVerification + ErrFailedConnectionStateVerification = types.ErrFailedConnectionStateVerification + ErrFailedChannelStateVerification = types.ErrFailedChannelStateVerification + ErrFailedPacketCommitmentVerification = types.ErrFailedPacketCommitmentVerification + ErrFailedPacketAckVerification = types.ErrFailedPacketAckVerification + ErrFailedPacketAckAbsenceVerification = types.ErrFailedPacketAckAbsenceVerification + ErrFailedNextSeqRecvVerification = types.ErrFailedNextSeqRecvVerification + ErrSelfConsensusStateNotFound = types.ErrSelfConsensusStateNotFound + EventTypeCreateClient = types.EventTypeCreateClient + EventTypeUpdateClient = types.EventTypeUpdateClient + EventTypeSubmitMisbehaviour = types.EventTypeSubmitMisbehaviour + AttributeValueCategory = types.AttributeValueCategory ) type ( - Keeper = keeper.Keeper - StakingKeeper = types.StakingKeeper - GenesisState = types.GenesisState - ConsensusStates = types.ClientConsensusStates + StakingKeeper = types.StakingKeeper + ConsensusStates = types.ClientConsensusStates + GenesisState = types.GenesisState + QueryAllClientsParams = types.QueryAllClientsParams + StateResponse = types.StateResponse + ConsensusStateResponse = types.ConsensusStateResponse + Keeper = keeper.Keeper ) diff --git a/x/ibc/02-client/handler.go b/x/ibc/02-client/handler.go index fe23c60ea..a15c8efa7 100644 --- a/x/ibc/02-client/handler.go +++ b/x/ibc/02-client/handler.go @@ -47,7 +47,7 @@ func HandleMsgCreateClient(ctx sdk.Context, k Keeper, msg exported.MsgCreateClie sdk.NewEvent( EventTypeCreateClient, sdk.NewAttribute(AttributeKeyClientID, msg.GetClientID()), - sdk.NewAttribute(AttrbuteKeyClientType, msg.GetClientType()), + sdk.NewAttribute(AttributeKeyClientType, msg.GetClientType()), ), sdk.NewEvent( sdk.EventTypeMessage, diff --git a/x/ibc/04-channel/keeper/handshake_test.go b/x/ibc/04-channel/keeper/handshake_test.go index d2c5ac6f0..4760ed0ef 100644 --- a/x/ibc/04-channel/keeper/handshake_test.go +++ b/x/ibc/04-channel/keeper/handshake_test.go @@ -85,7 +85,7 @@ func (suite *KeeperTestSuite) TestChanOpenTry() { {"success", func() { suite.chainA.CreateClient(suite.chainB) suite.chainB.CreateClient(suite.chainA) - _ = suite.chainA.createConnection( + suite.chainA.createConnection( testConnectionIDB, testConnectionIDA, testClientIDB, testClientIDA, connection.OPEN, ) @@ -94,27 +94,27 @@ func (suite *KeeperTestSuite) TestChanOpenTry() { suite.chainB.createChannel(testPort1, testChannel1, testPort2, testChannel2, types.INIT, types.ORDERED, testConnectionIDA) }, true}, {"previous channel with invalid state", func() { - _ = suite.chainA.createChannel( + suite.chainA.createChannel( testPort2, testChannel2, testPort1, testChannel1, types.UNINITIALIZED, types.ORDERED, testConnectionIDB, ) }, false}, {"connection doesn't exist", func() {}, false}, {"connection is not OPEN", func() { - _ = suite.chainA.createConnection( + suite.chainA.createConnection( testConnectionIDB, testConnectionIDA, testClientIDB, testClientIDA, connection.INIT, ) }, false}, {"consensus state not found", func() { - _ = suite.chainA.createConnection( + suite.chainA.createConnection( testConnectionIDB, testConnectionIDA, testClientIDB, testClientIDA, connection.OPEN, ) }, false}, {"channel verification failed", func() { suite.chainA.CreateClient(suite.chainB) - _ = suite.chainA.createConnection( + suite.chainA.createConnection( testConnectionIDB, testConnectionIDA, testClientIDB, testClientIDA, connection.OPEN, ) @@ -122,7 +122,7 @@ func (suite *KeeperTestSuite) TestChanOpenTry() { {"port capability not found", func() { suite.chainA.CreateClient(suite.chainB) suite.chainB.CreateClient(suite.chainA) - _ = suite.chainA.createConnection( + suite.chainA.createConnection( testConnectionIDB, testConnectionIDA, testClientIDB, testClientIDA, connection.OPEN, ) @@ -187,11 +187,11 @@ func (suite *KeeperTestSuite) TestChanOpenAck() { testConnectionIDB, testConnectionIDA, testClientIDB, testClientIDA, connection.OPEN, ) - _ = suite.chainB.createConnection( + suite.chainB.createConnection( testConnectionIDA, testConnectionIDB, testClientIDA, testClientIDB, connection.OPEN, ) - _ = suite.chainA.createChannel( + suite.chainA.createChannel( testPort1, testChannel1, testPort2, testChannel2, types.INIT, types.ORDERED, testConnectionIDB, ) @@ -202,44 +202,44 @@ func (suite *KeeperTestSuite) TestChanOpenAck() { }, true}, {"channel doesn't exist", func() {}, false}, {"channel state is not INIT or TRYOPEN", func() { - _ = suite.chainB.createChannel( + suite.chainB.createChannel( testPort1, testChannel1, testPort2, testChannel2, types.UNINITIALIZED, types.ORDERED, testConnectionIDA, ) }, false}, {"connection not found", func() { - _ = suite.chainB.createChannel( + suite.chainB.createChannel( testPort1, testChannel1, testPort2, testChannel2, types.TRYOPEN, types.ORDERED, testConnectionIDA, ) }, false}, {"connection is not OPEN", func() { - _ = suite.chainB.createConnection( + suite.chainB.createConnection( testConnectionIDA, testConnectionIDB, testClientIDA, testClientIDB, connection.TRYOPEN, ) - _ = suite.chainB.createChannel( + suite.chainB.createChannel( testPort1, testChannel1, testPort2, testChannel2, types.TRYOPEN, types.ORDERED, testConnectionIDA, ) }, false}, {"consensus state not found", func() { - _ = suite.chainB.createConnection( + suite.chainB.createConnection( testConnectionIDA, testConnectionIDB, testClientIDA, testClientIDB, connection.OPEN, ) - _ = suite.chainB.createChannel( + suite.chainB.createChannel( testPort1, testChannel1, testPort2, testChannel2, types.TRYOPEN, types.ORDERED, testConnectionIDA, ) }, false}, {"channel verification failed", func() { suite.chainB.CreateClient(suite.chainA) - _ = suite.chainB.createConnection( + suite.chainB.createConnection( testConnectionIDA, testConnectionIDB, testClientIDA, testClientIDB, connection.OPEN, ) - _ = suite.chainB.createChannel( + suite.chainB.createChannel( testPort1, testChannel1, testPort2, testChannel2, types.TRYOPEN, types.ORDERED, testConnectionIDA, ) @@ -251,11 +251,11 @@ func (suite *KeeperTestSuite) TestChanOpenAck() { testConnectionIDB, testConnectionIDA, testClientIDB, testClientIDA, connection.OPEN, ) - _ = suite.chainB.createConnection( + suite.chainB.createConnection( testConnectionIDA, testConnectionIDB, testClientIDA, testClientIDB, connection.OPEN, ) - _ = suite.chainA.createChannel( + suite.chainA.createChannel( testPort1, testChannel1, testPort2, testChannel2, types.INIT, types.ORDERED, testConnectionIDB, ) @@ -308,69 +308,7 @@ func (suite *KeeperTestSuite) TestChanOpenConfirm() { {"success", func() { suite.chainA.CreateClient(suite.chainB) suite.chainB.CreateClient(suite.chainA) - _ = suite.chainA.createConnection( - testConnectionIDB, testConnectionIDA, testClientIDB, testClientIDA, - connection.TRYOPEN, - ) - _ = suite.chainB.createConnection( - testConnectionIDA, testConnectionIDB, testClientIDA, testClientIDB, - connection.OPEN, - ) - _ = suite.chainA.createChannel( - testPort2, testChannel2, testPort1, testChannel1, types.OPEN, - types.ORDERED, testConnectionIDB, - ) - _ = suite.chainB.createChannel(testPort1, testChannel1, testPort2, testChannel2, - types.TRYOPEN, types.ORDERED, testConnectionIDA) - }, true}, - {"channel doesn't exist", func() {}, false}, - {"channel state is not TRYOPEN", func() { - _ = suite.chainA.createChannel( - testPort1, testChannel1, testPort2, testChannel2, types.UNINITIALIZED, - types.ORDERED, testConnectionIDB, - ) - }, false}, - {"connection not found", func() { - _ = suite.chainA.createChannel( - testPort2, testChannel2, testPort1, testChannel1, types.TRYOPEN, - types.ORDERED, testConnectionIDB, - ) - }, false}, - {"connection is not OPEN", func() { - _ = suite.chainA.createConnection( - testConnectionIDB, testConnectionIDA, testClientIDB, testClientIDA, - connection.TRYOPEN, - ) - _ = suite.chainA.createChannel( - testPort2, testChannel2, testPort1, testChannel1, types.TRYOPEN, - types.ORDERED, testConnectionIDB, - ) - }, false}, - {"consensus state not found", func() { - _ = suite.chainA.createConnection( - testConnectionIDB, testConnectionIDA, testClientIDB, testClientIDA, - connection.OPEN, - ) - _ = suite.chainA.createChannel( - testPort2, testChannel2, testPort1, testChannel1, types.TRYOPEN, - types.ORDERED, testConnectionIDB, - ) - }, false}, - {"channel verification failed", func() { - suite.chainA.CreateClient(suite.chainB) - _ = suite.chainA.createConnection( - testConnectionIDB, testConnectionIDA, testClientIDB, testClientIDA, - connection.OPEN, - ) - _ = suite.chainA.createChannel( - testPort2, testChannel2, testPort1, testChannel1, types.TRYOPEN, - types.ORDERED, testConnectionIDB, - ) - }, false}, - {"channel capability not found", func() { - suite.chainA.CreateClient(suite.chainB) - suite.chainB.CreateClient(suite.chainA) - _ = suite.chainA.createConnection( + suite.chainA.createConnection( testConnectionIDB, testConnectionIDA, testClientIDB, testClientIDA, connection.TRYOPEN, ) @@ -378,7 +316,69 @@ func (suite *KeeperTestSuite) TestChanOpenConfirm() { testConnectionIDA, testConnectionIDB, testClientIDA, testClientIDB, connection.OPEN, ) - _ = suite.chainA.createChannel( + suite.chainA.createChannel( + testPort2, testChannel2, testPort1, testChannel1, types.OPEN, + types.ORDERED, testConnectionIDB, + ) + suite.chainB.createChannel(testPort1, testChannel1, testPort2, testChannel2, + types.TRYOPEN, types.ORDERED, testConnectionIDA) + }, true}, + {"channel doesn't exist", func() {}, false}, + {"channel state is not TRYOPEN", func() { + suite.chainA.createChannel( + testPort1, testChannel1, testPort2, testChannel2, types.UNINITIALIZED, + types.ORDERED, testConnectionIDB, + ) + }, false}, + {"connection not found", func() { + suite.chainA.createChannel( + testPort2, testChannel2, testPort1, testChannel1, types.TRYOPEN, + types.ORDERED, testConnectionIDB, + ) + }, false}, + {"connection is not OPEN", func() { + suite.chainA.createConnection( + testConnectionIDB, testConnectionIDA, testClientIDB, testClientIDA, + connection.TRYOPEN, + ) + suite.chainA.createChannel( + testPort2, testChannel2, testPort1, testChannel1, types.TRYOPEN, + types.ORDERED, testConnectionIDB, + ) + }, false}, + {"consensus state not found", func() { + suite.chainA.createConnection( + testConnectionIDB, testConnectionIDA, testClientIDB, testClientIDA, + connection.OPEN, + ) + suite.chainA.createChannel( + testPort2, testChannel2, testPort1, testChannel1, types.TRYOPEN, + types.ORDERED, testConnectionIDB, + ) + }, false}, + {"channel verification failed", func() { + suite.chainA.CreateClient(suite.chainB) + suite.chainA.createConnection( + testConnectionIDB, testConnectionIDA, testClientIDB, testClientIDA, + connection.OPEN, + ) + suite.chainA.createChannel( + testPort2, testChannel2, testPort1, testChannel1, types.TRYOPEN, + types.ORDERED, testConnectionIDB, + ) + }, false}, + {"channel capability not found", func() { + suite.chainA.CreateClient(suite.chainB) + suite.chainB.CreateClient(suite.chainA) + suite.chainA.createConnection( + testConnectionIDB, testConnectionIDA, testClientIDB, testClientIDA, + connection.TRYOPEN, + ) + suite.chainB.createConnection( + testConnectionIDA, testConnectionIDB, testClientIDA, testClientIDB, + connection.OPEN, + ) + suite.chainA.createChannel( testPort2, testChannel2, testPort1, testChannel1, types.OPEN, types.ORDERED, testConnectionIDB, ) @@ -426,45 +426,45 @@ func (suite *KeeperTestSuite) TestChanCloseInit() { testCases := []testCase{ {"success", func() { suite.chainB.CreateClient(suite.chainA) - _ = suite.chainA.createConnection( + suite.chainA.createConnection( testConnectionIDA, testConnectionIDB, testClientIDA, testClientIDB, connection.OPEN, ) - _ = suite.chainA.createChannel( + suite.chainA.createChannel( testPort1, testChannel1, testPort2, testChannel2, types.OPEN, types.ORDERED, testConnectionIDA, ) }, true}, {"channel doesn't exist", func() {}, false}, {"channel state is CLOSED", func() { - _ = suite.chainA.createChannel( + suite.chainA.createChannel( testPort1, testChannel1, testPort2, testChannel2, types.CLOSED, types.ORDERED, testConnectionIDB, ) }, false}, {"connection not found", func() { - _ = suite.chainA.createChannel( + suite.chainA.createChannel( testPort1, testChannel1, testPort2, testChannel2, types.OPEN, types.ORDERED, testConnectionIDA, ) }, false}, {"connection is not OPEN", func() { - _ = suite.chainA.createConnection( + suite.chainA.createConnection( testConnectionIDA, testConnectionIDB, testClientIDA, testClientIDB, connection.TRYOPEN, ) - _ = suite.chainA.createChannel( + suite.chainA.createChannel( testPort1, testChannel1, testPort2, testChannel2, types.UNINITIALIZED, types.ORDERED, testConnectionIDA, ) }, false}, {"channel capability not found", func() { suite.chainB.CreateClient(suite.chainA) - _ = suite.chainA.createConnection( + suite.chainA.createConnection( testConnectionIDA, testConnectionIDB, testClientIDA, testClientIDB, connection.OPEN, ) - _ = suite.chainA.createChannel( + suite.chainA.createChannel( testPort1, testChannel1, testPort2, testChannel2, types.OPEN, types.ORDERED, testConnectionIDA, ) @@ -504,7 +504,7 @@ func (suite *KeeperTestSuite) TestChanCloseConfirm() { {"success", func() { suite.chainA.CreateClient(suite.chainB) suite.chainB.CreateClient(suite.chainA) - _ = suite.chainB.createConnection( + suite.chainB.createConnection( testConnectionIDB, testConnectionIDA, testClientIDA, testClientIDB, connection.OPEN, ) @@ -512,7 +512,7 @@ func (suite *KeeperTestSuite) TestChanCloseConfirm() { testConnectionIDA, testConnectionIDB, testClientIDB, testClientIDA, connection.OPEN, ) - _ = suite.chainB.createChannel( + suite.chainB.createChannel( testPort2, testChannel2, testPort1, testChannel1, types.OPEN, types.ORDERED, testConnectionIDB, ) @@ -523,44 +523,44 @@ func (suite *KeeperTestSuite) TestChanCloseConfirm() { }, true}, {"channel doesn't exist", func() {}, false}, {"channel state is CLOSED", func() { - _ = suite.chainB.createChannel( + suite.chainB.createChannel( testPort2, testChannel2, testPort1, testChannel1, types.CLOSED, types.ORDERED, testConnectionIDB, ) }, false}, {"connection not found", func() { - _ = suite.chainB.createChannel( + suite.chainB.createChannel( testPort2, testChannel2, testPort1, testChannel1, types.OPEN, types.ORDERED, testConnectionIDA, ) }, false}, {"connection is not OPEN", func() { - _ = suite.chainB.createConnection( + suite.chainB.createConnection( testConnectionIDB, testConnectionIDA, testClientIDA, testClientIDB, connection.TRYOPEN, ) - _ = suite.chainB.createChannel( + suite.chainB.createChannel( testPort2, testChannel2, testPort1, testChannel1, types.OPEN, types.ORDERED, testConnectionIDB, ) }, false}, {"consensus state not found", func() { - _ = suite.chainB.createConnection( + suite.chainB.createConnection( testConnectionIDB, testConnectionIDA, testClientIDA, testClientIDB, connection.OPEN, ) - _ = suite.chainB.createChannel( + suite.chainB.createChannel( testPort2, testChannel2, testPort1, testChannel1, types.OPEN, types.ORDERED, testConnectionIDB, ) }, false}, {"channel verification failed", func() { suite.chainB.CreateClient(suite.chainA) - _ = suite.chainB.createConnection( + suite.chainB.createConnection( testConnectionIDB, testConnectionIDA, testClientIDA, testClientIDB, connection.OPEN, ) - _ = suite.chainB.createChannel( + suite.chainB.createChannel( testPort2, testChannel2, testPort1, testChannel1, types.OPEN, types.ORDERED, testConnectionIDB, ) @@ -568,7 +568,7 @@ func (suite *KeeperTestSuite) TestChanCloseConfirm() { {"channel capability not found", func() { suite.chainA.CreateClient(suite.chainB) suite.chainB.CreateClient(suite.chainA) - _ = suite.chainB.createConnection( + suite.chainB.createConnection( testConnectionIDB, testConnectionIDA, testClientIDA, testClientIDB, connection.OPEN, ) @@ -576,7 +576,7 @@ func (suite *KeeperTestSuite) TestChanCloseConfirm() { testConnectionIDA, testConnectionIDB, testClientIDB, testClientIDA, connection.OPEN, ) - _ = suite.chainB.createChannel( + suite.chainB.createChannel( testPort2, testChannel2, testPort1, testChannel1, types.OPEN, types.ORDERED, testConnectionIDB, ) diff --git a/x/ibc/04-channel/keeper/keeper.go b/x/ibc/04-channel/keeper/keeper.go index 60664a1f3..6cea0edb3 100644 --- a/x/ibc/04-channel/keeper/keeper.go +++ b/x/ibc/04-channel/keeper/keeper.go @@ -128,6 +128,11 @@ func (k Keeper) GetPacketCommitment(ctx sdk.Context, portID, channelID string, s return bz } +// HasPacketCommitment returns true if the packet commitment exists +func (k Keeper) HasPacketCommitment(ctx sdk.Context, portID, channelID string, sequence uint64) bool { + return len(k.GetPacketCommitment(ctx, portID, channelID, sequence)) > 0 +} + // SetPacketCommitment sets the packet commitment hash to the store func (k Keeper) SetPacketCommitment(ctx sdk.Context, portID, channelID string, sequence uint64, commitmentHash []byte) { store := ctx.KVStore(k.storeKey) @@ -139,6 +144,19 @@ func (k Keeper) deletePacketCommitment(ctx sdk.Context, portID, channelID string store.Delete(host.KeyPacketCommitment(portID, channelID, sequence)) } +// deletePacketCommitmentsLTE removes all consecutive packet commitments less than or equal to sequence +// +// CONTRACT: this function can only be used for ORDERED channel batch clearing +func (k Keeper) deletePacketCommitmentsLTE(ctx sdk.Context, portID, channelID string, sequence uint64) { + store := ctx.KVStore(k.storeKey) + for i := sequence; i > 0; i-- { + if !k.HasPacketCommitment(ctx, portID, channelID, i) { + return + } + store.Delete(host.KeyPacketCommitment(portID, channelID, i)) + } +} + // SetPacketAcknowledgement sets the packet ack hash to the store func (k Keeper) SetPacketAcknowledgement(ctx sdk.Context, portID, channelID string, sequence uint64, ackHash []byte) { store := ctx.KVStore(k.storeKey) diff --git a/x/ibc/04-channel/keeper/keeper_test.go b/x/ibc/04-channel/keeper/keeper_test.go index 847131d6b..8f50a6fc4 100644 --- a/x/ibc/04-channel/keeper/keeper_test.go +++ b/x/ibc/04-channel/keeper/keeper_test.go @@ -432,8 +432,10 @@ func (chain *TestChain) createChannel( state types.State, order types.Order, connectionID string, ) types.Channel { counterparty := types.NewCounterparty(counterpartyPortID, counterpartyChannelID) + + // sets channel with given state channel := types.NewChannel(state, order, counterparty, - []string{connectionID}, "1.0", + []string{connectionID}, testChannelVersion, ) ctx := chain.GetContext() chain.App.IBCKeeper.ChannelKeeper.SetChannel(ctx, portID, channelID, channel) diff --git a/x/ibc/04-channel/keeper/packet.go b/x/ibc/04-channel/keeper/packet.go index bb90d69af..8c49adbc5 100644 --- a/x/ibc/04-channel/keeper/packet.go +++ b/x/ibc/04-channel/keeper/packet.go @@ -292,9 +292,10 @@ func (k Keeper) PacketExecuted( // AcknowledgePacket is called by a module to process the acknowledgement of a // packet previously sent by the calling module on a channel to a counterparty -// module on the counterparty chain. acknowledgePacket also cleans up the packet -// commitment, which is no longer necessary since the packet has been received -// and acted upon. +// module on the counterparty chain. Its intended usage is within the ante +// handler. A subsequent call to AcknowledgementExecuted will clean up the +// packet commitment, which is no longer necessary since the packet has been +// received and acted upon. func (k Keeper) AcknowledgePacket( ctx sdk.Context, packet exported.PacketI, @@ -304,7 +305,10 @@ func (k Keeper) AcknowledgePacket( ) (exported.PacketI, error) { channel, found := k.GetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) if !found { - return nil, sdkerrors.Wrap(types.ErrChannelNotFound, packet.GetSourceChannel()) + return nil, sdkerrors.Wrapf( + types.ErrChannelNotFound, + packet.GetSourcePort(), packet.GetSourceChannel(), + ) } if channel.State != types.OPEN { @@ -314,7 +318,7 @@ func (k Keeper) AcknowledgePacket( ) } - // NOTE: RecvPacket is called by the AnteHandler which acts upon the packet.Route(), + // NOTE: AcknowledgePacket is called by the AnteHandler which acts upon the packet.Route(), // so the capability authentication can be omitted here // packet must have been sent to the channel's counterparty @@ -377,6 +381,38 @@ func (k Keeper) AcknowledgePacket( k.SetNextSequenceAck(ctx, packet.GetDestPort(), packet.GetDestChannel(), nextSequenceAck+1) } + // NOTE: the remaining code is located in the AcknowledgementExecuted function + return packet, nil +} + +// AcknowledgementExecuted deletes the packet commitment from this chain. +// It is assumed that the acknowledgement verification has already occurred. +// +// NOTE: this function must be called in the handler +func (k Keeper) AcknowledgementExecuted( + ctx sdk.Context, + chanCap *capability.Capability, + packet exported.PacketI, +) error { + // sanity check + _, found := k.GetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) + if !found { + return sdkerrors.Wrapf( + types.ErrChannelNotFound, + packet.GetSourcePort(), packet.GetSourceChannel(), + ) + } + + capName := host.ChannelCapabilityPath(packet.GetSourcePort(), packet.GetSourceChannel()) + if !k.scopedKeeper.AuthenticateCapability(ctx, chanCap, capName) { + return sdkerrors.Wrap( + types.ErrInvalidChannelCapability, + "channel capability failed authentication", + ) + } + + k.deletePacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) + // log that a packet has been acknowledged k.Logger(ctx).Info(fmt.Sprintf("packet acknowledged: %v", packet)) @@ -394,21 +430,26 @@ func (k Keeper) AcknowledgePacket( ), }) - return packet, nil + return nil } // CleanupPacket is called by a module to remove a received packet commitment // from storage. The receiving end must have already processed the packet // (whether regularly or past timeout). // -// In the ORDERED channel case, CleanupPacket cleans-up a packet on an ordered -// channel by proving that the packet has been received on the other end. +// In the ORDERED channel case, CleanupPacket cleans-up all packets on an ordered +// channel less than or equal to the packet's sequence by proving that the packet +// has been received on the other end. // // In the UNORDERED channel case, CleanupPacket cleans-up a packet on an // unordered channel by proving that the associated acknowledgement has been -//written. +// written. +// +// NOTE: this functionality is not compatible with calling AcknowledgePacket +// and must be handled at the application level. func (k Keeper) CleanupPacket( ctx sdk.Context, + chanCap *capability.Capability, packet exported.PacketI, proof commitmentexported.Proof, proofHeight, @@ -427,17 +468,13 @@ func (k Keeper) CleanupPacket( ) } - // TODO: blocked by #5542 - // capKey, found := k.GetChannelCapability(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) - // if !found { - // return nil, types.ErrChannelCapabilityNotFound - // } - - // portCapabilityKey := sdk.NewKVStoreKey(capKey) - - // if !k.portKeeper.Authenticate(portCapabilityKey, packet.GetSourcePort()) { - // return nil, sdkerrors.Wrapf(port.ErrInvalidPort, "invalid source port: %s", packet.GetSourcePort()) - // } + capName := host.ChannelCapabilityPath(packet.GetSourcePort(), packet.GetSourceChannel()) + if !k.scopedKeeper.AuthenticateCapability(ctx, chanCap, capName) { + return nil, sdkerrors.Wrap( + types.ErrInvalidChannelCapability, + "channel capability failed authentication", + ) + } if packet.GetDestPort() != channel.Counterparty.PortID { return nil, sdkerrors.Wrapf(types.ErrInvalidPacket, @@ -459,40 +496,51 @@ func (k Keeper) CleanupPacket( // check that packet has been received on the other end if nextSequenceRecv <= packet.GetSequence() { - return nil, sdkerrors.Wrap(types.ErrInvalidPacket, "packet already received") + return nil, sdkerrors.Wrap(types.ErrInvalidPacket, "packet hasn't been received") } commitment := k.GetPacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) // verify we sent the packet and haven't cleared it out yet if !bytes.Equal(commitment, types.CommitPacket(packet)) { - return nil, sdkerrors.Wrap(types.ErrInvalidPacket, "packet hasn't been sent") + return nil, sdkerrors.Wrap(types.ErrInvalidPacket, "packet hasn't been sent or has been cleaned up") } - var err error switch channel.Ordering { case types.ORDERED: // check that the recv sequence is as claimed - err = k.connectionKeeper.VerifyNextSequenceRecv( + if err := k.connectionKeeper.VerifyNextSequenceRecv( ctx, connectionEnd, proofHeight, proof, packet.GetDestPort(), packet.GetDestChannel(), nextSequenceRecv, - ) + ); err != nil { + return nil, sdkerrors.Wrap( + client.ErrFailedNextSeqRecvVerification, + err.Error(), + ) + } + + // delete all packet commitments with a sequence less than or equal to the packet's sequence + k.deletePacketCommitmentsLTE(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) + case types.UNORDERED: - err = k.connectionKeeper.VerifyPacketAcknowledgement( + if err := k.connectionKeeper.VerifyPacketAcknowledgement( ctx, connectionEnd, proofHeight, proof, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(), acknowledgement, - ) + ); err != nil { + return nil, sdkerrors.Wrap( + client.ErrFailedPacketAckVerification, + err.Error(), + ) + } + + // delete the packet commitment + k.deletePacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) + default: panic(sdkerrors.Wrapf(types.ErrInvalidChannelOrdering, channel.Ordering.String())) } - if err != nil { - return nil, sdkerrors.Wrap(err, "packet verification failed") - } - - k.deletePacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) - // log that a packet has been acknowledged k.Logger(ctx).Info(fmt.Sprintf("packet cleaned-up: %v", packet)) diff --git a/x/ibc/04-channel/keeper/packet_test.go b/x/ibc/04-channel/keeper/packet_test.go index 379430ec1..9efcd3710 100644 --- a/x/ibc/04-channel/keeper/packet_test.go +++ b/x/ibc/04-channel/keeper/packet_test.go @@ -8,6 +8,7 @@ import ( connection "github.com/cosmos/cosmos-sdk/x/ibc/03-connection" "github.com/cosmos/cosmos-sdk/x/ibc/04-channel/exported" "github.com/cosmos/cosmos-sdk/x/ibc/04-channel/types" + commitmentexported "github.com/cosmos/cosmos-sdk/x/ibc/23-commitment/exported" host "github.com/cosmos/cosmos-sdk/x/ibc/24-host" ) @@ -265,7 +266,7 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { }.GetBytes() testCases := []testCase{ - {"success", func() { + {"success on ordered channel", func() { packet = types.NewPacket(mockSuccessPacket{}.GetBytes(), 1, testPort1, testChannel1, counterparty.GetPortID(), counterparty.GetChannelID(), timeoutHeight, disabledTimeoutTimestamp) suite.chainB.CreateClient(suite.chainA) suite.chainA.CreateClient(suite.chainB) @@ -277,6 +278,19 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { suite.chainA.App.IBCKeeper.ChannelKeeper.SetPacketAcknowledgement(suite.chainA.GetContext(), testPort2, testChannel2, 1, types.CommitAcknowledgement(ack)) suite.chainB.App.IBCKeeper.ChannelKeeper.SetNextSequenceAck(suite.chainB.GetContext(), counterparty.GetPortID(), counterparty.GetChannelID(), 1) }, true}, + {"success on unordered channel", func() { + packet = types.NewPacket(mockSuccessPacket{}.GetBytes(), 1, testPort1, testChannel1, counterparty.GetPortID(), counterparty.GetChannelID(), timeoutHeight, disabledTimeoutTimestamp) + suite.chainB.CreateClient(suite.chainA) + suite.chainA.CreateClient(suite.chainB) + suite.chainB.createConnection(testConnectionIDA, testConnectionIDB, testClientIDA, testClientIDB, connection.OPEN) + suite.chainA.createConnection(testConnectionIDB, testConnectionIDA, testClientIDB, testClientIDA, connection.OPEN) + suite.chainB.createChannel(testPort1, testChannel1, testPort2, testChannel2, types.OPEN, types.UNORDERED, testConnectionIDA) + suite.chainA.createChannel(testPort2, testChannel2, testPort1, testChannel1, types.OPEN, types.UNORDERED, testConnectionIDB) + suite.chainB.App.IBCKeeper.ChannelKeeper.SetPacketCommitment(suite.chainB.GetContext(), testPort1, testChannel1, 1, types.CommitPacket(packet)) + suite.chainA.App.IBCKeeper.ChannelKeeper.SetPacketAcknowledgement(suite.chainA.GetContext(), testPort2, testChannel2, 1, types.CommitAcknowledgement(ack)) + suite.chainB.App.IBCKeeper.ChannelKeeper.SetNextSequenceAck(suite.chainB.GetContext(), counterparty.GetPortID(), counterparty.GetChannelID(), 1) + }, true}, + {"channel not found", func() {}, false}, {"channel not open", func() { packet = types.NewPacket(mockSuccessPacket{}.GetBytes(), 1, testPort1, testChannel1, counterparty.GetPortID(), counterparty.GetChannelID(), timeoutHeight, disabledTimeoutTimestamp) @@ -347,6 +361,7 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { ctx := suite.chainB.GetContext() packetOut, err := suite.chainB.App.IBCKeeper.ChannelKeeper.AcknowledgePacket(ctx, packet, ack, proof, proofHeight+1) + if tc.expPass { suite.Require().NoError(err) suite.Require().NotNil(packetOut) @@ -358,18 +373,89 @@ func (suite *KeeperTestSuite) TestAcknowledgePacket() { } } +// TestAcknowledgementExectued verifies that packet commitments are deleted after +// capabilities are verified. +func (suite *KeeperTestSuite) TestAcknowledgementExecuted() { + sequence := uint64(1) + counterparty := types.NewCounterparty(testPort2, testChannel2) + + var ( + packet types.Packet + chanCap *capability.Capability + ) + + testCases := []testCase{ + {"success ORDERED", func() { + packet = types.NewPacket(mockSuccessPacket{}.GetBytes(), sequence, testPort1, testChannel1, counterparty.GetPortID(), counterparty.GetChannelID(), timeoutHeight, disabledTimeoutTimestamp) + suite.chainA.createChannel(testPort1, testChannel1, testPort2, testChannel2, types.OPEN, types.ORDERED, testConnectionIDA) + suite.chainA.App.IBCKeeper.ChannelKeeper.SetPacketCommitment(suite.chainA.GetContext(), packet.GetSourcePort(), packet.GetSourceChannel(), sequence, types.CommitPacket(packet)) + }, true}, + {"channel not found", func() {}, false}, + {"incorrect capability", func() { + packet = types.NewPacket(mockSuccessPacket{}.GetBytes(), sequence, testPort1, testChannel1, counterparty.GetPortID(), counterparty.GetChannelID(), timeoutHeight, disabledTimeoutTimestamp) + suite.chainA.createChannel(testPort1, testChannel1, testPort2, testChannel2, types.OPEN, types.ORDERED, testConnectionIDA) + chanCap = capability.NewCapability(100) + }, false}, + } + + for i, tc := range testCases { + tc := tc + suite.Run(fmt.Sprintf("Case %s, %d/%d tests", tc.msg, i, len(testCases)), func() { + suite.SetupTest() // reset + + var err error + chanCap, err = suite.chainA.App.ScopedIBCKeeper.NewCapability( + suite.chainA.GetContext(), host.ChannelCapabilityPath(testPort1, testChannel1), + ) + suite.Require().NoError(err, "could not create capability") + + tc.malleate() + + err = suite.chainA.App.IBCKeeper.ChannelKeeper.AcknowledgementExecuted(suite.chainA.GetContext(), chanCap, packet) + pc := suite.chainA.App.IBCKeeper.ChannelKeeper.GetPacketCommitment(suite.chainA.GetContext(), packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) + + if tc.expPass { + suite.NoError(err) + suite.Nil(pc) + } else { + suite.Error(err) + } + }) + } +} func (suite *KeeperTestSuite) TestCleanupPacket() { counterparty := types.NewCounterparty(testPort2, testChannel2) - packetKey := host.KeyPacketAcknowledgement(testPort2, testChannel2, 1) + unorderedPacketKey := host.KeyPacketAcknowledgement(testPort2, testChannel2, 1) + orderedPacketKey := host.KeyNextSequenceRecv(testPort2, testChannel2) + var ( packet types.Packet nextSeqRecv uint64 + ordered bool ) ack := []byte("ack") testCases := []testCase{ - {"success", func() { + {"success on ordered channel", func() { + ordered = true + nextSeqRecv = 6 + suite.chainA.CreateClient(suite.chainB) + suite.chainB.CreateClient(suite.chainA) + suite.chainB.createConnection(testConnectionIDA, testConnectionIDB, testClientIDA, testClientIDB, connection.OPEN) + suite.chainA.createConnection(testConnectionIDB, testConnectionIDA, testClientIDB, testClientIDA, connection.OPEN) + suite.chainB.createChannel(testPort1, testChannel1, testPort2, testChannel2, types.OPEN, types.ORDERED, testConnectionIDA) + suite.chainA.createChannel(testPort2, testChannel2, testPort1, testChannel1, types.OPEN, types.ORDERED, testConnectionIDB) + // create several packet commitments + for i := uint64(1); i < nextSeqRecv; i++ { + packet = types.NewPacket(mockSuccessPacket{}.GetBytes(), i, testPort1, testChannel1, counterparty.GetPortID(), counterparty.GetChannelID(), timeoutHeight, disabledTimeoutTimestamp) + suite.chainB.App.IBCKeeper.ChannelKeeper.SetPacketCommitment(suite.chainB.GetContext(), testPort1, testChannel1, i, types.CommitPacket(packet)) + } + // set next sequence recv + suite.chainA.App.IBCKeeper.ChannelKeeper.SetNextSequenceRecv(suite.chainA.GetContext(), testPort2, testChannel2, nextSeqRecv) + }, true}, + {"success on unordered channel", func() { + ordered = false nextSeqRecv = 10 packet = types.NewPacket(mockSuccessPacket{}.GetBytes(), 1, testPort1, testChannel1, counterparty.GetPortID(), counterparty.GetChannelID(), timeoutHeight, disabledTimeoutTimestamp) suite.chainA.CreateClient(suite.chainB) @@ -383,6 +469,7 @@ func (suite *KeeperTestSuite) TestCleanupPacket() { }, true}, {"channel not found", func() {}, false}, {"channel not open", func() { + ordered = true packet = types.NewPacket(mockSuccessPacket{}.GetBytes(), 1, testPort1, testChannel1, counterparty.GetPortID(), counterparty.GetChannelID(), timeoutHeight, disabledTimeoutTimestamp) suite.chainB.createChannel(testPort1, testChannel1, testPort2, testChannel2, types.CLOSED, types.ORDERED, testConnectionIDA) }, false}, @@ -423,6 +510,7 @@ func (suite *KeeperTestSuite) TestCleanupPacket() { }, false}, {"packet ack verification failed", func() { nextSeqRecv = 10 + ordered = false packet = types.NewPacket(mockSuccessPacket{}.GetBytes(), 1, testPort1, testChannel1, counterparty.GetPortID(), counterparty.GetChannelID(), timeoutHeight, disabledTimeoutTimestamp) suite.chainB.createConnection(testConnectionIDA, testConnectionIDB, testClientIDA, testClientIDB, connection.OPEN) suite.chainB.createChannel(testPort1, testChannel1, testPort2, testChannel2, types.OPEN, types.UNORDERED, testConnectionIDA) @@ -433,6 +521,9 @@ func (suite *KeeperTestSuite) TestCleanupPacket() { for i, tc := range testCases { tc := tc suite.Run(fmt.Sprintf("Case %s, %d/%d tests", tc.msg, i, len(testCases)), func() { + var proof commitmentexported.Proof + var proofHeight uint64 + suite.SetupTest() // reset tc.malleate() @@ -440,14 +531,32 @@ func (suite *KeeperTestSuite) TestCleanupPacket() { suite.chainB.updateClient(suite.chainA) suite.chainA.updateClient(suite.chainB) - proof, proofHeight := queryProof(suite.chainA, packetKey) + + if ordered { + proof, proofHeight = queryProof(suite.chainA, orderedPacketKey) + } else { + proof, proofHeight = queryProof(suite.chainA, unorderedPacketKey) + } + + cap, err := suite.chainB.App.ScopedIBCKeeper.NewCapability(ctx, host.ChannelCapabilityPath(testPort1, testChannel1)) + suite.Require().NoError(err) if tc.expPass { - packetOut, err := suite.chainB.App.IBCKeeper.ChannelKeeper.CleanupPacket(ctx, packet, proof, proofHeight+1, nextSeqRecv, ack) + packetOut, err := suite.chainB.App.IBCKeeper.ChannelKeeper.CleanupPacket(ctx, cap, packet, proof, proofHeight+1, nextSeqRecv, ack) suite.Require().NoError(err) suite.Require().NotNil(packetOut) + + if ordered { + for i := uint64(1); i < nextSeqRecv; i++ { + pc := suite.chainB.App.IBCKeeper.ChannelKeeper.GetPacketCommitment(ctx, testPort1, testChannel1, i) + suite.Require().Nil(pc) + } + } else { + pc := suite.chainB.App.IBCKeeper.ChannelKeeper.GetPacketCommitment(ctx, testPort1, testChannel1, packet.GetSequence()) + suite.Require().Nil(pc) + } } else { - packetOut, err := suite.chainB.App.IBCKeeper.ChannelKeeper.CleanupPacket(ctx, packet, proof, proofHeight, nextSeqRecv, ack) + packetOut, err := suite.chainB.App.IBCKeeper.ChannelKeeper.CleanupPacket(ctx, cap, packet, proof, proofHeight, nextSeqRecv, ack) suite.Require().Error(err) suite.Require().Nil(packetOut) } diff --git a/x/ibc/04-channel/keeper/timeout.go b/x/ibc/04-channel/keeper/timeout.go index 70d427347..f2b5cc82d 100644 --- a/x/ibc/04-channel/keeper/timeout.go +++ b/x/ibc/04-channel/keeper/timeout.go @@ -18,7 +18,8 @@ import ( // packet to a counterparty module, where the timeout height has passed on the // counterparty chain without the packet being committed, to prove that the // packet can no longer be executed and to allow the calling module to safely -// perform appropriate state transitions. +// perform appropriate state transitions. Its intended usage is within the +// ante handler. func (k Keeper) TimeoutPacket( ctx sdk.Context, packet exported.PacketI, @@ -109,6 +110,39 @@ func (k Keeper) TimeoutPacket( return nil, err } + // NOTE: the remaining code is located in the TimeoutExecuted function + return packet, nil +} + +// TimeoutExecuted deletes the commitment send from this chain after it verifies timeout. +// If the timed-out packet came from an ORDERED channel then this channel will be closed. +// +// NOTE: this function must be called in the handler +func (k Keeper) TimeoutExecuted( + ctx sdk.Context, + chanCap *capability.Capability, + packet exported.PacketI, +) error { + channel, found := k.GetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) + if !found { + return sdkerrors.Wrapf(types.ErrChannelNotFound, packet.GetSourcePort(), packet.GetSourceChannel()) + } + + capName := host.ChannelCapabilityPath(packet.GetSourcePort(), packet.GetSourceChannel()) + if !k.scopedKeeper.AuthenticateCapability(ctx, chanCap, capName) { + return sdkerrors.Wrap( + types.ErrChannelCapabilityNotFound, + "caller does not own capability for channel", + ) + } + + k.deletePacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) + + if channel.Ordering == types.ORDERED { + channel.State = types.CLOSED + k.SetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), channel) + } + k.Logger(ctx).Info(fmt.Sprintf("packet timed-out: %v", packet)) // emit an event marking that we have processed the timeout @@ -125,28 +159,6 @@ func (k Keeper) TimeoutPacket( ), }) - // NOTE: the remaining code is located on the TimeoutExecuted function - return packet, nil -} - -// TimeoutExecuted deletes the commitment send from this chain after it verifies timeout -func (k Keeper) TimeoutExecuted(ctx sdk.Context, chanCap *capability.Capability, packet exported.PacketI) error { - channel, found := k.GetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) - if !found { - return sdkerrors.Wrapf(types.ErrChannelNotFound, packet.GetSourcePort(), packet.GetSourceChannel()) - } - - if !k.scopedKeeper.AuthenticateCapability(ctx, chanCap, host.ChannelCapabilityPath(packet.GetSourcePort(), packet.GetSourceChannel())) { - return sdkerrors.Wrap(types.ErrChannelCapabilityNotFound, "caller does not own capability for channel") - } - - k.deletePacketCommitment(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence()) - - if channel.Ordering == types.ORDERED { - channel.State = types.CLOSED - k.SetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), channel) - } - return nil } @@ -155,7 +167,8 @@ func (k Keeper) TimeoutExecuted(ctx sdk.Context, chanCap *capability.Capability, // never be received (even if the timeoutHeight has not yet been reached). func (k Keeper) TimeoutOnClose( ctx sdk.Context, - packet types.Packet, // nolint: interfacer + chanCap *capability.Capability, + packet exported.PacketI, proof, proofClosed commitmentexported.Proof, proofHeight, @@ -166,17 +179,13 @@ func (k Keeper) TimeoutOnClose( return nil, sdkerrors.Wrapf(types.ErrChannelNotFound, packet.GetSourcePort(), packet.GetSourceChannel()) } - // TODO: blocked by #5542 - // capKey, found := k.GetChannelCapability(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) - // if !found { - // return nil, types.ErrChannelCapabilityNotFound - // } - - // portCapabilityKey := sdk.NewKVStoreKey(capKey) - - // if !k.portKeeper.Authenticate(portCapabilityKey, packet.GetSourcePort()) { - // return nil, sdkerrors.Wrap(port.ErrInvalidPort, packet.GetSourcePort()) - // } + capName := host.ChannelCapabilityPath(packet.GetSourcePort(), packet.GetSourceChannel()) + if !k.scopedKeeper.AuthenticateCapability(ctx, chanCap, capName) { + return nil, sdkerrors.Wrap( + types.ErrInvalidChannelCapability, + "channel capability failed authentication", + ) + } if packet.GetDestPort() != channel.Counterparty.PortID { return nil, sdkerrors.Wrapf( @@ -227,6 +236,11 @@ func (k Keeper) TimeoutOnClose( var err error switch channel.Ordering { case types.ORDERED: + // check that packet has not been received + if nextSequenceRecv > packet.GetSequence() { + return nil, sdkerrors.Wrap(types.ErrInvalidPacket, "packet already received") + } + // check that the recv sequence is as claimed err = k.connectionKeeper.VerifyNextSequenceRecv( ctx, connectionEnd, proofHeight, proof, @@ -235,7 +249,7 @@ func (k Keeper) TimeoutOnClose( case types.UNORDERED: err = k.connectionKeeper.VerifyPacketAcknowledgementAbsence( ctx, connectionEnd, proofHeight, proof, - packet.GetSourcePort(), packet.GetSourceChannel(), packet.GetSequence(), + packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(), ) default: panic(sdkerrors.Wrapf(types.ErrInvalidChannelOrdering, channel.Ordering.String())) diff --git a/x/ibc/04-channel/keeper/timeout_test.go b/x/ibc/04-channel/keeper/timeout_test.go index ebc7a1391..deb5c52a5 100644 --- a/x/ibc/04-channel/keeper/timeout_test.go +++ b/x/ibc/04-channel/keeper/timeout_test.go @@ -6,6 +6,7 @@ import ( "github.com/cosmos/cosmos-sdk/x/capability" connection "github.com/cosmos/cosmos-sdk/x/ibc/03-connection" "github.com/cosmos/cosmos-sdk/x/ibc/04-channel/types" + commitmentexported "github.com/cosmos/cosmos-sdk/x/ibc/23-commitment/exported" host "github.com/cosmos/cosmos-sdk/x/ibc/24-host" ) @@ -104,14 +105,21 @@ func (suite *KeeperTestSuite) TestTimeoutPacket() { } } +// TestTimeoutExectued verifies that packet commitments are deleted after +// capabilities are verified. func (suite *KeeperTestSuite) TestTimeoutExecuted() { - var packet types.Packet + sequence := uint64(1) + + var ( + packet types.Packet + chanCap *capability.Capability + ) - var chanCap *capability.Capability testCases := []testCase{ {"success ORDERED", func() { packet = types.NewPacket(newMockTimeoutPacket().GetBytes(), 1, testPort1, testChannel1, testPort2, testChannel2, timeoutHeight, disabledTimeoutTimestamp) suite.chainA.createChannel(testPort1, testChannel1, testPort2, testChannel2, types.OPEN, types.ORDERED, testConnectionIDA) + suite.chainA.App.IBCKeeper.ChannelKeeper.SetPacketCommitment(suite.chainA.GetContext(), packet.GetSourcePort(), packet.GetSourceChannel(), sequence, types.CommitPacket(packet)) }, true}, {"channel not found", func() {}, false}, {"incorrect capability", func() { @@ -135,11 +143,13 @@ func (suite *KeeperTestSuite) TestTimeoutExecuted() { tc.malleate() err = suite.chainA.App.IBCKeeper.ChannelKeeper.TimeoutExecuted(suite.chainA.GetContext(), chanCap, packet) + pc := suite.chainA.App.IBCKeeper.ChannelKeeper.GetPacketCommitment(suite.chainA.GetContext(), packet.GetSourcePort(), packet.GetSourceChannel(), sequence) if tc.expPass { - suite.Require().NoError(err) + suite.NoError(err) + suite.Nil(pc) } else { - suite.Require().Error(err) + suite.Error(err) } }) } @@ -147,15 +157,31 @@ func (suite *KeeperTestSuite) TestTimeoutExecuted() { func (suite *KeeperTestSuite) TestTimeoutOnClose() { channelKey := host.KeyChannel(testPort2, testChannel2) - packetKey := host.KeyPacketAcknowledgement(testPort1, testChannel1, 2) + unorderedPacketKey := host.KeyPacketAcknowledgement(testPort2, testChannel2, 2) + orderedPacketKey := host.KeyNextSequenceRecv(testPort2, testChannel2) + counterparty := types.NewCounterparty(testPort2, testChannel2) var ( packet types.Packet nextSeqRecv uint64 + ordered bool ) testCases := []testCase{ - {"success", func() { + {"success on ordered channel", func() { + ordered = true + packet = types.NewPacket(mockSuccessPacket{}.GetBytes(), 2, testPort1, testChannel1, counterparty.GetPortID(), counterparty.GetChannelID(), timeoutHeight, disabledTimeoutTimestamp) + suite.chainB.CreateClient(suite.chainA) + suite.chainA.CreateClient(suite.chainB) + suite.chainB.createConnection(testConnectionIDA, testConnectionIDB, testClientIDA, testClientIDB, connection.OPEN) + suite.chainA.createConnection(testConnectionIDB, testConnectionIDA, testClientIDB, testClientIDA, connection.OPEN) + suite.chainB.createChannel(testPort1, testChannel1, testPort2, testChannel2, types.OPEN, types.ORDERED, testConnectionIDA) + suite.chainA.createChannel(testPort2, testChannel2, testPort1, testChannel1, types.CLOSED, types.ORDERED, testConnectionIDB) // channel on chainA is closed + suite.chainB.App.IBCKeeper.ChannelKeeper.SetPacketCommitment(suite.chainB.GetContext(), testPort1, testChannel1, 2, types.CommitPacket(packet)) + suite.chainA.App.IBCKeeper.ChannelKeeper.SetNextSequenceRecv(suite.chainA.GetContext(), testPort2, testChannel2, nextSeqRecv) + }, true}, + {"success on unordered channel", func() { + ordered = false packet = types.NewPacket(mockSuccessPacket{}.GetBytes(), 2, testPort1, testChannel1, counterparty.GetPortID(), counterparty.GetChannelID(), timeoutHeight, disabledTimeoutTimestamp) suite.chainB.CreateClient(suite.chainA) suite.chainA.CreateClient(suite.chainB) @@ -164,10 +190,10 @@ func (suite *KeeperTestSuite) TestTimeoutOnClose() { suite.chainB.createChannel(testPort1, testChannel1, testPort2, testChannel2, types.OPEN, types.UNORDERED, testConnectionIDA) suite.chainA.createChannel(testPort2, testChannel2, testPort1, testChannel1, types.CLOSED, types.UNORDERED, testConnectionIDB) // channel on chainA is closed suite.chainB.App.IBCKeeper.ChannelKeeper.SetPacketCommitment(suite.chainB.GetContext(), testPort1, testChannel1, 2, types.CommitPacket(packet)) - suite.chainB.App.IBCKeeper.ChannelKeeper.SetNextSequenceRecv(suite.chainB.GetContext(), testPort1, testChannel1, nextSeqRecv) }, true}, {"channel not found", func() {}, false}, {"packet dest port ≠ channel counterparty port", func() { + ordered = true packet = types.NewPacket(mockSuccessPacket{}.GetBytes(), 1, testPort1, testChannel1, counterparty.GetPortID(), counterparty.GetChannelID(), timeoutHeight, disabledTimeoutTimestamp) suite.chainB.createChannel(testPort1, testChannel1, testPort3, testChannel2, types.OPEN, types.ORDERED, testConnectionIDA) }, false}, @@ -185,6 +211,7 @@ func (suite *KeeperTestSuite) TestTimeoutOnClose() { suite.chainB.createChannel(testPort1, testChannel1, testPort2, testChannel2, types.OPEN, types.ORDERED, testConnectionIDA) }, false}, {"channel verification failed", func() { + ordered = false packet = types.NewPacket(mockSuccessPacket{}.GetBytes(), 2, testPort1, testChannel1, counterparty.GetPortID(), counterparty.GetChannelID(), timeoutHeight, disabledTimeoutTimestamp) suite.chainB.CreateClient(suite.chainA) suite.chainB.createConnection(testConnectionIDA, testConnectionIDB, testClientIDA, testClientIDB, connection.OPEN) @@ -193,6 +220,7 @@ func (suite *KeeperTestSuite) TestTimeoutOnClose() { suite.chainB.App.IBCKeeper.ChannelKeeper.SetNextSequenceRecv(suite.chainB.GetContext(), testPort1, testChannel1, nextSeqRecv) }, false}, {"next seq receive verification failed", func() { + ordered = true packet = types.NewPacket(mockSuccessPacket{}.GetBytes(), 2, testPort1, testChannel1, counterparty.GetPortID(), counterparty.GetChannelID(), timeoutHeight, disabledTimeoutTimestamp) suite.chainB.CreateClient(suite.chainA) suite.chainB.createConnection(testConnectionIDA, testConnectionIDB, testClientIDA, testClientIDB, connection.OPEN) @@ -201,6 +229,7 @@ func (suite *KeeperTestSuite) TestTimeoutOnClose() { suite.chainB.App.IBCKeeper.ChannelKeeper.SetNextSequenceRecv(suite.chainB.GetContext(), testPort1, testChannel1, nextSeqRecv) }, false}, {"packet ack verification failed", func() { + ordered = false packet = types.NewPacket(mockSuccessPacket{}.GetBytes(), 2, testPort1, testChannel1, counterparty.GetPortID(), counterparty.GetChannelID(), timeoutHeight, disabledTimeoutTimestamp) suite.chainB.CreateClient(suite.chainA) suite.chainB.createConnection(testConnectionIDA, testConnectionIDB, testClientIDA, testClientIDB, connection.OPEN) @@ -213,22 +242,32 @@ func (suite *KeeperTestSuite) TestTimeoutOnClose() { for i, tc := range testCases { tc := tc suite.Run(fmt.Sprintf("Case %s, %d/%d tests", tc.msg, i, len(testCases)), func() { + var proof commitmentexported.Proof + suite.SetupTest() // reset tc.malleate() suite.chainB.updateClient(suite.chainA) suite.chainA.updateClient(suite.chainB) proofClosed, proofHeight := queryProof(suite.chainA, channelKey) - proofAckAbsence, _ := queryProof(suite.chainA, packetKey) + + if ordered { + proof, _ = queryProof(suite.chainA, orderedPacketKey) + } else { + proof, _ = queryProof(suite.chainA, unorderedPacketKey) + } ctx := suite.chainB.GetContext() + cap, err := suite.chainB.App.ScopedIBCKeeper.NewCapability(ctx, host.ChannelCapabilityPath(testPort1, testChannel1)) + suite.Require().NoError(err) + if tc.expPass { - packetOut, err := suite.chainB.App.IBCKeeper.ChannelKeeper.TimeoutOnClose(ctx, packet, proofAckAbsence, proofClosed, proofHeight+1, nextSeqRecv) + packetOut, err := suite.chainB.App.IBCKeeper.ChannelKeeper.TimeoutOnClose(ctx, cap, packet, proof, proofClosed, proofHeight+1, nextSeqRecv) suite.Require().NoError(err) suite.Require().NotNil(packetOut) } else { // switch the proofs to invalidate them - packetOut, err := suite.chainB.App.IBCKeeper.ChannelKeeper.TimeoutOnClose(ctx, packet, proofClosed, proofAckAbsence, proofHeight+1, nextSeqRecv) + packetOut, err := suite.chainB.App.IBCKeeper.ChannelKeeper.TimeoutOnClose(ctx, cap, packet, proofClosed, proof, proofHeight+1, nextSeqRecv) suite.Require().Error(err) suite.Require().Nil(packetOut) } diff --git a/x/ibc/handler.go b/x/ibc/handler.go index f5f3f4b7d..acb34031c 100644 --- a/x/ibc/handler.go +++ b/x/ibc/handler.go @@ -171,7 +171,7 @@ func NewHandler(k Keeper) sdk.Handler { case channel.MsgAcknowledgement: // Lookup module by channel capability - module, _, err := k.ChannelKeeper.LookupModuleByChannel(ctx, msg.Packet.SourcePort, msg.Packet.SourceChannel) + module, cap, err := k.ChannelKeeper.LookupModuleByChannel(ctx, msg.Packet.SourcePort, msg.Packet.SourceChannel) if err != nil { return nil, sdkerrors.Wrap(err, "could not retrieve module from port-id") } @@ -181,7 +181,19 @@ func NewHandler(k Keeper) sdk.Handler { if !ok { return nil, sdkerrors.Wrapf(port.ErrInvalidRoute, "route not found to module: %s", module) } - return cbs.OnAcknowledgementPacket(ctx, msg.Packet, msg.Acknowledgement) + + // Perform application logic callback + res, err := cbs.OnAcknowledgementPacket(ctx, msg.Packet, msg.Acknowledgement) + if err != nil { + return nil, err + } + + // Delete packet commitment + if err = k.ChannelKeeper.AcknowledgementExecuted(ctx, cap, msg.Packet); err != nil { + return nil, err + } + + return res, nil case channel.MsgTimeout: // Lookup module by channel capability @@ -195,16 +207,19 @@ func NewHandler(k Keeper) sdk.Handler { if !ok { return nil, sdkerrors.Wrapf(port.ErrInvalidRoute, "route not found to module: %s", module) } + + // Perform application logic callback res, err := cbs.OnTimeoutPacket(ctx, msg.Packet) if err != nil { + return nil, err + } + // Delete packet commitment + if err = k.ChannelKeeper.TimeoutExecuted(ctx, cap, msg.Packet); err != nil { return nil, err } - err = k.ChannelKeeper.TimeoutExecuted(ctx, cap, msg.Packet) - if err != nil { - return nil, err - } - return res, err + + return res, nil default: return nil, sdkerrors.Wrapf(sdkerrors.ErrUnknownRequest, "unrecognized IBC message type: %T", msg)