Fix query unrelayed packets logic (#6733)

* add some query functions to channel keeper

* update grpc queries and cli cmd and tests

* rerun proto gen after merging master

* fix build broken from merge conflicts

* add test and update proto

* Update x/ibc/04-channel/client/cli/query.go

Co-authored-by: Federico Kunze <31522760+fedekunze@users.noreply.github.com>

proto/ibc/channel/query.proto

@@ -24,6 +24,9 @@ service Query {
   // PacketCommitments returns the all the packet commitments hashes associated with a channel.
   rpc PacketCommitments(QueryPacketCommitmentsRequest) returns (QueryPacketCommitmentsResponse) {}
 
+  // PacketAcknowledgement queries a stored packet acknowledgement hash.
+  rpc PacketAcknowledgement(QueryPacketAcknowledgementRequest) returns (QueryPacketAcknowledgementResponse) {}
+
   // UnrelayedPackets returns all the unrelayed IBC packets associated with a channel and sequences.
   rpc UnrelayedPackets(QueryUnrelayedPacketsRequest) returns (QueryUnrelayedPacketsResponse) {}
 
@@ -133,26 +136,47 @@ message QueryPacketCommitmentsResponse {
   int64 height = 3;
 }
 
-// QueryUnrelayedPacketsRequest is the request type for the Query/QueryConnectionChannels RPC method
+// QueryPacketAcknowledgementRequest is the request type for the Query/PacketAcknowledgement RPC method
+message QueryPacketAcknowledgementRequest {
+  // port unique identifier
+  string port_id = 1 [(gogoproto.customname) = "PortID"];
+  // channel unique identifier
+  string channel_id = 2 [(gogoproto.customname) = "ChannelID"];
+  // packet sequence
+  uint64 sequence = 3;
+}
+
+// QueryPacketAcknowledgementResponse defines the client query response for a packet which also
+// includes a proof, its path and the height form which the proof was retrieved
+message QueryPacketAcknowledgementResponse {
+  // packet associated with the request fields
+  bytes acknowledgement = 1;
+  // merkle proof of existence
+  bytes proof = 2;
+  // merkle proof path
+  string proof_path = 3;
+  // height at which the proof was retrieved
+  uint64 proof_height = 4;
+}
+
+// QueryUnrelayedPacketsRequest is the request type for the Query/UnrelayedPackets RPC method
 message QueryUnrelayedPacketsRequest {
   // port unique identifier
   string port_id = 1 [(gogoproto.customname) = "PortID"];
   // channel unique identifier
   string channel_id = 2 [(gogoproto.customname) = "ChannelID"];
   // list of packet sequences
-  repeated uint64 sequences = 3;
+  repeated uint64 packet_commitment_sequences = 3 [(gogoproto.customname) = "PacketCommitmentSequences"];
-  // pagination request
+  // flag indicating if the return value is packet commitments or acknowledgements
-  cosmos.query.PageRequest req = 4;
+  bool acknowledgements = 4;
 }
 
-// QueryUnrelayedPacketsResponse is the request type for the Query/QueryConnectionChannels RPC method
+// QueryUnrelayedPacketsResponse is the request type for the Query/UnrelayedPacketCommitments RPC method
 message QueryUnrelayedPacketsResponse {
-  // list of unrelayed packets sequences
+  // list of unrelayed packet sequences
-  repeated uint64 packets = 1;
+  repeated uint64 sequences = 1;
-  // pagination response
-  cosmos.query.PageResponse res = 2;
   // query block height
-  int64 height = 3;
+  int64 height = 2;
 }
 
 // QueryNextSequenceReceiveRequest is the request type for the Query/QueryNextSequenceReceiveRequest RPC method

x/ibc/02-client/types/querier.go

@@ -13,8 +13,6 @@ import (
 // query routes supported by the IBC client Querier
 const (
 	QueryAllClients = "client_states"
-	QueryClientState    = "client_state"
-	QueryConsensusState = "consensus_state"
 )
 
 // QueryAllClientsParams defines the parameters necessary for querying for all

x/ibc/03-connection/types/querier.go

@@ -7,13 +7,6 @@ import (
 	host "github.com/cosmos/cosmos-sdk/x/ibc/24-host"
 )
 
-// query routes supported by the IBC connection Querier
-const (
-	QueryAllConnections       = "connections"
-	QueryClientConnections    = "client_connections"
-	QueryAllClientConnections = "all_client_connections"
-)
-
 // NewQueryConnectionResponse creates a new QueryConnectionResponse instance
 func NewQueryConnectionResponse(
 	connection ConnectionEnd, proof []byte, height int64,

x/ibc/04-channel/client/cli/query.go

@@ -16,7 +16,10 @@ import (
 	host "github.com/cosmos/cosmos-sdk/x/ibc/24-host"
 )
 
-const flagSequences = "sequences"
+const (
+	flagSequences        = "sequences"
+	flagAcknowledgements = "acknowledgements"
+)
 
 // GetCmdQueryChannels defines the command to query all the channels ends
 // that this chain mantains.
@@ -265,18 +268,21 @@ func GetCmdQueryPacketCommitment() *cobra.Command {
 	return cmd
 }
 
-// GetCmdQueryUnrelayedPackets defines the command to query all the unrelayed packets.
+// GetCmdQueryUnrelayedPackets defines the command to query all the unrelayed
+// packets for either packet commitments or acknowledgements.
 func GetCmdQueryUnrelayedPackets() *cobra.Command {
 	cmd := &cobra.Command{
 		Use:   "unrelayed-packets [port-id] [channel-id]",
 		Short: "Query all the unrelayed packets associated with a channel",
-		Long: `It indicates if a packet, given a list of packet commitment sequences, is unrelayed.
+		Long: `Determine if a packet, given a list of packet commitment sequences, is unrelayed.
-An unrelayed packet corresponds to:
 
-- Unrelayed packet commitments: when no acknowledgement exists for the given sequence.
+If the '-acknowledgements' flag is false (default) then the return value represents:
-- Unrelayed packet acknowledgements: when an acknowledgement exists and a packet commitment also exists.`,
+- Unrelayed packet commitments: no acknowledgement exists for the given packet commitment sequence.
-		Example: fmt.Sprintf("%s query %s %s unrelayed-packets [port-id] [channel-id] --sequences=1,2,3", version.AppName, host.ModuleName, types.SubModuleName),
+
-		Args:    cobra.ExactArgs(2),
+Otherwise, the return value represents:
+- Unrelayed packet acknowledgements: an acknowledgement exists for the given packet commitment sequence.`,
+		Example: fmt.Sprintf("%s query %s %s unrelayed-packets [port-id] [channel-id] --sequences=1,2,3 --acknowledgements=false", version.AppName, host.ModuleName, types.SubModuleName),
+		Args:    cobra.ExactArgs(3),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			clientCtx := client.GetClientContextFromCmd(cmd)
 			clientCtx, err := client.ReadQueryCommandFlags(clientCtx, cmd.Flags())
@@ -290,22 +296,21 @@ An unrelayed packet corresponds to:
 				return err
 			}
 
+			acknowledgements, err := cmd.Flags().GetBool(flagAcknowledgements)
+			if err != nil {
+				return err
+			}
+
 			seqs := make([]uint64, len(seqSlice))
 			for i := range seqSlice {
 				seqs[i] = uint64(seqSlice[i])
 			}
 
-			offset, _ := cmd.Flags().GetInt(flags.FlagPage)
-			limit, _ := cmd.Flags().GetInt(flags.FlagLimit)
-
 			req := &types.QueryUnrelayedPacketsRequest{
 				PortID:                    args[0],
 				ChannelID:                 args[1],
-				Sequences: seqs,
+				PacketCommitmentSequences: seqs,
-				Req: &query.PageRequest{
+				Acknowledgements:          acknowledgements,
-					Offset: uint64(offset),
-					Limit:  uint64(limit),
-				},
 			}
 
 			res, err := queryClient.UnrelayedPackets(context.Background(), req)
@@ -313,14 +318,12 @@ An unrelayed packet corresponds to:
 				return err
 			}
 
-			clientCtx = clientCtx.WithHeight(res.Height)
 			return clientCtx.PrintOutput(res)
 		},
 	}
 
 	cmd.Flags().Int64Slice(flagSequences, []int64{}, "comma separated list of packet sequence numbers")
-	cmd.Flags().Int(flags.FlagPage, 1, "pagination page of light clients to to query for")
+	cmd.Flags().Bool(flagAcknowledgements, false, "boolean indicating if unrelayed acknowledgements (true) or unrelayed packet commitments (false) are returned.")
-	cmd.Flags().Int(flags.FlagLimit, 100, "pagination limit of light clients to query for")
 	flags.AddQueryFlagsToCmd(cmd)
 
 	return cmd

x/ibc/04-channel/keeper/grpc_query.go

@@ -189,7 +189,40 @@ func (q Keeper) PacketCommitments(c context.Context, req *types.QueryPacketCommi
 	}, nil
 }
 
-// UnrelayedPackets implements the Query/UnrelayedPackets gRPC method
+// PacketAcknowledgement implements the Query/PacketAcknowledgement gRPC method
+func (q Keeper) PacketAcknowledgement(c context.Context, req *types.QueryPacketAcknowledgementRequest) (*types.QueryPacketAcknowledgementResponse, error) {
+	if req == nil {
+		return nil, status.Error(codes.InvalidArgument, "empty request")
+	}
+
+	if err := validategRPCRequest(req.PortID, req.ChannelID); err != nil {
+		return nil, err
+	}
+
+	if req.Sequence == 0 {
+		return nil, status.Error(codes.InvalidArgument, "packet sequence cannot be 0")
+	}
+
+	ctx := sdk.UnwrapSDKContext(c)
+
+	acknowledgementBz, found := q.GetPacketAcknowledgement(ctx, req.PortID, req.ChannelID, req.Sequence)
+	if !found || len(acknowledgementBz) == 0 {
+		return nil, status.Error(codes.NotFound, "packet acknowledgement hash not found")
+	}
+
+	return types.NewQueryPacketAcknowledgementResponse(req.PortID, req.ChannelID, req.Sequence, acknowledgementBz, nil, ctx.BlockHeight()), nil
+}
+
+// UnrelayedPackets implements the Query/UnrelayedPackets gRPC method. Given
+// a list of counterparty packet commitments, the querier checks if the packet
+// sequence has an acknowledgement stored. If req.Acknowledgements is true then
+// all unrelayed acknowledgements are returned (ack exists), otherwise all
+// unrelayed packet commitments are returned (ack does not exist).
+//
+// NOTE: The querier makes the assumption that the provided list of packet
+// commitments is correct and will not function properly if the list
+// is not up to date. Ideally the query height should equal the latest height
+// on the counterparty's client which represents this chain.
 func (q Keeper) UnrelayedPackets(c context.Context, req *types.QueryUnrelayedPacketsRequest) (*types.QueryUnrelayedPacketsResponse, error) {
 	if req == nil {
 		return nil, status.Error(codes.InvalidArgument, "empty request")
@@ -201,33 +234,22 @@ func (q Keeper) UnrelayedPackets(c context.Context, req *types.QueryUnrelayedPac
 
 	ctx := sdk.UnwrapSDKContext(c)
 
-	var (
+	var unrelayedSequences = []uint64{}
-		unrelayedPackets = []uint64{}
-		store            sdk.KVStore
-		res              *query.PageResponse
-		err              error
-	)
 
-	for i, seq := range req.Sequences {
+	for i, seq := range req.PacketCommitmentSequences {
 		if seq == 0 {
 			return nil, status.Errorf(codes.InvalidArgument, "packet sequence %d cannot be 0", i)
 		}
 
-		store = prefix.NewStore(ctx.KVStore(q.storeKey), host.KeyPacketAcknowledgement(req.PortID, req.ChannelID, seq))
+		// if req.Acknowledgements is true append sequences with an existing acknowledgement
-		res, err = query.Paginate(store, req.Req, func(_, _ []byte) error {
+		// otherwise append sequences without an existing acknowledgement.
-			return nil
+		if _, found := q.GetPacketAcknowledgement(ctx, req.PortID, req.ChannelID, seq); found == req.Acknowledgements {
-		})
+			unrelayedSequences = append(unrelayedSequences, seq)
-
-		if err != nil {
-			// ignore error and continue to the next sequence item
-			continue
 		}
 
-		unrelayedPackets = append(unrelayedPackets, seq)
 	}
 	return &types.QueryUnrelayedPacketsResponse{
-		Packets: unrelayedPackets,
+		Sequences: unrelayedSequences,
-		Res:     res,
 		Height:    ctx.BlockHeight(),
 	}, nil
 }

x/ibc/04-channel/keeper/grpc_query_test.go

@@ -481,6 +481,103 @@ func (suite *KeeperTestSuite) TestQueryPacketCommitments() {
 	}
 }
 
+func (suite *KeeperTestSuite) TestQueryPacketAcknowledgement() {
+	var (
+		req    *types.QueryPacketAcknowledgementRequest
+		expAck []byte
+	)
+
+	testCases := []struct {
+		msg      string
+		malleate func()
+		expPass  bool
+	}{
+		{
+			"empty request",
+			func() {
+				req = nil
+			},
+			false,
+		},
+		{
+			"invalid port ID",
+			func() {
+				req = &types.QueryPacketAcknowledgementRequest{
+					PortID:    "",
+					ChannelID: "test-channel-id",
+					Sequence:  0,
+				}
+			},
+			false,
+		},
+		{
+			"invalid channel ID",
+			func() {
+				req = &types.QueryPacketAcknowledgementRequest{
+					PortID:    "test-port-id",
+					ChannelID: "",
+					Sequence:  0,
+				}
+			},
+			false,
+		},
+		{"invalid sequence",
+			func() {
+				req = &types.QueryPacketAcknowledgementRequest{
+					PortID:    "test-port-id",
+					ChannelID: "test-channel-id",
+					Sequence:  0,
+				}
+			},
+			false,
+		},
+		{"channel not found",
+			func() {
+				req = &types.QueryPacketAcknowledgementRequest{
+					PortID:    "test-port-id",
+					ChannelID: "test-channel-id",
+					Sequence:  1,
+				}
+			},
+			false,
+		},
+		{
+			"success",
+			func() {
+				_, _, _, _, channelA, _ := suite.coordinator.Setup(suite.chainA, suite.chainB)
+				expAck = []byte("hash")
+				suite.chainA.App.IBCKeeper.ChannelKeeper.SetPacketAcknowledgement(suite.chainA.GetContext(), channelA.PortID, channelA.ID, 1, expAck)
+
+				req = &types.QueryPacketAcknowledgementRequest{
+					PortID:    channelA.PortID,
+					ChannelID: channelA.ID,
+					Sequence:  1,
+				}
+			},
+			true,
+		},
+	}
+
+	for _, tc := range testCases {
+		suite.Run(fmt.Sprintf("Case %s", tc.msg), func() {
+			suite.SetupTest() // reset
+
+			tc.malleate()
+			ctx := sdk.WrapSDKContext(suite.chainA.GetContext())
+
+			res, err := suite.chainA.QueryServer.PacketAcknowledgement(ctx, req)
+
+			if tc.expPass {
+				suite.Require().NoError(err)
+				suite.Require().NotNil(res)
+				suite.Require().Equal(expAck, res.Acknowledgement)
+			} else {
+				suite.Require().Error(err)
+			}
+		})
+	}
+}
+
 func (suite *KeeperTestSuite) TestQueryUnrelayedPackets() {
 	var (
 		req    *types.QueryUnrelayedPacketsRequest
@@ -525,15 +622,53 @@ func (suite *KeeperTestSuite) TestQueryUnrelayedPackets() {
 				req = &types.QueryUnrelayedPacketsRequest{
 					PortID:                    "test-port-id",
 					ChannelID:                 "test-channel-id",
-					Sequences: []uint64{0},
+					PacketCommitmentSequences: []uint64{0},
 				}
 			},
 			false,
 		},
 		{
-			"success",
+			"basic success unrelayed packet commitments",
 			func() {
 				_, _, _, _, channelA, _ := suite.coordinator.Setup(suite.chainA, suite.chainB)
+
+				// no ack exists
+
+				expSeq = []uint64{1}
+				req = &types.QueryUnrelayedPacketsRequest{
+					PortID:                    channelA.PortID,
+					ChannelID:                 channelA.ID,
+					PacketCommitmentSequences: []uint64{1},
+					Acknowledgements:          false,
+				}
+			},
+			true,
+		},
+		{
+			"basic success unrelayed packet commitments, nothing to relay",
+			func() {
+				_, _, _, _, channelA, _ := suite.coordinator.Setup(suite.chainA, suite.chainB)
+
+				// ack exists
+				ack := types.NewPacketAckCommitment(channelA.PortID, channelA.ID, 1, []byte("hash"))
+				suite.chainA.App.IBCKeeper.ChannelKeeper.SetPacketAcknowledgement(suite.chainA.GetContext(), channelA.PortID, channelA.ID, 1, ack.Hash)
+
+				expSeq = []uint64{}
+				req = &types.QueryUnrelayedPacketsRequest{
+					PortID:                    channelA.PortID,
+					ChannelID:                 channelA.ID,
+					PacketCommitmentSequences: []uint64{1},
+					Acknowledgements:          false,
+				}
+			},
+			true,
+		},
+		{
+			"basic success unrelayed acknowledgements",
+			func() {
+				_, _, _, _, channelA, _ := suite.coordinator.Setup(suite.chainA, suite.chainB)
+
+				// ack exists
 				ack := types.NewPacketAckCommitment(channelA.PortID, channelA.ID, 1, []byte("hash"))
 				suite.chainA.App.IBCKeeper.ChannelKeeper.SetPacketAcknowledgement(suite.chainA.GetContext(), channelA.PortID, channelA.ID, 1, ack.Hash)
 
@@ -541,12 +676,80 @@ func (suite *KeeperTestSuite) TestQueryUnrelayedPackets() {
 				req = &types.QueryUnrelayedPacketsRequest{
 					PortID:                    channelA.PortID,
 					ChannelID:                 channelA.ID,
-					Sequences: []uint64{1},
+					PacketCommitmentSequences: []uint64{1},
-					Req: &query.PageRequest{
+					Acknowledgements:          true,
-						Key:        nil,
+				}
-						Limit:      1,
-						CountTotal: false,
 			},
+			true,
+		},
+		{
+			"basic success unrelayed acknowledgements, nothing to relay",
+			func() {
+				_, _, _, _, channelA, _ := suite.coordinator.Setup(suite.chainA, suite.chainB)
+
+				// no ack exists
+
+				expSeq = []uint64{}
+				req = &types.QueryUnrelayedPacketsRequest{
+					PortID:                    channelA.PortID,
+					ChannelID:                 channelA.ID,
+					PacketCommitmentSequences: []uint64{1},
+					Acknowledgements:          true,
+				}
+			},
+			true,
+		},
+		{
+			"success multiple unrelayed packet commitments",
+			func() {
+				_, _, _, _, channelA, _ := suite.coordinator.Setup(suite.chainA, suite.chainB)
+				expSeq = []uint64{} // reset
+				packetCommitments := []uint64{}
+
+				// set ack for every other sequence
+				for seq := uint64(1); seq < 10; seq++ {
+					packetCommitments = append(packetCommitments, seq)
+
+					if seq%2 == 0 {
+						ack := types.NewPacketAckCommitment(channelA.PortID, channelA.ID, seq, []byte("hash"))
+						suite.chainA.App.IBCKeeper.ChannelKeeper.SetPacketAcknowledgement(suite.chainA.GetContext(), channelA.PortID, channelA.ID, seq, ack.Hash)
+					} else {
+						expSeq = append(expSeq, seq)
+					}
+				}
+
+				req = &types.QueryUnrelayedPacketsRequest{
+					PortID:                    channelA.PortID,
+					ChannelID:                 channelA.ID,
+					PacketCommitmentSequences: packetCommitments,
+					Acknowledgements:          false,
+				}
+			},
+			true,
+		},
+		{
+			"success multiple unrelayed acknowledgements",
+			func() {
+				_, _, _, _, channelA, _ := suite.coordinator.Setup(suite.chainA, suite.chainB)
+				expSeq = []uint64{} // reset
+				packetCommitments := []uint64{}
+
+				// set ack for every other sequence
+				for seq := uint64(1); seq < 10; seq++ {
+					packetCommitments = append(packetCommitments, seq)
+
+					if seq%2 == 0 {
+						ack := types.NewPacketAckCommitment(channelA.PortID, channelA.ID, seq, []byte("hash"))
+						suite.chainA.App.IBCKeeper.ChannelKeeper.SetPacketAcknowledgement(suite.chainA.GetContext(), channelA.PortID, channelA.ID, seq, ack.Hash)
+						expSeq = append(expSeq, seq)
+					}
+				}
+
+				req = &types.QueryUnrelayedPacketsRequest{
+					PortID:                    channelA.PortID,
+					ChannelID:                 channelA.ID,
+					PacketCommitmentSequences: packetCommitments,
+					Acknowledgements:          true,
 				}
 			},
 			true,
@@ -565,7 +768,7 @@ func (suite *KeeperTestSuite) TestQueryUnrelayedPackets() {
 			if tc.expPass {
 				suite.Require().NoError(err)
 				suite.Require().NotNil(res)
-				suite.Require().Equal(expSeq, res.Packets)
+				suite.Require().Equal(expSeq, res.Sequences)
 			} else {
 				suite.Require().Error(err)
 			}

x/ibc/04-channel/types/querier.go

@@ -9,14 +9,8 @@ import (
 
 // query routes supported by the IBC channel Querier
 const (
-	QueryAllChannels               = "channels"
-	QueryChannel                   = "channel"
-	QueryConnectionChannels        = "connection-channels"
 	QueryChannelClientState    = "channel-client-state"
 	QueryChannelConsensusState = "channel-consensus-state"
-	QueryPacketCommitments         = "packet-commitments"
-	QueryUnrelayedAcknowledgements = "unrelayed-acknowledgements"
-	QueryUnrelayedPacketSends      = "unrelayed-packet-sends"
 )
 
 // NewQueryChannelResponse creates a new QueryChannelResponse instance
@@ -43,6 +37,19 @@ func NewQueryPacketCommitmentResponse(
 	}
 }
 
+// NewQueryPacketAcknowledgementResponse creates a new QueryPacketAcknowledgementResponse instance
+func NewQueryPacketAcknowledgementResponse(
+	portID, channelID string, sequence uint64, acknowledgement []byte, proof []byte, height int64,
+) *QueryPacketAcknowledgementResponse {
+	path := commitmenttypes.NewMerklePath(strings.Split(host.PacketAcknowledgementPath(portID, channelID, sequence), "/"))
+	return &QueryPacketAcknowledgementResponse{
+		Acknowledgement: acknowledgement,
+		Proof:           proof,
+		ProofPath:       path.Pretty(),
+		ProofHeight:     uint64(height),
+	}
+}
+
 // NewQueryNextSequenceReceiveResponse creates a new QueryNextSequenceReceiveResponse instance
 func NewQueryNextSequenceReceiveResponse(
 	portID, channelID string, sequence uint64, proof []byte, height int64,

x/ibc/keeper/grpc_query.go

@@ -47,6 +47,11 @@ func (q Keeper) PacketCommitments(c context.Context, req *channeltypes.QueryPack
 	return q.ChannelKeeper.PacketCommitments(c, req)
 }
 
+// PacketAcknowledgement implements the IBC QueryServer interface
+func (q Keeper) PacketAcknowledgement(c context.Context, req *channeltypes.QueryPacketAcknowledgementRequest) (*channeltypes.QueryPacketAcknowledgementResponse, error) {
+	return q.ChannelKeeper.PacketAcknowledgement(c, req)
+}
+
 // UnrelayedPackets implements the IBC QueryServer interface
 func (q Keeper) UnrelayedPackets(c context.Context, req *channeltypes.QueryUnrelayedPacketsRequest) (*channeltypes.QueryUnrelayedPacketsResponse, error) {
 	return q.ChannelKeeper.UnrelayedPackets(c, req)