Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add height field to gRPC requests #7303

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

add height field to gRPC requests #7303

wants to merge 4 commits into from

Conversation

noot
Copy link

@noot noot commented Sep 17, 2024

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: #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
Copy link
Member

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
Copy link
Author

noot commented Oct 1, 2024

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
Copy link

erwanor commented Dec 2, 2024

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

@erwanor
Copy link

erwanor commented Dec 3, 2024

@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
Copy link
Member

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
Copy link

erwanor commented Dec 3, 2024

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
Copy link

rnbguy commented Dec 3, 2024

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
Copy link
Contributor

note: eureka grpcs would need to add these too.

erwanor pushed a commit to penumbra-zone/penumbra that referenced this pull request Dec 4, 2024
## Describe your changes

Check if height is specified in the metadata for relevant IBC gRPC
queries and get the respective snapshot if it is provided. If the height
is not specified in the metadata, the latest snapshot is used, which is
the existing behaviour.

Reasoning: since astria uses only gRPC queries for all the IBC queries
in the hermes impl, but the gRPC proto messages don't contain the query
height, we implemented a fix by putting the height in the metadata.
Without this fix, our hermes impl would submit unexpected proofs on
occasion, as the proofs would be off-by-one height than that was
expected.

I opened a PR to update the protos, but the response by the maintainer
of ibc-go suggests to me that they intend for having the height in the
header be the actual fix (cosmos/ibc-go#7303).

## Issue ticket number and link

n/a

## Checklist before requesting a review

- [ ] I have added guiding text to explain how a reviewer should test
these changes.

- [ ] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label. Otherwise, I declare my belief that there
are not consensus-breaking changes, for the following reason:

> only affects the IBC gRPC server, and does not affect existing
behaviour

---------

Co-authored-by: Richard Janis Goldschmidt <[email protected]>
@erwanor
Copy link

erwanor commented Dec 5, 2024

@DimitrisJim good idea, where does eureka dev happen?

@DimitrisJim
Copy link
Contributor

DimitrisJim commented Dec 6, 2024

@erwanor currently in #7505. Most grpcs for channel/V2 have been added at this point.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add heights to every gRPC request
5 participants