penumbra: update ecosystem tendermint/ibc crates #4980

conorsch · 2025-01-10T22:52:44Z

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 chore: 🧃 upgrade to [email protected] #4400
ibc-types ibc-types: update to v0.14.0 #4682
cnidarium Use published cnidarium in the workspace #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

explicitly configuring a default crypto provider via rustls
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

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:

There are significant version bumps in this patch set, but previous discussion concluded that the changes are not consensus-breaking; see ibc-types: update to v0.14.0 #4682 (comment)

SuperFluffy · 2025-01-13T16:59:15Z

@conorsch Could you flag the critical bits? In #4963 (the precusor to #4973) I think I flagged some critical parts in http1/http2 support where the out-of-the-box support provided by axum and hyper was not clear.

I am very interested in what steps you took to address your issues.

## Describe your changes While working on the tonic version bumps in #4980, we discovered that the HTTPS client support can break without failing CI. This PR builds on the new tests in #4979, trying to establish a baseline sanity check that "yes, the programs can talk to https endpoints" so that during a large refactor, we can easily confirm no regressions. ## Issue ticket number and link Refs * #4980 * #4979 * #4400 ## Testing and review Check out this branch, run `just integration-testnet` and confirm all checks pass. I also tacked on a commit enabling these new tests in CI, which I consider temporary, but useful for the immediate near-term. It'd also be helpful to point out any places that might use HTTPS clients that aren't covered yet. ## 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: > tests/CI only, no app code changes

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]>

Pulls in a dependency on `rustls` to configure the TLS config for HTTPS clients. Refactored the `pd_channel` logic into a reusable class method.

conorsch · 2025-01-14T19:42:13Z

@SuperFluffy The most recent commit, fix: use rustls for https client support, shows what was necessary to restore HTTPS client functionality. The new integration tests for talking to HTTPS endpoints (#4983) were helpful in debugging, but notably they don't exercise the auto-https logic for pd. I manually tested the auto-https logic and confirmed it's working, with the cryptoprovider changes made to pd.

erwanor

LGTM, except for the pd behavior

erwanor · 2025-01-14T19:44:35Z

crates/bin/pclientd/src/main.rs

@@ -24,5 +25,11 @@ async fn main() -> Result<()> {

    let opt = Opt::parse();

+    // Initialize HTTPS support
+    // rustls::crypto::aws_lc_rs::default_provider().install_default();


Suggested change

// rustls::crypto::aws_lc_rs::default_provider().install_default();

erwanor · 2025-01-14T19:45:25Z

crates/bin/pd/src/main.rs

+    // Initialize HTTPS support
+    aws_lc_rs::default_provider()
+        .install_default()
+        .expect("failed to initialize rustls support, via aws-lc-rs");


I think we should trace the error, but allow the full node to start

@erwanor

Addressing review comments by @erwanor.

cronokirby · 2025-01-14T20:19:25Z

crates/core/app/src/server/events.rs

+                    // TODO: what should be done with this error? Emit a warning?
+                    Err(_err) => {
+                        attr.set_index(false);
+                    }


Maybe we could log here, but otherwise seems fine to just not index non-utf8 attributes. We don't generate any of those, and the basic cometbft events don't contain such keys either.

cronokirby

Failing for HTTPS in pclientd but not pd makes sense to me

This reverts commit 64c32ef, which constitutes the squash-merge of PR #4980. We're backing this change out strictly to simplify release engineering: we want the `main` branch to remain fully compatible with the `0.81.x` series, and we'll continue QA of significant version changes in a parallel release branch, `release/v0.82.x`. I'll handle preparing the latter shortly. See related discussion in #4988 & #4991.

) ## Describe your changes This reverts commit 64c32ef, which constitutes the squash-merge of PR #4980. We're backing this change out strictly to simplify release engineering: we want the `main` branch to remain fully compatible with the `0.81.x` series, and we'll continue QA of significant version changes in a parallel release branch, `release/v0.82.x`. I'll handle preparing the latter shortly. ## Issue ticket number and link See related discussion in #4988 & #4991. ## Testing and review This is a programmatic change, in that I simply ran `git revert 64c32ef`, wrote some notes into the commit message, and pushed it up. I also made sure to rerun `just proto` to regenerate the protos, and confirmed there are no changes. That's good, that's precisely what we wanted to see. Preferably this change would land before #4992, since #4992 changes protos. I'll regenerate protos in 4992 on top of this once it lands on main. ## 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: > This commit is expressly intended to preserve protocol compatibility with 0.81.x. Future work on QA to ensure compat with 0.82 will happen in a separate branch.

…4993) This reverts commit a454870.

