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

Use published cnidarium in the workspace #4956

Closed
erwanor opened this issue Dec 9, 2024 · 1 comment
Closed

Use published cnidarium in the workspace #4956

erwanor opened this issue Dec 9, 2024 · 1 comment
Labels
needs-refinement unclear, incomplete, or stub issue that needs work

Comments

@erwanor
Copy link
Member

erwanor commented Dec 9, 2024

Is your feature request related to a problem? Please describe.
We published cnidarium here:

So we should housekeep the code in the monorepo, and plug into version 0.80.

@github-actions github-actions bot added the needs-refinement unclear, incomplete, or stub issue that needs work label Dec 9, 2024
@erwanor
Copy link
Member Author

erwanor commented Dec 25, 2024

Done in #4973

@erwanor erwanor closed this as completed Dec 25, 2024
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-refinement unclear, incomplete, or stub issue that needs work
Projects
None yet
Development

No branches or pull requests

1 participant