add height field to gRPC requests

[noot]

Description

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

Following up to my previous PR #5685 - this came up as an issue again when testing IBC with the hermes relayer, as only proofs at the latest height were returned, but this is not the state root we were expecting to compare against, causing verification errors.

Let me know any thoughts/feedback on this approach, if it looks good I can update the rest of the code.

closes: https://github.com/cosmos/ibc-go/issues/149

---

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/).
• [ ] Added relevant godoc comments.
• [ ] Provide a conventional commit message to follow the repository standards.
• [ ] Include a descriptive changelog entry when appropriate. This may be left to the discretion of the PR reviewers. (e.g. chores should be omitted from changelog)
• [ ] Re-reviewed Files changed in the GitHub PR explorer.
• [ ] Review SonarCloud Report in the comment section below once CI passes.

[damiannolan]

Hi @noot, thanks for opening the PR. I have a couple of questions:

• How would these fields intend to be used inside of the ibc-go implementation code?
• Are you using a different implementation of ibc where you are seeing the errors mentioned?

this came up as an issue again when testing IBC with the hermes relayer, as only proofs at the latest height were returned, but this is not the state root we were expecting to compare against, causing verification errors.

I think I mentioned in the previous PR that the intended approach for querying state at historical heights is to the x-cosmos-block-height. It is up to the implementation of the application to consume this correctly, and load a version of the db store at the given height within this header.

[noot]

Hi @damiannolan ,

Are you using a different implementation of ibc where you are seeing the errors mentioned?

yes, I'm using the Rust IBC implementation by penumbra and a fork of the hermes relayer which uses the gRPC API to query for IBC state and respective proofs.

How would these fields intend to be used inside of the ibc-go implementation code?

The IBC implementation would return proofs at the specified heights for those queries. Right now, the server always returns the proofs at the latest height as no height can be specified (see here) which sometimes causes off-by-one errors, as hermes expects a block at some height, but the latest height has already advanced.

the intended approach for querying state at historical heights is to the x-cosmos-block-height

This is the workaround I'm currently doing, however I'm not using cosmos so using a cosmos-specific approach seems off. I think having the heights in the requests would be preferred for non-cosmos IBC implementations, but if putting the height in the gRPC header is the accepted method, that is also ok.

[erwanor]

Unsurprisingly, we ran into this (non-Cosmos SDK). And it was a massive pain point.

[erwanor]

@damiannolan I think it would be much better to not have to stuff the gRPC headers with secret API arguments. Instead, we should change the messages so that they can model a useful RPC protocol. That's how the QueryConsensusStateRequest messages are already laid out:

/// QueryConsensusStateRequest is the request type for the Query/ConsensusState
// RPC method. Besides the consensus state, it includes a proof and the height
// from which the proof was retrieved.
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;
}

It is very useful to be able to obtain commitment proofs for an arbitrary height, and not just the latest one. This is not just a source of bugs, but also unlocks future work. Is there anything we could do to get this reviewed and merged?

[damiannolan]

Thanks for input @erwanor. And apologies that this PR has fallen off the radar for so long. I will try to prioritise getting eyes on this from the team.

Personally, I'm happy to add the additional arguments to requests if it relieves pain points. It will need to be clearly documented that fulfilling the fields has no affect for cosmos-sdk based nodes. The baseapp in cosmos-sdk controls loading the versioned tree state before the req hits the intended rpc handler.

IMHO the best long term approach to standardising the protobuf definitions is moving them outside of ibc-go and into cosmos/ibc.

[erwanor]

Thanks, appreciate it.

IMHO the best long term approach to standardizing the protobuf definitions is moving them outside of ibc-go and into cosmos/ibc.

Yes, agreed. I think this would be better, because we are slightly past an ecosystem zero-to-one moment in terms of having multiple real world production IBC implementation existing. So this will become more important to do as time goes on. Happy to help push for this and support it with review time, or otherwise.

[rnbguy]

For situations like this, at ibc-rs, we maintain the domain types with an optional height field. When we convert ibc-proto types to our domain types -- we just keep it None and follow ibc-go's implementation. (ref)

This year, when ibc-rs team worked on IBC integration for sovereign-sdk, we used these domain types directly to expose the height as an argument.

[DimitrisJim]

note: eureka grpcs would need to add these too.

[erwanor]

@DimitrisJim good idea, where does eureka dev happen?

[DimitrisJim]

@erwanor currently in https://github.com/cosmos/ibc-go/pull/7505. Most grpcs for channel/V2 have been added at this point.

I'll open up an issue to not lose track of this!

[gjermundgaraba]

This has been open for quite some time, and I don't want to have lingering PRs. We currently don't plan to implement this, but I am open to discussing this in the issue if this is critical.

[erwanor]

I don't understand how the reaction to "this is a massive pain point" is to let this PR bitrot not once, but twice, and then close it?

[gjermundgaraba]

@erwanor I apologize for that. This has indeed not been prioritized. There are always many point points, and it was not clear to me that this one did not have reasonable workarounds (also since it has been quite here for a while). Some issues will be closed if they are not currently planned, but they can easily be reopened if the need is big enough. In other words, the philosophy is to rather close old things, because if they are important they will resurface.

But I am happy to discuss re-opening this issue and accepting a contribution on this if we can:

1. Understand better why this is such a paint point, what use cases it enables, and who it affects
2. Find a cohesive design we can apply across the repo