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-types: update to v0.14.0 #4682

Closed
conorsch opened this issue Jun 28, 2024 · 3 comments
Closed

ibc-types: update to v0.14.0 #4682

conorsch opened this issue Jun 28, 2024 · 3 comments
Labels
consensus-breaking breaking change to execution of on-chain data needs-refinement unclear, incomplete, or stub issue that needs work

Comments

@conorsch
Copy link
Contributor

As of v0.78.0 (#4582), the monorepo is still depending on ibc-types version 0.12.0: https://github.com/penumbra-zone/penumbra/blob/v0.78.0/Cargo.toml#L156

We've merged some upstream work in ibc-types since, e.g. penumbra-zone/ibc-types#88 & penumbra-zone/ibc-types#84, the latter of which looks like it may be state-breaking. We should make a plan to update the monorepo to the latest version of ibc-types, including writing migrations if necessary.

@github-project-automation github-project-automation bot moved this to Backlog in Penumbra Jun 28, 2024
@github-actions github-actions bot added the needs-refinement unclear, incomplete, or stub issue that needs work label Jun 28, 2024
@conorsch conorsch added the consensus-breaking breaking change to execution of on-chain data label Oct 8, 2024
@erwanor
Copy link
Member

erwanor commented Nov 19, 2024

Thanks for adding this tag @conorsch, it's indeed a (small) consensus breaking change since we amended one of the messages modeling the IBC protocol (following ibc-proto support for IBC v8) in penumbra-zone/ibc-types#84

@erwanor
Copy link
Member

erwanor commented Jan 9, 2025

This was done in #4973

reasoning: #4963 (comment)

conorsch pushed a commit that referenced this issue Jan 10, 2025
Continuation of #4963, into a release branch `v0.82.x` before tagging a
release candidate at that version and publishing the workspace using an
`alpha` version.

This handles the domain type change for upgradeable channels
(penumbra-zone/ibc-types#84) smoothly. It makes
sure to write default values to the new fields, which avoids wire
protocol changes, and makes this PR non consensus/state breaking.

Includes substantial version changes to:

* tendermint-rs
* tonic #4400
* ibc-types #4682
* cnidarium #4956

---------

Co-authored-by: Richard Janis Goldschmidt <[email protected]>
conorsch pushed a commit that referenced this issue Jan 13, 2025
Continuation of #4963, into a release branch `v0.82.x` before tagging a
release candidate at that version and publishing the workspace using an
`alpha` version.

This handles the domain type change for upgradeable channels
(penumbra-zone/ibc-types#84) smoothly. It makes
sure to write default values to the new fields, which avoids wire
protocol changes, and makes this PR non consensus/state breaking.

Includes substantial version changes to:

* tendermint-rs
* tonic #4400
* ibc-types #4682
* cnidarium #4956

---------

Co-authored-by: Richard Janis Goldschmidt <[email protected]>
conorsch pushed a commit that referenced this issue Jan 14, 2025
Continuation of #4963, into a release branch `v0.82.x` before tagging a
release candidate at that version and publishing the workspace using an
`alpha` version.

This handles the domain type change for upgradeable channels
(penumbra-zone/ibc-types#84) smoothly. It makes
sure to write default values to the new fields, which avoids wire
protocol changes, and makes this PR non consensus/state breaking.

Includes substantial version changes to:

* tendermint-rs
* tonic #4400
* ibc-types #4682
* cnidarium #4956

---------

Co-authored-by: Richard Janis Goldschmidt <[email protected]>
conorsch added a commit that referenced this issue Jan 14, 2025
## Describe your changes
This PR re-implements the changes from #4973, which were merged into the
`release/v0.82.x` branch, but never landed on main. I'm resubmitting
them so that we can address the HTTPS breakage, documented below, prior
to tackling the rest of the changes required for getting the workspace
crates published (#4978).

Continuation of #4963, into a release branch `v0.82.x` before tagging a
release candidate at that version and publishing the workspace using an
`alpha` version.

This handles the domain type change for upgradeable channels
(penumbra-zone/ibc-types#84) smoothly. It makes
sure to write default values to the new fields, which avoids wire
protocol changes, and makes this PR non consensus/state breaking.

Includes substantial version changes to:

* tendermint-rs
* tonic #4400
* ibc-types #4682
* cnidarium #4956

## Issue ticket number and link

This PR resubmits the changes in #4973, in an attempt to isolate
problematic behavior.

## Testing and review

The primary motivation for this changeset was to address the following
error, which occurred when I tried to sync a wallet against testnet
(using an HTTPS connection):
```
2025-01-10T22:41:38.135876Z DEBUG load_or_initialize{path=Some("/tmp/nix-shell.5VH2AV/tmp.4Ay22mKmyY/pcli-view.sqlite") url=https://testnet.plinfra.net/}: penumbra_view::storage: database does not exist path="/tmp/nix-shell.5VH2AV/tmp.4Ay22mKmyY/pcli-view.sqlite"
thread 'main' panicked at /home/conor/.cargo/registry/src/index.crates.io-6f17d22bba15001f/rustls-0.23.20/src/crypto/mod.rs:249:14:
no process-level CryptoProvider available -- call CryptoProvider::install_default() before this point
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```

The most recent commit on this branch addresses that problem, by

1. explicitly configuring a default crypto provider via
[rustls](https://docs.rs/rustls)
2. reusing a new `ViewServer::get_pd_channel` method throughout the
codebase to handle conditional TLS config

Feedback welcome on whether the new logic is clearly documented and
stored in the right place.

## Checklist before requesting a review

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

> There are significant version bumps in this patch set, but previous
discussion concluded that the changes are not consensus-breaking; see
#4682 (comment)

---------

Co-authored-by: Erwan Or <[email protected]>
Co-authored-by: Richard Janis Goldschmidt <[email protected]>
conorsch added a commit that referenced this issue Jan 21, 2025
## Describe your changes

Once again, we update significant dependencies on the main branch.
Significantly, we've removed all git-dependencies from the workspace,
which required upgrading `tonic` to upstream, which cascaded into other
dependencies updates. With this change, we make the workspace nearly
ready for publication to crates.io (#4978): still to come is a
superficial refactor to rename the crates to `penumbra-sdk-*`, which
I'll submit in a follow-up PR, to aid in review.

This reverts commit a454870.

## Issue ticket number and link


We've added and then removed this changeset a few times over the past
week:

* #4973
* #4980
* #4993

Now that we've tested it extensively, it's time to merge it into main
and keep moving forward. Relevant version bumps were tracked in the
following issues:

* tonic #4400
* ibc-types #4682
* cnidarium #4956
* tendermint-rs (no issue)


## Testing and review
In order to be certain that the changes honor protocol compatibility, I
made sure to test syncing a mainnet fullnode based on this changeset
from height 2622918—which is shortly after the change upgrade to
0.81.0—to 3136597, which is current height at time of writing. This
gives us great confidence that the hard work toward ensuring that
changes like #4682 were achieved in a compatible way.

## Checklist before requesting a review

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

> see testing notes above: we're confident we've done the needful with
this patch

Co-authored-by: Erwan Or <[email protected]>
Co-authored-by: Erwan Or <[email protected]>
Co-authored-by: Richard Janis Goldschmidt <[email protected]>
@conorsch
Copy link
Contributor Author

Resolved in #4997, which shipped as part of v1.0.0.

@github-project-automation github-project-automation bot moved this from Backlog to Done in Penumbra Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus-breaking breaking change to execution of on-chain data needs-refinement unclear, incomplete, or stub issue that needs work
Projects
Archived in project
Development

No branches or pull requests

2 participants