Add a gRPC endpoint for fetching consensus state heights

[seanchen1991]

Summary

The ibc-rs codebase would benefit from a gRPC endpoint that only fetches consensus state heights for a given client.

Problem Definition

At the moment, height information is extracted after querying for all consensus states via the QueryConsensusStatesRequest query, which is suboptimal. It would be nice to be able to query just for consensus state heights so that the extra step of extracting all of the heights can be eliminated.

Proposal

Add a gRPC endpoint (perhaps something like QueryConsensusHeightsRequest) that returns all consensus state heights for a given client.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[adizere]

In terms of priority, this is not urgent, something like priority 7 on a scale from 1 to 10 (10 highest).

Rationale: The lack of this gRPC endpoint does impact the quality and stability of relayer operations, because relayers must rely on an expensive query (QueryConsensusStatesRequest) to fetch relatively simple data (consensus state heights). One consistent complaint of operators is that they must invest in keeping beefy full node machines to be able to support the query pressure that relayers put on their machines. Adding something like QueryConsensusHeightsRequest is a straightforward way to reduce query pressure because we'd avoid QueryConsensusStatesRequest.

[crodriguezvega]

Thanks for the rationale, @adizere. :)

[crodriguezvega]

Would a gRPC with the following request/response messages satisfy the requirements?

message QueryConsensusStateHeightsRequest {
  // client identifier
  string client_id = 1;
  // pagination request
  cosmos.base.query.v1beta1.PageRequest pagination = 2;
}

message QueryConsensusStateHeightsResponse {
  // consensus state heights
  repeated Height consensus_state_heights = 1 [(gogoproto.nullable) = false];
  // pagination response
  cosmos.base.query.v1beta1.PageResponse pagination = 2;
}

The heights in the response should be in decreasing order.

[adizere]

Would a gRPC with the following request/response messages satisfy the requirements?

Yes, thank you!

The heights in the response should be in decreasing order.

This would be great. We usually sort the heights in descending order after we fetch them, so descending order is the semantic we'd like to achieve.

Note that the protobuf type PageRequest has already a reverse option. Our typical use-case is that we either want:

• all consensus heights of a client, or
• the last X (in descending order) consensus heights installed for a client.

For the first case we use PageRequest parametrized with limit: u64::MAX. For the second case, we would expect to use limit: X. In both cases we set reverse: true.

[crodriguezvega]

Thanks for the feedback, @adizere. So if I understand you correctly, the heights should be in descending order if reverse: true? Otherwise, we should return them in increasing order?

[adizere]

So if I understand you correctly, the heights should be in descending order if reverse: true?

Yes.

The new request should probably adhere to the same semantics as the others requests. The default ordering semantics are "ascending order". On the client side (i.e., relayer side), we will probably set reverse: true to pull the heights in descending order (or we will sort the heights in the response entirely on the client side, alternatively).

Otherwise, we should return them in increasing order?
Yep, I think this is the default that clients would expect, so I think it makes sense to retain the same expectations with this new request.

[adizere]

Anca pointed out that the default ordering in gRPC responses works on lexicographic value of Height, not on their numerical value.

Here is an example (trimmed from an actual query). If the chain has states for heights:
830, 821, 766, 222, 104, 76, 30, 25, 20, 10
The gRPC query returns:
10, 104, 20, 222, 25, 30, 76, 766, 8, 821, 830

Shall I open a separate issue for this? Or will this PR have the correct ordering implemented (i.e., adhering to numerical, not lexicographical order)?

[crodriguezvega]

Shall I open a separate issue for this? Or will this PR have the correct ordering implemented (i.e., adhering to numerical, not lexicographical order)?

@catShaark, can you please check this in #1336 and fix it and add a test if needed?

[catShaark]

Shall I open a separate issue for this? Or will this PR have the correct ordering implemented (i.e., adhering to numerical, not lexicographical order)?

@catShaark, can you please check this in #1336 and fix it and add a test if needed?

I can't find any ways to do numerical ordering efficiently as the ConsensusState store's iterator can only iterate in lexicographical order. Should we iterate all and then sort ?

On second thought, I think we should index Height by converting Height.RevisionNumber along with Height.RevisionHeight directly to 16 bytes ( 2 uint64 -> 16 bytes, big-endian ) in instead of turning them into a string ( uint64 -> string + "-" + uint64 -> string ) and then to bytes. Tho this change is state breaking and requires store migration so It might not be okay ??

[catShaark]

@crodriguezvega , I've opened an issue to propose my solution.

[catShaark]

I think we should close #1336 without the numerical ordering and then do separate PR for numerical ordering on both ConsensusStates and ConsensusStateHeights query. What do you think @crodriguezvega ?

[crodriguezvega]

This is now available in v3.1.0, v2.3.0 and v1.5.0.