From 1c575c95b2329df8713935949aadb9c1aee2c386 Mon Sep 17 00:00:00 2001 From: colin axner <25233464+colin-axner@users.noreply.github.com> Date: Mon, 29 Jun 2020 10:54:45 +0200 Subject: [PATCH] Remove CleanupPacket from 04-channel (#6533) * remove cleanup func * remove unused func --- x/ibc/04-channel/alias.go | 1 - x/ibc/04-channel/keeper/keeper.go | 13 -- x/ibc/04-channel/keeper/packet.go | 141 +-------------- x/ibc/04-channel/keeper/packet_test.go | 234 ++----------------------- x/ibc/04-channel/types/errors.go | 1 + x/ibc/04-channel/types/events.go | 1 - 6 files changed, 20 insertions(+), 371 deletions(-) diff --git a/x/ibc/04-channel/alias.go b/x/ibc/04-channel/alias.go index 91175a394a..89b67e6523 100644 --- a/x/ibc/04-channel/alias.go +++ b/x/ibc/04-channel/alias.go @@ -19,7 +19,6 @@ const ( EventTypeSendPacket = types.EventTypeSendPacket EventTypeRecvPacket = types.EventTypeRecvPacket EventTypeAcknowledgePacket = types.EventTypeAcknowledgePacket - EventTypeCleanupPacket = types.EventTypeCleanupPacket EventTypeTimeoutPacket = types.EventTypeTimeoutPacket AttributeKeyData = types.AttributeKeyData AttributeKeyAck = types.AttributeKeyAck diff --git a/x/ibc/04-channel/keeper/keeper.go b/x/ibc/04-channel/keeper/keeper.go index ee78db2d85..1d1c7c4ae2 100644 --- a/x/ibc/04-channel/keeper/keeper.go +++ b/x/ibc/04-channel/keeper/keeper.go @@ -145,19 +145,6 @@ 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/packet.go b/x/ibc/04-channel/keeper/packet.go index 125a1a7827..d89871d809 100644 --- a/x/ibc/04-channel/keeper/packet.go +++ b/x/ibc/04-channel/keeper/packet.go @@ -255,13 +255,16 @@ func (k Keeper) PacketExecuted( ) } - if len(acknowledgement) > 0 || channel.Ordering == types.UNORDERED { - k.SetPacketAcknowledgement( - ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(), - types.CommitAcknowledgement(acknowledgement), - ) + if len(acknowledgement) == 0 { + return sdkerrors.Wrap(types.ErrInvalidAcknowledgement, "acknowledgement cannot be empty") } + // always set the acknowledgement so that it can be verified on the other side + k.SetPacketAcknowledgement( + ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(), + types.CommitAcknowledgement(acknowledgement), + ) + if channel.Ordering == types.ORDERED { nextSequenceRecv, found := k.GetNextSequenceRecv(ctx, packet.GetDestPort(), packet.GetDestChannel()) if !found { @@ -447,131 +450,3 @@ func (k Keeper) AcknowledgementExecuted( 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 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. -// -// 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 *capabilitytypes.Capability, - packet exported.PacketI, - proof []byte, - proofHeight, - nextSequenceRecv uint64, - acknowledgement []byte, -) (exported.PacketI, error) { - channel, found := k.GetChannel(ctx, packet.GetSourcePort(), packet.GetSourceChannel()) - if !found { - return nil, sdkerrors.Wrap(types.ErrChannelNotFound, packet.GetSourceChannel()) - } - - if channel.State != types.OPEN { - return nil, sdkerrors.Wrapf( - types.ErrInvalidChannelState, - "channel state is not OPEN (got %s)", channel.State.String(), - ) - } - - 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, - "packet destination port doesn't match the counterparty's port (%s ≠ %s)", packet.GetDestPort(), channel.Counterparty.PortID, - ) - } - - if packet.GetDestChannel() != channel.Counterparty.ChannelID { - return nil, sdkerrors.Wrapf( - types.ErrInvalidPacket, - "packet destination channel doesn't match the counterparty's channel (%s ≠ %s)", packet.GetDestChannel(), channel.Counterparty.ChannelID, - ) - } - - connectionEnd, found := k.connectionKeeper.GetConnection(ctx, channel.ConnectionHops[0]) - if !found { - return nil, sdkerrors.Wrap(connection.ErrConnectionNotFound, channel.ConnectionHops[0]) - } - - // check that packet has been received on the other end - if nextSequenceRecv <= packet.GetSequence() { - 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 or has been cleaned up") - } - - switch channel.Ordering { - case types.ORDERED: - // check that the recv sequence is as claimed - 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: - 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())) - } - - // log that a packet has been acknowledged - k.Logger(ctx).Info(fmt.Sprintf("packet cleaned-up: %v", packet)) - - // emit an event marking that we have cleaned up the packet - ctx.EventManager().EmitEvents(sdk.Events{ - sdk.NewEvent( - types.EventTypeCleanupPacket, - sdk.NewAttribute(types.AttributeKeyTimeoutHeight, fmt.Sprintf("%d", packet.GetTimeoutHeight())), - 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()), - ), - }) - - return packet, nil -} diff --git a/x/ibc/04-channel/keeper/packet_test.go b/x/ibc/04-channel/keeper/packet_test.go index 5718290630..89bf281852 100644 --- a/x/ibc/04-channel/keeper/packet_test.go +++ b/x/ibc/04-channel/keeper/packet_test.go @@ -347,6 +347,7 @@ func (suite *KeeperTestSuite) TestPacketExecuted() { var ( packet types.Packet channelCap *capabilitytypes.Capability + ack []byte ) testCases := []testCase{ @@ -410,16 +411,24 @@ func (suite *KeeperTestSuite) TestPacketExecuted() { channelCap = capabilitytypes.NewCapability(3) }, false}, + {"acknowledgement is empty", func() { + _, clientB, _, _, channelA, channelB := suite.coordinator.Setup(suite.chainA, suite.chainB) + packet = types.NewPacket(validPacketData, 1, channelA.PortID, channelA.ID, channelB.PortID, channelB.ID, timeoutHeight, disabledTimeoutTimestamp) + suite.coordinator.SendPacket(suite.chainA, suite.chainB, packet, clientB) + channelCap = suite.chainB.GetChannelCapability(channelB.PortID, channelB.ID) + + ack = []byte{} + }, 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 + suite.SetupTest() // reset + ack = ibctesting.TestHash // must explicity be changed in malleate tc.malleate() - ack := ibctesting.TestHash err := suite.chainB.App.IBCKeeper.ChannelKeeper.PacketExecuted(suite.chainB.GetContext(), channelCap, packet, ack) if tc.expPass { @@ -639,224 +648,3 @@ func (suite *KeeperTestSuite) TestAcknowledgementExecuted() { }) } } - -// TestCleanupPacket tests calling CleanupPacket on chainA. Each test case must specifiy the -// channel ordering so the appropriate proof can be created. It must also specifiy the next -// sequence receive so that correct amount of deleted packet commitments can be checked for. -// The last sent/received packet and the channel capability are expected to be defined. -func (suite *KeeperTestSuite) TestCleanupPacket() { - var ( - packet types.Packet - nextSeqRecv uint64 - ordered bool - channelCap *capabilitytypes.Capability - ack = ibctesting.TestHash - ) - - testCases := []testCase{ - {"success: ORDERED channel", func() { - ordered = true - nextSeqRecv = 6 - - clientA, clientB, connA, connB := suite.coordinator.SetupClientConnections(suite.chainA, suite.chainB, clientexported.Tendermint) - channelA, channelB := suite.coordinator.CreateChannel(suite.chainA, suite.chainB, connA, connB, types.ORDERED) - - // create several send and receives - for i := uint64(1); i < nextSeqRecv; i++ { - packet = types.NewPacket(validPacketData, i, channelA.PortID, channelA.ID, channelB.PortID, channelB.ID, timeoutHeight, disabledTimeoutTimestamp) - - // create packet commitment - err := suite.coordinator.SendPacket(suite.chainA, suite.chainB, packet, clientB) - suite.Require().NoError(err) - - // create packet acknowledgement - err = suite.coordinator.PacketExecuted(suite.chainB, suite.chainA, packet, clientA) - suite.Require().NoError(err) - } - channelCap = suite.chainA.GetChannelCapability(channelA.PortID, channelA.ID) - }, true}, - {"success: UNORDERED channel", func() { - ordered = false - nextSeqRecv = 5 - - // setup uses an UNORDERED channel - clientA, clientB, _, _, channelA, channelB := suite.coordinator.Setup(suite.chainA, suite.chainB) - - for i := uint64(1); i < nextSeqRecv; i++ { - packet = types.NewPacket(validPacketData, i, channelA.PortID, channelA.ID, channelB.PortID, channelB.ID, timeoutHeight, disabledTimeoutTimestamp) - - // create packet commitment - err := suite.coordinator.SendPacket(suite.chainA, suite.chainB, packet, clientB) - suite.Require().NoError(err) - - // create packet acknowledgement - err = suite.coordinator.PacketExecuted(suite.chainB, suite.chainA, packet, clientA) - suite.Require().NoError(err) - } - channelCap = suite.chainA.GetChannelCapability(channelA.PortID, channelA.ID) - }, true}, - {"channel not found", func() { - // use wrong channel naming - _, _, _, _, channelA, channelB := suite.coordinator.Setup(suite.chainA, suite.chainB) - packet = types.NewPacket(validPacketData, 1, ibctesting.InvalidID, ibctesting.InvalidID, channelB.PortID, channelB.ID, timeoutHeight, disabledTimeoutTimestamp) - channelCap = suite.chainA.GetChannelCapability(channelA.PortID, channelA.ID) - }, false}, - {"channel not open", func() { - _, _, _, _, channelA, channelB := suite.coordinator.Setup(suite.chainA, suite.chainB) - packet = types.NewPacket(validPacketData, 1, channelA.PortID, channelA.ID, channelB.PortID, channelB.ID, timeoutHeight, disabledTimeoutTimestamp) - - err := suite.coordinator.SetChannelClosed(suite.chainA, suite.chainB, channelA) - suite.Require().NoError(err) - }, false}, - {"packet destination port ≠ channel counterparty port", func() { - _, _, _, _, channelA, channelB := suite.coordinator.Setup(suite.chainA, suite.chainB) - // use wrong port for dest - packet = types.NewPacket(validPacketData, 1, channelA.PortID, channelA.ID, ibctesting.InvalidID, channelB.ID, timeoutHeight, disabledTimeoutTimestamp) - channelCap = suite.chainA.GetChannelCapability(channelA.PortID, channelA.ID) - }, false}, - {"packet destination channel ID ≠ channel counterparty channel ID", func() { - _, _, _, _, channelA, channelB := suite.coordinator.Setup(suite.chainA, suite.chainB) - // use wrong channel for dest - packet = types.NewPacket(validPacketData, 1, channelA.PortID, channelA.ID, channelB.PortID, ibctesting.InvalidID, timeoutHeight, disabledTimeoutTimestamp) - channelCap = suite.chainA.GetChannelCapability(channelA.PortID, channelA.ID) - }, false}, - {"connection not found", func() { - channelA := ibctesting.TestChannel{PortID: portID, ID: channelIDA} - channelB := ibctesting.TestChannel{PortID: portID, ID: channelIDB} - // pass channel check - suite.chainA.App.IBCKeeper.ChannelKeeper.SetChannel( - suite.chainA.GetContext(), - channelA.PortID, channelA.ID, - types.NewChannel(types.OPEN, types.ORDERED, types.NewCounterparty(channelB.PortID, channelB.ID), []string{connIDA}, ibctesting.ChannelVersion), - ) - suite.chainA.CreateChannelCapability(channelA.PortID, channelA.ID) - channelCap = suite.chainA.GetChannelCapability(channelA.PortID, channelA.ID) - }, false}, - {"connection not OPEN", func() { - clientA, clientB := suite.coordinator.SetupClients(suite.chainA, suite.chainB, clientexported.Tendermint) - // connection on chainA is in INIT - connA, connB, err := suite.coordinator.ConnOpenInit(suite.chainA, suite.chainB, clientA, clientB) - suite.Require().NoError(err) - - channelA := connA.NextTestChannel() - channelB := connB.NextTestChannel() - // pass channel check - suite.chainA.App.IBCKeeper.ChannelKeeper.SetChannel( - suite.chainA.GetContext(), - channelA.PortID, channelA.ID, - types.NewChannel(types.OPEN, types.ORDERED, types.NewCounterparty(channelB.PortID, channelB.ID), []string{connA.ID}, ibctesting.ChannelVersion), - ) - packet = types.NewPacket(validPacketData, 1, channelA.PortID, channelA.ID, channelB.PortID, channelB.ID, timeoutHeight, disabledTimeoutTimestamp) - - suite.chainA.CreateChannelCapability(channelA.PortID, channelA.ID) - channelCap = suite.chainA.GetChannelCapability(channelA.PortID, channelA.ID) - }, false}, - {"packet hasn't been received", func() { - ordered = true - nextSeqRecv = 6 - - _, clientB, connA, connB := suite.coordinator.SetupClientConnections(suite.chainA, suite.chainB, clientexported.Tendermint) - channelA, channelB := suite.coordinator.CreateChannel(suite.chainA, suite.chainB, connA, connB, types.ORDERED) - - for i := uint64(1); i < nextSeqRecv; i++ { - packet = types.NewPacket(validPacketData, i, channelA.PortID, channelA.ID, channelB.PortID, channelB.ID, timeoutHeight, disabledTimeoutTimestamp) - - // create packet commitment - err := suite.coordinator.SendPacket(suite.chainA, suite.chainB, packet, clientB) - suite.Require().NoError(err) - } - channelCap = suite.chainA.GetChannelCapability(channelA.PortID, channelA.ID) - }, false}, - {"packet hasn't been sent", func() { - ordered = false - nextSeqRecv = 5 - - _, _, _, _, channelA, channelB := suite.coordinator.Setup(suite.chainA, suite.chainB) - - packet = types.NewPacket(validPacketData, nextSeqRecv-1, channelA.PortID, channelA.ID, channelB.PortID, channelB.ID, timeoutHeight, disabledTimeoutTimestamp) - channelCap = suite.chainA.GetChannelCapability(channelA.PortID, channelA.ID) - }, false}, - {"next seq receive verification failed: ORDERED channel", func() { - // set ordered to false giving wrong proof - ordered = false - nextSeqRecv = 2 - - clientA, clientB, connA, connB := suite.coordinator.SetupClientConnections(suite.chainA, suite.chainB, clientexported.Tendermint) - channelA, channelB := suite.coordinator.CreateChannel(suite.chainA, suite.chainB, connA, connB, types.ORDERED) - - // create several send and receives - for i := uint64(1); i < nextSeqRecv; i++ { - packet = types.NewPacket(validPacketData, i, channelA.PortID, channelA.ID, channelB.PortID, channelB.ID, timeoutHeight, disabledTimeoutTimestamp) - - // create packet commitment - err := suite.coordinator.SendPacket(suite.chainA, suite.chainB, packet, clientB) - suite.Require().NoError(err) - - // create packet acknowledgement - err = suite.coordinator.PacketExecuted(suite.chainB, suite.chainA, packet, clientA) - suite.Require().NoError(err) - } - channelCap = suite.chainA.GetChannelCapability(channelA.PortID, channelA.ID) - }, false}, - {"packet ack verification failed: UNORDERED channel", func() { - // set ordered to true giving wrong proof - ordered = true - nextSeqRecv = 5 - - clientA, clientB, _, _, channelA, channelB := suite.coordinator.Setup(suite.chainA, suite.chainB) - - for i := uint64(1); i < nextSeqRecv; i++ { - packet = types.NewPacket(validPacketData, i, channelA.PortID, channelA.ID, channelB.PortID, channelB.ID, timeoutHeight, disabledTimeoutTimestamp) - - // create packet commitment - err := suite.coordinator.SendPacket(suite.chainA, suite.chainB, packet, clientB) - suite.Require().NoError(err) - - // create packet acknowledgement - err = suite.coordinator.PacketExecuted(suite.chainB, suite.chainA, packet, clientA) - suite.Require().NoError(err) - } - channelCap = suite.chainA.GetChannelCapability(channelA.PortID, channelA.ID) - }, false}, - } - - for i, tc := range testCases { - tc := tc - suite.Run(fmt.Sprintf("Case %s, %d/%d tests", tc.msg, i, len(testCases)), func() { - var ( - proof []byte - proofHeight uint64 - ) - - suite.SetupTest() // reset - tc.malleate() - - orderedPacketKey := host.KeyNextSequenceRecv(packet.GetDestPort(), packet.GetDestChannel()) - unorderedPacketKey := host.KeyPacketAcknowledgement(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence()) - - if ordered { - proof, proofHeight = suite.chainB.QueryProof(orderedPacketKey) - } else { - proof, proofHeight = suite.chainB.QueryProof(unorderedPacketKey) - } - - _, err := suite.chainA.App.IBCKeeper.ChannelKeeper.CleanupPacket(suite.chainA.GetContext(), channelCap, packet, proof, proofHeight, nextSeqRecv, ack) - - if tc.expPass { - suite.Require().NoError(err) - - if ordered { - for i := uint64(1); i < nextSeqRecv; i++ { - pc := suite.chainA.App.IBCKeeper.ChannelKeeper.GetPacketCommitment(suite.chainA.GetContext(), packet.GetSourcePort(), packet.GetSourceChannel(), i) - suite.Require().Nil(pc) - } - } else { - pc := suite.chainA.App.IBCKeeper.ChannelKeeper.GetPacketCommitment(suite.chainA.GetContext(), packet.GetSourceChannel(), packet.GetSourceChannel(), packet.GetSequence()) - suite.Require().Nil(pc) - } - } else { - suite.Require().Error(err) - } - }) - } -} diff --git a/x/ibc/04-channel/types/errors.go b/x/ibc/04-channel/types/errors.go index 692b0feade..a944dd0e7b 100644 --- a/x/ibc/04-channel/types/errors.go +++ b/x/ibc/04-channel/types/errors.go @@ -21,4 +21,5 @@ var ( ErrPacketTimeout = sdkerrors.Register(SubModuleName, 14, "packet timeout") ErrTooManyConnectionHops = sdkerrors.Register(SubModuleName, 15, "too many connection hops") ErrAcknowledgementTooLong = sdkerrors.Register(SubModuleName, 16, "acknowledgement too long") + ErrInvalidAcknowledgement = sdkerrors.Register(SubModuleName, 17, "invalid acknowledgement") ) diff --git a/x/ibc/04-channel/types/events.go b/x/ibc/04-channel/types/events.go index fced7d8694..187c9ece54 100644 --- a/x/ibc/04-channel/types/events.go +++ b/x/ibc/04-channel/types/events.go @@ -17,7 +17,6 @@ const ( EventTypeSendPacket = "send_packet" EventTypeRecvPacket = "recv_packet" EventTypeAcknowledgePacket = "acknowledge_packet" - EventTypeCleanupPacket = "cleanup_packet" EventTypeTimeoutPacket = "timeout_packet" AttributeKeyData = "packet_data"