…4993) This reverts commit a454870. Co-authored-by: Erwan Or <[email protected]> Co-authored-by: Erwan Or <[email protected]> Co-authored-by: Richard Janis Goldschmidt <[email protected]>

## 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 requested a review from erwanor January 10, 2025 22:52

conorsch had a problem deploying to smoke-test January 10, 2025 22:52 — with GitHub Actions Failure

conorsch temporarily deployed to smoke-test January 13, 2025 17:42 — with GitHub Actions Inactive

conorsch force-pushed the update-crates-ibc-tendermint-and-tonic branch from 2e4ee89 to a44f661 Compare January 13, 2025 18:05

conorsch temporarily deployed to smoke-test January 13, 2025 18:05 — with GitHub Actions Inactive

conorsch mentioned this pull request Jan 14, 2025

test: add integration tests for https functionality to binaries #4983

Merged

2 tasks

SuperFluffy mentioned this pull request Jan 14, 2025

bump Sequencer to tendermint 0.40 astriaorg/astria#1914

Open

conorsch force-pushed the update-crates-ibc-tendermint-and-tonic branch from a44f661 to 4ba03ff Compare January 14, 2025 18:50

conorsch temporarily deployed to smoke-test January 14, 2025 18:50 — with GitHub Actions Inactive

conorsch marked this pull request as ready for review January 14, 2025 19:08

conorsch requested a review from cronokirby January 14, 2025 19:15

fix: use rustls for https client support

30e986e

Pulls in a dependency on `rustls` to configure the TLS config for HTTPS clients. Refactored the `pd_channel` logic into a reusable class method.

conorsch force-pushed the update-crates-ibc-tendermint-and-tonic branch from 4ba03ff to 30e986e Compare January 14, 2025 19:37

conorsch temporarily deployed to smoke-test January 14, 2025 19:38 — with GitHub Actions Inactive

erwanor approved these changes Jan 14, 2025

View reviewed changes

fix: log pd cryptoprovider error, but don't exit

0221ed6

Addressing review comments by @erwanor.

conorsch temporarily deployed to smoke-test January 14, 2025 20:09 — with GitHub Actions Inactive

cronokirby reviewed Jan 14, 2025

View reviewed changes

cronokirby approved these changes Jan 14, 2025

View reviewed changes

conorsch merged commit 64c32ef into main Jan 14, 2025
15 checks passed

conorsch deleted the update-crates-ibc-tendermint-and-tonic branch January 14, 2025 20:27

This was referenced Jan 16, 2025

Release v0.81.1, via point-release #4988

Closed

Release v1.0.0 🎉 #4991

Closed

conorsch mentioned this pull request Jan 17, 2025

Revert "penumbra: update ecosystem tendermint/ibc crates (#4980)" #4993

Merged

2 tasks

conorsch added a commit that referenced this pull request Jan 17, 2025

Reapply "penumbra: update ecosystem tendermint/ibc crates (#4980)" (#…

07c6f73

…4993) This reverts commit a454870.

conorsch added a commit that referenced this pull request Jan 19, 2025

Reapply "penumbra: update ecosystem tendermint/ibc crates (#4980)" (#…

5e50e08

…4993) This reverts commit a454870.

conorsch added a commit that referenced this pull request Jan 21, 2025

Reapply "penumbra: update ecosystem tendermint/ibc crates (#4980)" (#…

2137191

…4993) This reverts commit a454870.

conorsch mentioned this pull request Jan 21, 2025

penumbra: update ecosystem tendermint/ibc crates #4997

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

penumbra: update ecosystem tendermint/ibc crates #4980

penumbra: update ecosystem tendermint/ibc crates #4980

conorsch commented Jan 10, 2025 •

edited

Loading

SuperFluffy commented Jan 13, 2025

conorsch commented Jan 14, 2025

erwanor left a comment

erwanor Jan 14, 2025

erwanor Jan 14, 2025

cronokirby Jan 14, 2025

cronokirby left a comment

penumbra: update ecosystem tendermint/ibc crates #4980

penumbra: update ecosystem tendermint/ibc crates #4980

Conversation

conorsch commented Jan 10, 2025 • edited Loading

Describe your changes

Issue ticket number and link

Testing and review

Checklist before requesting a review

SuperFluffy commented Jan 13, 2025

conorsch commented Jan 14, 2025

erwanor left a comment

Choose a reason for hiding this comment

erwanor Jan 14, 2025

Choose a reason for hiding this comment

erwanor Jan 14, 2025

Choose a reason for hiding this comment

cronokirby Jan 14, 2025

Choose a reason for hiding this comment

cronokirby left a comment

Choose a reason for hiding this comment

conorsch commented Jan 10, 2025 •

edited

Loading