add height field to gRPC requests

[noot]

Description

Adds height field to gRPC requests and latest_height bool to override height and query at the latest height.

Let me know if this direction looks good - the other option would be to make height optional, and use latest if it's nil. However I noticed other heights have [(gogoproto.nullable) = false] so I didn't go that route, and it's also less explicit.

closes: #149

Commit Message / Changelog Entry

feat: add height field to gRPC requests

see the guidelines for commit messages. (view raw markdown for examples)

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against the correct branch (see CONTRIBUTING.md).
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards and Go style guide.
• Wrote unit and integration tests.
• Updated relevant documentation (docs/) or specification (x/<module>/spec/).
• Added relevant godoc comments.
• Provide a commit message to be used for the changelog entry in the PR description for review.
• Re-reviewed Files changed in the Github PR explorer.
• Review Codecov Report in the comment section below once CI passes.

[crodriguezvega]

@noot Thanks for working on this and opening the PR to request early feedback!

I have a couple of questions:

• why do you think it's better to have a Height type instead of just a uint64?
• instead of adding a extra boolean field to override the height, would it be possible to use the latest height by default if the height is not set (or is set to the nil value)?

[damiannolan]

If I understand correctly, we use the following in pretty much all query cmds:

flags.AddQueryFlagsToCmd(cmd)

This should add the default query flags from the sdk, one of which is height: https://github.com/cosmos/cosmos-sdk/blob/main/client/flags/flags.go#L110

If you are trying to use a grpc client without the use of the binary CLIs etc, you should be able to provide this header as far as I know:
https://github.com/cosmos/cosmos-sdk/blob/main/types/grpc/headers.go#L5

Have you tried either of these for your use-case?

[noot]

@crodriguezvega I'd somewhat modeled it off of the existing:

message QueryConsensusStateRequest {
  // client identifier
  string client_id = 1;
  // consensus state revision number
  uint64 revision_number = 2;
  // consensus state revision height
  uint64 revision_height = 3;
  // latest_height overrides the height field and queries the latest stored
  // ConsensusState
  bool latest_height = 4;
}

where ibc.core.client.v1.Height contains revision_number and revision_height. Since revision_numbers might be in use I'm not sure just a uint64 height would be sufficient?

instead of adding a extra boolean field to override the height, would it be possible to use the latest height by default if the height is not set (or is set to the nil value)?

yeah I can change it to this, if you'd prefer! should the QueryConsensusStateRequest also be changed for consistency?

[noot]

@damiannolan yep, adding the height to the HTTP header is my current workaround, but as I'm using a Rust IBC implementation, I felt this was a bit strange as it required adding reading height from the HTTP header on the server side. It would be simpler imo to just have the heights in the gRPC requests themselves.

[crodriguezvega]

Hi @noot. Sorry for the late reply. We have discussed this issue together with ibc-rs and hermes teams and the conclusions have been:

• CometBFT-based chains have the ability to answer queries at various heights via ABCI queries.
• Cosmos chains (ie. chains using ibc-go) have the ability to answer queries at various heights via the x-cosmos-block-height header.

We have also reasoned that the situations where querying at an older height might be needed are: 1) during client upgrades (when chain X commits to a change of the chain parameters at a specific height) and 2) when the validator set changes across a gap of X blocks that is too large and thus the relayer needs to perform bisection to find a historical height that allows for the client to be updated.

If we are right, and these are the only two situations where querying at historical heights is needed, then we would only add the height field to the Protobuf messages for QueryClientStateRequest and QueryConsensusStateRequest. And since (as you already pointed out above) QueryConsensusStateRequest already supports historical queries, then we only need to extend the proto message for QueryClientStateRequest.

I would like to ask you then if you have encountered any other situations where querying other type of data at historical heights was needed. Because otherwise, we could just extend the proto message for QueryClientStateRequest and we would be done. :) Looking forward to your answer!

[crodriguezvega]

Copying here some feedback from @Farhad-Shabani:

"I've been working on implementing the verification of existence/non-existence of commitment proofs for Sovereign light clients. I've realized that with the current gRPC request types, we can only return commitment proofs at the latest height of a state machine, However, the state root we are checking the proof against is not the latest one. Even in normal Cosmos chain relaying operations, this state root may also not be the latest one. In the specific case of Sovereign SDK, they emit an aggregated ZK proof for every number of blocks containing the latest state root covered by that proof. For instance, if the rollup is currently at height 12 and we have ZK proof for every 5 heights, we can process IBC packets up to height 10. Consequently, we should be able to query commitment proofs at height 10. So, any gRPC request that includes proof in their response should provide this capability to specify the query height."

So basically we need to add the height field to every request that returns a proof. @noot Are you still interested in working on this PR or you prefer if we take over?

[DimitrisJim]

Are you still interested in working on this PR or you prefer if we take over?

looks to me like we should pick it up? Considering we can't push to this branch, I'll close this and we can assign issue to someone during one of next iterations. Commits here can obviously be cherry-picked if needed and final merge to main can include attribution.

@noot thanks so much again for this initial PR! Feel free to re-open/implement if issue hasn't been assigned.