Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Relax Send/Sync/Clone requirements for Pair #14647

Merged
merged 5 commits into from
Jul 27, 2023

Conversation

davxy
Copy link
Member

@davxy davxy commented Jul 26, 2023

In general Pair trait is not strictly required to be Send/Sync/Clone.

Secrets throughout the codebase are constructed and destroyed asap (as should be).

In some situation these bounds could even be detrimental; some components may require the secrets to not be shared across threads (in practice silently moved to another stack without cleaning up the previous location) or cloned (e.g. in cases where the backend doesn't implement something like zeroize to clean-up memory).

Furthermore, secrets provided by some backends may not allow for these bounds. For example the upcoming Bandersnatch ring-vrf secret is not Sync. So here there is not much space for options wrt this trait bound.


The only place where these bounds are currently vacuously required is in AURA code.

Here some components are generic over the key type and these components are required to be Send.
However the Pair is not part of the components struct instance and the generic is only used as a bound to a particular key type and thus authority id type (i.e. these structs contain a PhantomData<Pair>).

The issue is easily addressed by using PhantomData<fn() -> Pair> instead of PhantomData<Pair>.
PhantomData<fn() -> Pair> is both Send and Sync (regardless of Pair bounds).

That is, the types with PhantomData doesn't own a Pair, but instead they can potentially produce Pairs (this is the big difference).


The PR also contains trivial cleanups and removal of other bounds where possible


cumulus companion: paritytech/cumulus#2941

required by #14412

Nomicon ref (not published yet): rust-lang/nomicon#411

@davxy davxy requested a review from a team July 26, 2023 14:16
@davxy davxy self-assigned this Jul 26, 2023
@davxy davxy added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jul 26, 2023
@davxy davxy changed the title Relax Send/Sync requirements for Pair Relax Send/Sync/Clone requirements for Pair Jul 26, 2023
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.

👍

davxy and others added 2 commits July 27, 2023 10:05
Co-authored-by: Sebastian Kunert <skunert49@gmail.com>
@davxy
Copy link
Member Author

davxy commented Jul 27, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 14e0a0b into master Jul 27, 2023
5 checks passed
@paritytech-processbot paritytech-processbot bot deleted the davxy-relax-pair-bounds branch July 27, 2023 10:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants