From 0fbb2a6d1d690ddba47a3c6a5d501f4b956079aa Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Fri, 24 Mar 2023 15:06:15 +0200 Subject: [PATCH 1/4] fix: handle ordered packets in UnreceivedPackets query. --- modules/core/04-channel/keeper/grpc_query.go | 51 ++++++++++++++++--- .../core/04-channel/keeper/grpc_query_test.go | 49 ++++++++++++++++++ 2 files changed, 92 insertions(+), 8 deletions(-) diff --git a/modules/core/04-channel/keeper/grpc_query.go b/modules/core/04-channel/keeper/grpc_query.go index ee11b40251e..fc91aa9c696 100644 --- a/modules/core/04-channel/keeper/grpc_query.go +++ b/modules/core/04-channel/keeper/grpc_query.go @@ -398,18 +398,53 @@ func (q Keeper) UnreceivedPackets(c context.Context, req *types.QueryUnreceivedP ctx := sdk.UnwrapSDKContext(c) - unreceivedSequences := []uint64{} + channel, found := q.GetChannel(sdk.UnwrapSDKContext(c), req.PortId, req.ChannelId) + if !found { + return nil, status.Error( + codes.NotFound, + errorsmod.Wrapf(types.ErrChannelNotFound, "port-id: %s, channel-id %s", req.PortId, req.ChannelId).Error(), + ) + } - for i, seq := range req.PacketCommitmentSequences { - if seq == 0 { - return nil, status.Errorf(codes.InvalidArgument, "packet sequence %d cannot be 0", i) + unreceivedSequences := []uint64{} + switch channel.Ordering { + case types.UNORDERED: + for i, seq := range req.PacketCommitmentSequences { + if seq == 0 { + return nil, status.Errorf(codes.InvalidArgument, "packet sequence %d cannot be 0", i) + } + + // if packet receipt exists on the receiving chain, then packet has already been received + if _, found := q.GetPacketReceipt(ctx, req.PortId, req.ChannelId, seq); !found { + unreceivedSequences = append(unreceivedSequences, seq) + } } - - // if packet receipt exists on the receiving chain, then packet has already been received - if _, found := q.GetPacketReceipt(ctx, req.PortId, req.ChannelId, seq); !found { - unreceivedSequences = append(unreceivedSequences, seq) + case types.ORDERED: + nextSequenceRecv, found := q.GetNextSequenceRecv(ctx, req.PortId, req.ChannelId) + if !found { + return nil, status.Error( + codes.InvalidArgument, + errorsmod.Wrapf( + types.ErrSequenceReceiveNotFound, + "destination port: %s, destination channel: %s", req.PortId, req.ChannelId, + ).Error(), + ) } + for i, seq := range req.PacketCommitmentSequences { + if seq == 0 { + return nil, status.Errorf(codes.InvalidArgument, "packet sequence %d cannot be 0", i) + } + + // If the packet sequence is greater than the sequence most recently received, then the packet has not been received. + if seq >= nextSequenceRecv { + unreceivedSequences = append(unreceivedSequences, seq) + } + } + default: + return nil, status.Error( + codes.InvalidArgument, + errorsmod.Wrapf(types.ErrInvalidChannelOrdering, channel.Ordering.String()).Error()) } selfHeight := clienttypes.GetSelfHeight(ctx) diff --git a/modules/core/04-channel/keeper/grpc_query_test.go b/modules/core/04-channel/keeper/grpc_query_test.go index 0082663a243..75c590f528a 100644 --- a/modules/core/04-channel/keeper/grpc_query_test.go +++ b/modules/core/04-channel/keeper/grpc_query_test.go @@ -1158,6 +1158,16 @@ func (suite *KeeperTestSuite) TestQueryUnreceivedPackets() { }, false, }, + { + "channel not found", + func() { + req = &types.QueryUnreceivedPacketsRequest{ + PortId: "test-port-id", + ChannelId: "test-channel-id", + } + }, + false, + }, { "basic success unreceived packet commitments", func() { @@ -1219,6 +1229,45 @@ func (suite *KeeperTestSuite) TestQueryUnreceivedPackets() { }, true, }, + { + "success basic unreceived packet commitments, ordered channel", + func() { + path := ibctesting.NewPath(suite.chainA, suite.chainB) + path.SetChannelOrdered() + suite.coordinator.Setup(path) + + suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetNextSequenceRecv(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, 1) + + expSeq = []uint64{1} + req = &types.QueryUnreceivedPacketsRequest{ + PortId: path.EndpointA.ChannelConfig.PortID, + ChannelId: path.EndpointA.ChannelID, + PacketCommitmentSequences: []uint64{1}, + } + }, + true, + }, + { + "success basic multiple unreceived packet commitments, ordered channel", + func() { + path := ibctesting.NewPath(suite.chainA, suite.chainB) + path.SetChannelOrdered() + suite.coordinator.Setup(path) + + // Excersise scenario from issue 1532. NextSequenceRecv is 5, commitments for 2, 7, 9, 10 + // are present. 2 is received so 7, 9, 10 should be unreceived. + expSeq = []uint64{7, 9, 10} + packetCommitments := []uint64{2, 7, 9, 10} + suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetNextSequenceRecv(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, 5) + + req = &types.QueryUnreceivedPacketsRequest{ + PortId: path.EndpointA.ChannelConfig.PortID, + ChannelId: path.EndpointA.ChannelID, + PacketCommitmentSequences: packetCommitments, + } + }, + true, + }, } for _, tc := range testCases { From c4bb39ee08804a2a6ddb9f3a6d6c19ae0587fb52 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Tue, 28 Mar 2023 10:17:29 +0300 Subject: [PATCH 2/4] address review comments. --- modules/core/04-channel/keeper/grpc_query.go | 4 +-- .../core/04-channel/keeper/grpc_query_test.go | 26 ++++++++++++++----- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/modules/core/04-channel/keeper/grpc_query.go b/modules/core/04-channel/keeper/grpc_query.go index fc91aa9c696..85f1670fb3d 100644 --- a/modules/core/04-channel/keeper/grpc_query.go +++ b/modules/core/04-channel/keeper/grpc_query.go @@ -423,7 +423,7 @@ func (q Keeper) UnreceivedPackets(c context.Context, req *types.QueryUnreceivedP nextSequenceRecv, found := q.GetNextSequenceRecv(ctx, req.PortId, req.ChannelId) if !found { return nil, status.Error( - codes.InvalidArgument, + codes.NotFound, errorsmod.Wrapf( types.ErrSequenceReceiveNotFound, "destination port: %s, destination channel: %s", req.PortId, req.ChannelId, @@ -444,7 +444,7 @@ func (q Keeper) UnreceivedPackets(c context.Context, req *types.QueryUnreceivedP default: return nil, status.Error( codes.InvalidArgument, - errorsmod.Wrapf(types.ErrInvalidChannelOrdering, channel.Ordering.String()).Error()) + errorsmod.Wrap(types.ErrInvalidChannelOrdering, channel.Ordering.String()).Error()) } selfHeight := clienttypes.GetSelfHeight(ctx) diff --git a/modules/core/04-channel/keeper/grpc_query_test.go b/modules/core/04-channel/keeper/grpc_query_test.go index 75c590f528a..50806646895 100644 --- a/modules/core/04-channel/keeper/grpc_query_test.go +++ b/modules/core/04-channel/keeper/grpc_query_test.go @@ -1158,12 +1158,27 @@ func (suite *KeeperTestSuite) TestQueryUnreceivedPackets() { }, false, }, + { + "invalid seq, ordered channel", + func() { + path := ibctesting.NewPath(suite.chainA, suite.chainB) + path.SetChannelOrdered() + suite.coordinator.Setup(path) + + req = &types.QueryUnreceivedPacketsRequest{ + PortId: path.EndpointA.ChannelConfig.PortID, + ChannelId: path.EndpointA.ChannelID, + PacketCommitmentSequences: []uint64{0}, + } + }, + false, + }, { "channel not found", func() { req = &types.QueryUnreceivedPacketsRequest{ - PortId: "test-port-id", - ChannelId: "test-channel-id", + PortId: "invalid-port-id", + ChannelId: "invalid-channel-id", } }, false, @@ -1230,14 +1245,13 @@ func (suite *KeeperTestSuite) TestQueryUnreceivedPackets() { true, }, { - "success basic unreceived packet commitments, ordered channel", + "basic success unreceived packet commitments, ordered channel", func() { path := ibctesting.NewPath(suite.chainA, suite.chainB) path.SetChannelOrdered() suite.coordinator.Setup(path) - suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetNextSequenceRecv(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, 1) - + // Note: NextSequenceRecv is set to 1 on channel creation. expSeq = []uint64{1} req = &types.QueryUnreceivedPacketsRequest{ PortId: path.EndpointA.ChannelConfig.PortID, @@ -1248,7 +1262,7 @@ func (suite *KeeperTestSuite) TestQueryUnreceivedPackets() { true, }, { - "success basic multiple unreceived packet commitments, ordered channel", + "basic success multiple unreceived packet commitments, ordered channel", func() { path := ibctesting.NewPath(suite.chainA, suite.chainB) path.SetChannelOrdered() From 92c9d76dc1ae7e116614279c7ceaf28d9eaf6725 Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Thu, 30 Mar 2023 14:35:26 +0300 Subject: [PATCH 3/4] address review comments. --- modules/core/04-channel/keeper/grpc_query.go | 10 +++-- .../core/04-channel/keeper/grpc_query_test.go | 37 +++++++++++++++++-- 2 files changed, 40 insertions(+), 7 deletions(-) diff --git a/modules/core/04-channel/keeper/grpc_query.go b/modules/core/04-channel/keeper/grpc_query.go index 773689cd1e0..a7c7f4a0cdb 100644 --- a/modules/core/04-channel/keeper/grpc_query.go +++ b/modules/core/04-channel/keeper/grpc_query.go @@ -406,15 +406,16 @@ func (q Keeper) UnreceivedPackets(c context.Context, req *types.QueryUnreceivedP ) } - unreceivedSequences := []uint64{} + var unreceivedSequences []uint64 switch channel.Ordering { case types.UNORDERED: for i, seq := range req.PacketCommitmentSequences { + // filter for invalid sequences to ensure they are not included in the response value. if seq == 0 { return nil, status.Errorf(codes.InvalidArgument, "packet sequence %d cannot be 0", i) } - // if packet receipt exists on the receiving chain, then packet has already been received + // if the packet receipt does not exist, then it is unreceived if _, found := q.GetPacketReceipt(ctx, req.PortId, req.ChannelId, seq); !found { unreceivedSequences = append(unreceivedSequences, seq) } @@ -432,11 +433,12 @@ func (q Keeper) UnreceivedPackets(c context.Context, req *types.QueryUnreceivedP } for i, seq := range req.PacketCommitmentSequences { + // filter for invalid sequences to ensure they are not included in the response value. if seq == 0 { return nil, status.Errorf(codes.InvalidArgument, "packet sequence %d cannot be 0", i) } - // If the packet sequence is greater than the sequence most recently received, then the packet has not been received. + // Any sequence greater than or equal to the next sequence to be received is not received. if seq >= nextSequenceRecv { unreceivedSequences = append(unreceivedSequences, seq) } @@ -444,7 +446,7 @@ func (q Keeper) UnreceivedPackets(c context.Context, req *types.QueryUnreceivedP default: return nil, status.Error( codes.InvalidArgument, - errorsmod.Wrap(types.ErrInvalidChannelOrdering, channel.Ordering.String()).Error()) + errorsmod.Wrapf(types.ErrInvalidChannelOrdering, "channel order %s is not supported", channel.Ordering.String()).Error()) } selfHeight := clienttypes.GetSelfHeight(ctx) diff --git a/modules/core/04-channel/keeper/grpc_query_test.go b/modules/core/04-channel/keeper/grpc_query_test.go index 60e7012ef97..8be4870066f 100644 --- a/modules/core/04-channel/keeper/grpc_query_test.go +++ b/modules/core/04-channel/keeper/grpc_query_test.go @@ -1112,7 +1112,7 @@ func (suite *KeeperTestSuite) TestQueryPacketAcknowledgements() { func (suite *KeeperTestSuite) TestQueryUnreceivedPackets() { var ( req *types.QueryUnreceivedPacketsRequest - expSeq = []uint64{} + expSeq = []uint64(nil) ) testCases := []struct { @@ -1183,6 +1183,21 @@ func (suite *KeeperTestSuite) TestQueryUnreceivedPackets() { }, false, }, + { + "basic success empty packet commitments", + func() { + path := ibctesting.NewPath(suite.chainA, suite.chainB) + suite.coordinator.Setup(path) + + expSeq = []uint64(nil) + req = &types.QueryUnreceivedPacketsRequest{ + PortId: path.EndpointA.ChannelConfig.PortID, + ChannelId: path.EndpointA.ChannelID, + PacketCommitmentSequences: []uint64{}, + } + }, + true, + }, { "basic success unreceived packet commitments", func() { @@ -1208,7 +1223,7 @@ func (suite *KeeperTestSuite) TestQueryUnreceivedPackets() { suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetPacketReceipt(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, 1) - expSeq = []uint64{} + expSeq = []uint64(nil) req = &types.QueryUnreceivedPacketsRequest{ PortId: path.EndpointA.ChannelConfig.PortID, ChannelId: path.EndpointA.ChannelID, @@ -1222,7 +1237,7 @@ func (suite *KeeperTestSuite) TestQueryUnreceivedPackets() { func() { path := ibctesting.NewPath(suite.chainA, suite.chainB) suite.coordinator.Setup(path) - expSeq = []uint64{} // reset + expSeq = []uint64(nil) // reset packetCommitments := []uint64{} // set packet receipt for every other sequence @@ -1244,6 +1259,22 @@ func (suite *KeeperTestSuite) TestQueryUnreceivedPackets() { }, true, }, + { + "basic success empty packet commitments, ordered channel", + func() { + path := ibctesting.NewPath(suite.chainA, suite.chainB) + path.SetChannelOrdered() + suite.coordinator.Setup(path) + + expSeq = []uint64(nil) + req = &types.QueryUnreceivedPacketsRequest{ + PortId: path.EndpointA.ChannelConfig.PortID, + ChannelId: path.EndpointA.ChannelID, + PacketCommitmentSequences: []uint64{}, + } + }, + true, + }, { "basic success unreceived packet commitments, ordered channel", func() { From 5ef8854bd68e262215f03285c24d1135ad610eaa Mon Sep 17 00:00:00 2001 From: DimitrisJim Date: Thu, 30 Mar 2023 14:36:34 +0300 Subject: [PATCH 4/4] comment for the issue. --- modules/core/04-channel/keeper/grpc_query_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/modules/core/04-channel/keeper/grpc_query_test.go b/modules/core/04-channel/keeper/grpc_query_test.go index 8be4870066f..4a0b60809f6 100644 --- a/modules/core/04-channel/keeper/grpc_query_test.go +++ b/modules/core/04-channel/keeper/grpc_query_test.go @@ -1299,8 +1299,8 @@ func (suite *KeeperTestSuite) TestQueryUnreceivedPackets() { path.SetChannelOrdered() suite.coordinator.Setup(path) - // Excersise scenario from issue 1532. NextSequenceRecv is 5, commitments for 2, 7, 9, 10 - // are present. 2 is received so 7, 9, 10 should be unreceived. + // Exercise scenario from issue #1532. NextSequenceRecv is 5, packet commitments provided are 2, 7, 9, 10. + // Packet sequence 2 is already received so only sequences 7, 9, 10 should be considered unreceived. expSeq = []uint64{7, 9, 10} packetCommitments := []uint64{2, 7, 9, 10} suite.chainA.App.GetIBCKeeper().ChannelKeeper.SetNextSequenceRecv(suite.chainA.GetContext(), path.EndpointA.ChannelConfig.PortID, path.EndpointA.ChannelID, 5)