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(identity): make rand optional #4349

Merged
merged 33 commits into from
Oct 18, 2023
Merged

Conversation

monoid
Copy link
Contributor

@monoid monoid commented Aug 19, 2023

Description

Some restricted environments lack proper random source, and getrandom crate fails to compile. However, random sources is needed only for random key generation which is not always required.

This change introduces rand feature flag that is gate to all the functionality that requires rand crate.

Notes & open questions

The idea of this pull request was born when I tried to use the crate in NEAR contract.

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

Some restricted environments lack proper random source, and `getrandom` crate
fails to compile.  However, random sources is needed only for random key
generation which is not always required.

This change introduces `rand` feature flag that is gate to all the
functionality that requires `rand` crate.
@mergify
Copy link
Contributor

mergify bot commented Aug 19, 2023

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

@thomaseizinger
Copy link
Contributor

Thanks! This is unfortunately a breaking change so I'd like to hold off on it for a while. We've only just released a breaking change to libp2p-identity 2 months ago.

Is this blocking you on something? Out of interest, what is your usecase where you don't have a randomness source? Do libp2p-tls or libp2p-noise even work without randomness?

@monoid
Copy link
Contributor Author

monoid commented Aug 19, 2023

Thanks! This is unfortunately a breaking change so I'd like to hold off on it for a while. We've only just released a breaking change to libp2p-identity 2 months ago.

Is this blocking you on something?

Well, we can live with git branch deps for some time. I've submitted it mostly because it can be useful for others.

Out of interest, what is your usecase where you don't have a randomness source? Do libp2p-tls or libp2p-noise even work without randomness?

We have bunch of (normal) peers who do some coordinated task and submit their result on a chain where a smartcontract validates their outputs. The smartcontract uses libp2p-identity to match public keys and peer IDs, not using any other libp2p library.

@thomaseizinger
Copy link
Contributor

Out of interest, what is your usecase where you don't have a randomness source? Do libp2p-tls or libp2p-noise even work without randomness?

We have bunch of (normal) peers who do some coordinated task and submit their result on a chain where a smartcontract validates their outputs. The smartcontract uses libp2p-identity to match public keys and peer IDs, not using any other libp2p library.

Thanks for sharing!

Not sure if that is viable but perhaps other libraries also need randomness and the target should register a randomness source?

See https://docs.rs/getrandom/latest/getrandom/index.html#custom-implementations.

In any case we can leave this open until we are making another breaking change. libp2p-identity is the leaf dependency of the entire ecosystem of libp2p because multiaddr depends on it and breaking changes thus have quite the blast radius.

@thomaseizinger thomaseizinger added this to the v0.53.0 milestone Aug 20, 2023
@thomaseizinger
Copy link
Contributor

Note for myself: Create a separate milestone and link it to the next rust-multiaddr milestone.

It has to be enabled by rand feature only.
@mergify
Copy link
Contributor

mergify bot commented Oct 2, 2023

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

@thomaseizinger
Copy link
Contributor

I am tempted to release this as a patch-release. Whilst technically a breaking change, it will only hit users that actively say default-features = false or when they depend on a crate that set default-features = false. Because features are additive, this can easily be resolved by just adding a dependency on libp2p-identity and activating the rand feature.

@mxinden @jxs What do you think of this idea?

identity/src/keypair.rs Outdated Show resolved Hide resolved
identity/Cargo.toml Outdated Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor

Can you add the rand feature here please?

libp2p-identity = { workspace = true }

@monoid
Copy link
Contributor Author

monoid commented Oct 4, 2023

@thomaseizinger Done!

@monoid monoid requested a review from thomaseizinger October 4, 2023 08:19
@jxs
Copy link
Member

jxs commented Oct 4, 2023

I am tempted to release this as a patch-release. Whilst technically a breaking change, it will only hit users that actively say default-features = false or when they depend on a crate that set default-features = false. Because features are additive, this can easily be resolved by just adding a dependency on libp2p-identity and activating the rand feature.

@mxinden @jxs What do you think of this idea?

yeah makes sense to me Thomas, did sorta similar with refinery

