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

ibc: handle height in gRPC request metadata if specified #4905

Merged
merged 5 commits into from
Dec 4, 2024

Conversation

noot
Copy link
Collaborator

@noot noot commented Oct 25, 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

debug!("height header was missing; assuming a height of 0");
Ok(TheHeight::zero().into_inner())
}
Some(entry) => entry
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use the ibc-types-core-client::Height type and avoid the newtyping. With that said, I think we should just merge this, and then favor using the new messages once those become available: cosmos/ibc-go#7303

@erwanor
Copy link
Member

erwanor commented Dec 3, 2024

Thanks a lot, learned a bunch from going down this rabbit hole. I am doing some testing and will merge EOD or early tomorrow.

Copy link
Member

@erwanor erwanor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙏

@erwanor erwanor merged commit ac7abac into main Dec 4, 2024
14 checks passed
@erwanor erwanor deleted the noot/ibc-height-query branch December 4, 2024 20:26
conorsch added a commit to penumbra-zone/hermes that referenced this pull request Dec 5, 2024
Updates the penumbra deps to commit [0] on main branch, including merge
of [1]. The naive dep update breaks builds, so further changes are
required.

[0] ac7abacc9bb09503d6fd6a396bc0b6850079084e
[1] penumbra-zone/penumbra#4905
conorsch added a commit to penumbra-zone/hermes that referenced this pull request Dec 6, 2024
Bumping the Penumbra dependencies to v0.80.x, specifically on the current main branch of the protocol repo. Ideally we'd tag a v0.80.10, containing fixes from penumbra-zone/penumbra#4905, but we haven't done that just yet.

Notably, the bump in Penumbra protocol deps will involve a schema change for the view database. I haven't tested locally yet, but I expect Hermes when upgraded to throw an error about the view schema mismatch. If and when that happens during interactive testing on the testnet, I'll make sure to document those errors so we can communicate the need to perform maintenance to operators.
@conorsch conorsch mentioned this pull request Dec 9, 2024
6 tasks
conorsch pushed a commit that referenced this pull request Dec 10, 2024
## Describe your changes

This PR:
- instruments IBC gRPC spans on `DEBUG`
- prefer using the `HostInterface` trait to hardcoding height/revision
for gRPC responses
- systematically defer to the query height hint (in the gRPC headers)
for **every API that returns a proof** (extending #4905, cc @noot)

We will be able to rip out all of the header selection logic once
`https://github.com/cosmos/ibc-go/pull/7303` is merged.

## Checklist before requesting a review

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

- [x] 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:

  > RPC
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.

3 participants