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: compute IBC heights from revision number derived by chain id #3100

Merged
merged 2 commits into from
Sep 28, 2023

Conversation

avahowell
Copy link
Contributor

@avahowell avahowell commented Sep 26, 2023

Previously, we hardcoded 0 when constructing IBCHeights (which consist of a revision number, and revision height) to verify that counterparties committed correct penumbra consensus states. This PR instead computes the revision number from the chain ID.

There is an underlying issue here, which is that ibc-go enforces a relationship between the chain ID and the revision number. The revision number is always parsed from the chain ID, matching with a regex that matches chain ids in the form .*[^\n-]-{1}[1-9][0-9]. If the matching fails, the revision number is assumed to be zero. This is why, despite our chain ID not fitting this regex, our handshakes worked.

In the future, we can either

  • keep non-ibcgo-standard chain IDs, and commit to app_version (revision number) always being zero

or

  • amend chain IDs to fit this format, eg penumbra-testnet-dione-1

@avahowell avahowell changed the title ibc: use APP_VERSION when construction IBC heights ibc: compute IBC heights from revision number derived by chain id Sep 26, 2023
@avahowell avahowell force-pushed the ibc-height-comparison branch from 3c38fda to cc10955 Compare September 26, 2023 02:06
@avahowell avahowell temporarily deployed to smoke-test September 26, 2023 02:07 — with GitHub Actions Inactive
@avahowell avahowell force-pushed the ibc-height-comparison branch from cc10955 to 8730380 Compare September 26, 2023 23:27
@avahowell avahowell temporarily deployed to smoke-test September 26, 2023 23:27 — with GitHub Actions Inactive
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.

I think we can drop the revision number suffix, and default to zero. My understanding is that its purpose is to disambiguate block numbers for chains that do height-reseting upgrades.

@avahowell avahowell merged commit b1ce102 into main Sep 28, 2023
@avahowell avahowell deleted the ibc-height-comparison branch September 28, 2023 19:14
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.

2 participants