@thomaseizinger thomaseizinger changed the title feat(identity)!: make rand optional feat(identity): make rand optional Oct 5, 2023
@thomaseizinger thomaseizinger removed this from the v0.53.0 milestone Oct 5, 2023
@thomaseizinger
Copy link
Contributor

Thanks for fixing the tests @monoid !

As a last thing, can you bump the patch-version of libp2p-identity and write a short changelog entry? I'll merge and release this right away after.

@monoid
Copy link
Contributor Author

monoid commented Oct 5, 2023

@thomaseizinger done!

@thomaseizinger
Copy link
Contributor

Merged and merged again.

Awesome, thanks! Sorry for the hassle!

@thomaseizinger thomaseizinger mentioned this pull request Oct 10, 2023
@monoid
Copy link
Contributor Author

monoid commented Oct 10, 2023

@thomaseizinger no problem! This is a life of an actively developed project. :)

@thomaseizinger
Copy link
Contributor

I think some unit tests still implicitly depend on the rand feature and are now failing because it is not there. I think you'll have to add a dedicated dev-dependencies entry for those where we activate the rand feature.

@monoid
Copy link
Contributor Author

monoid commented Oct 10, 2023

@thomaseizinger tests should work now.

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.

Feel free to go ahead here @thomaseizinger. Thank you for driving this to both of you.

@thomaseizinger when you cut the patch release, would you mind keeping an eye open the repositories of our largest users to make sure they don't face any difficulties with the upgrade?

Projects that come to my mind are:

@thomaseizinger
Copy link
Contributor

@thomaseizinger tests should work now.

Still a few more failures unfortunately. You can ignore the semver job, that one fails because this is a bit of a special situation with libp2p-identity.

@thomaseizinger
Copy link
Contributor

Ugh, these two kademlia tests seems to be flaky as of recently. I'll deal with that tomorrow ..

@monoid
Copy link
Contributor Author

monoid commented Oct 11, 2023

Ok, I'm always at your disposal to merge/rebase/etc.

@thomaseizinger
Copy link
Contributor

Okay, flaky tests are fixed on master, can you merge that into this branch please?

@monoid
Copy link
Contributor Author

monoid commented Oct 14, 2023

Done!

@mxinden
Copy link
Member

mxinden commented Oct 16, 2023

Many CI steps are failing now that #4620 is merged.

Given that libp2p now enables the identity/rand feature and given that we will release libp2p along with libp2p-identity, I am assuming that we can ignore the failures. Is that assumption correct @thomaseizinger?

@thomaseizinger
Copy link
Contributor

Many CI steps are failing now that #4620 is merged.

Yeah I realized that the situation is not ideal 😅
In other PRs, I've added internal changelog entries but that doesn't scale very well.

I am assuming that we can ignore the failures.

You can't ignore them because they are marked as required. Mergify won't merge your PR until they pass. Let's brain-storm something later today.

@mergify
Copy link
Contributor

mergify bot commented Oct 16, 2023

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

@thomaseizinger thomaseizinger added the internal-change Pull requests that make internal changes to crates and thus don't need to include a changelog entry. label Oct 17, 2023
@thomaseizinger
Copy link
Contributor

thomaseizinger commented Oct 18, 2023

@monoid There are no other PRs queued for merge at the moment so if you are around, we can get this one in soon :)

I'll need you to merge master in one more time. Unfortunately, GitHub doesn't allow me to update your branch because it is coming from an organisation. For future contributions (:pray:), things would be a bit easier if you'd have a personal fork :)

Thanks for this PR and sorry that it has been dragging on for so long!

@monoid
Copy link
Contributor Author

monoid commented Oct 18, 2023

Yeah, I know! Will make PR from the personal repo next time :) I wasn't aware of this restriction (perhaps, it's just particular organization's security settings).

Thank you and other guys for the assistance and advices!

I've merged the master branch. I will be online for the next 12h if any more action is needed on my side.

@mergify mergify bot merged commit 17c166a into libp2p:master Oct 18, 2023
71 of 72 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal-change Pull requests that make internal changes to crates and thus don't need to include a changelog entry. send-it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants