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

feat: Better error reporting when features are disabled #2972

Merged
merged 39 commits into from
Nov 23, 2022

Conversation

thomaseizinger
Copy link
Contributor

@thomaseizinger thomaseizinger commented Oct 4, 2022

Description

In case support for e.g. RSA keys is disabled at compile-time, we will now print a better error message. For example:

Failed to dial Some(PeerId("QmcZf59bWwK5XFi76CZX8cbJ4BhTzzA3gU1ZjYZcYW3dwt")): Failed to negotiate transport protocol(s): [(/dnsaddr/bootstrap.libp2p.io/p2p/QmcZf59bWwK5XFi76CZX8cbJ4BhTzzA3gU1ZjYZcYW3dwt): : Handshake failed: Handshake failed: Invalid public key: Key decoding error: RSA keys are unsupported)]

Fixes #2971.

Links to any relevant issues

Depends-On: #3081

Open Questions

Change checklist

  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • A changelog entry has been made in the appropriate crates

Copy link
Member

@mxinden mxinden left a comment

Choose a reason for hiding this comment

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

As always, great work. Thank you for tackling this so thoroughly. Thanks for breaking it up into smaller meaningful commits.

core/src/identity.rs Outdated Show resolved Hide resolved
swarm/src/lib.rs Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor Author

@mxinden
Copy link
Member

mxinden commented Oct 5, 2022

CI is failing with:

 error: failed to select a version for `zeroize`.
    ... required by package `der v0.6.0`
    ... which satisfies dependency `der = "^0.6"` of package `pkcs8 v0.9.0`
    ... which satisfies dependency `pkcs8 = "^0.9"` of package `sec1 v0.3.0`
    ... which satisfies dependency `sec1 = "^0.3.0"` of package `libp2p-core v0.37.0 (https://github.com/libp2p/rust-libp2p?rev=fd870f8f4ee1fe54ff605869632ea7e21ecfd3b7#fd870f8f)`
    ... which satisfies git dependency `libp2p-core` of package `libp2p v0.49.0 (https://github.com/libp2p/rust-libp2p?rev=fd870f8f4ee1fe54ff605869632ea7e21ecfd3b7#fd870f8f)`
    ... which satisfies git dependency `libp2pmaster` of package `testplan v0.1.0 (/usr/src/testplan/plan)`
versions that meet the requirements `^1.5` are: 1.5.7, 1.5.6, 1.5.5, 1.5.4, 1.5.3

all possible versions conflict with previously selected packages.

  previously selected package `zeroize v1.3.0`
    ... which satisfies dependency `zeroize = "^1"` (locked to 1.3.0) of package `chacha20 v0.8.2`
    ... which satisfies dependency `chacha20 = "^0.8"` (locked to 0.8.2) of package `chacha20poly1305 v0.9.1`
    ... which satisfies dependency `chacha20poly1305 = "^0.9"` (locked to 0.9.1) of package `snow v0.9.0`
    ... which satisfies dependency `snow = "^0.9.0"` (locked to 0.9.0) of package `libp2p-noise v0.35.0`
    ... which satisfies dependency `libp2p-noise = "^0.35.0"` (locked to 0.35.0) of package `libp2p v0.44.0`
    ... which satisfies dependency `libp2pv0440 = "^0.44.0"` (locked to 0.44.0) of package `testplan v0.1.0 (/usr/src/testplan/plan)`

failed to select a version for `zeroize` which could resolve this conflict

I would like to test this with a fresh Cargo.lock. I am guessing that previously cached runs influence this one. @laurentsenta is there anyway I can run the CI job without any cache from previous runs?

@mxinden
Copy link
Member

mxinden commented Oct 17, 2022

I should have included this in #2931 but I forgot.

Can you update the pull request now that v0.49.0 is released @thomaseizinger?

@thomaseizinger thomaseizinger marked this pull request as draft November 2, 2022 04:10
mxinden added a commit to mxinden/test-plans that referenced this pull request Nov 2, 2022
Updates dependency. In the hope to resolve conflict from
libp2p/rust-libp2p#2972.
mxinden added a commit to libp2p/test-plans that referenced this pull request Nov 2, 2022
Updates dependency. In the hope to resolve conflict from
libp2p/rust-libp2p#2972.
mxinden pushed a commit to libp2p/test-plans that referenced this pull request Nov 18, 2022
By having one Rust binary per version, we can vary the actual binary from
version to version and f.e. fix deprecated API calls. It does introduce a bit of
duplication between the different versions but I'd rather have that then not
being able to adapt the tests to new APIs.

Instead of activating a feature per libp2p version, we add them all as
dependencies. This ensures all transitive dependencies are properly tracked in
`Cargo.lock`. Additionally, this gives us a single place we are can activate all
the feature.

For `master` and pull-request builds, we replace the git target or rev with the
one coming from the CI build. Once we trigger a build, `cargo` will update and
resolve the necessary dependencies before that, thus fixing problems such as
libp2p/rust-libp2p#2972.
@mxinden
Copy link
Member

mxinden commented Nov 18, 2022

@thomaseizinger libp2p/test-plans#72 is merged. Can you update this pull request?

@thomaseizinger thomaseizinger marked this pull request as ready for review November 18, 2022 13:20
@mergify
Copy link
Contributor

mergify bot commented Nov 18, 2022

This pull request has merge conflicts. Could you please resolve them @thomaseizinger? 🙏

@thomaseizinger thomaseizinger changed the title core,swarm,transports/noise: Improve error reporting feat: Better error reporting when features are disabled Nov 22, 2022
Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

@mxinden mxinden added this to the v0.50.0 milestone Nov 22, 2022
@thomaseizinger
Copy link
Contributor Author

Here we go!

@mergify mergify bot merged commit 2c96d64 into master Nov 23, 2022
@mxinden
Copy link
Member

mxinden commented Nov 23, 2022

🎉

@thomaseizinger thomaseizinger deleted the 2971-better-error-message branch February 24, 2023 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unable to dial bootstrap nodes InvalidKey
3 participants