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

Make BEEFY client keystore generic over BEEFY AuthorityId type #2258

Conversation

drskalman
Copy link
Contributor

This is the significant step to make BEEFY client able to handle both ECDSA and (ECDSA, BLS) type signature. The idea is having BEEFY Client generic on crypto types makes migration to new types smoother.

This makes the BEEFY Keystore generic over AuthorityId and extends its tests to cover the case when the AuthorityId is of type (ECDSA, BLS12-377)

drskalman and others added 9 commits October 27, 2023 12:21
…test

- update `substrate/primitives/consensus/beefy/src/lib.rs` accordingly
make BEEFY keystore generic over `AuthorityId` and works with both (ECDSA) and (ECDSA, BLS377) key types.
pass all the BEEFY tests for ECDSA
- Make BEEFY Keystore test generic over `AuthorityId`.
- Implement keystore tests for `ecdsa_bls` crypto.
- Fix `bls::Pair::derive` and write test for it.
Co-authored-by: Robert Hambrock <[email protected]>
@paritytech-review-bot paritytech-review-bot bot requested a review from a team November 9, 2023 18:27
@davxy davxy added the T0-node This PR/Issue is related to the topic “node”. label Nov 10, 2023
Copy link
Contributor

@Lederstrumpf Lederstrumpf left a comment

Choose a reason for hiding this comment

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

First round of scattered fixes & comments.

Also, do you have an issue on a w3f repo or so for tracking the big picture goal of these sequential PRs? It would likely increase the quality of reviews your PRs receive if not only their incremental purpose, but also the changes to follow the PR, is clear. More details in the PR description would help too (not a comment on this PR specifically, but the overall flow of them so far).

substrate/primitives/consensus/beefy/src/test_utils.rs Outdated Show resolved Hide resolved
substrate/client/consensus/beefy/src/keystore.rs Outdated Show resolved Hide resolved
substrate/client/consensus/beefy/src/keystore.rs Outdated Show resolved Hide resolved
substrate/client/consensus/beefy/src/keystore.rs Outdated Show resolved Hide resolved
substrate/primitives/consensus/beefy/src/lib.rs Outdated Show resolved Hide resolved
substrate/primitives/core/src/paired_crypto.rs Outdated Show resolved Hide resolved
substrate/primitives/keystore/src/lib.rs Outdated Show resolved Hide resolved
substrate/client/keystore/src/local.rs Show resolved Hide resolved
Co-authored-by: Robert Hambrock <[email protected]>
@drskalman
Copy link
Contributor Author

lso, do you have an issue on a w3f repo or so for tracking the big picture goal of these sequential PRs?

I made an issue: w3f#3

@davxy davxy added the R0-silent Changes should not be mentioned in any release notes label Jan 29, 2024
Copy link
Contributor

@Lederstrumpf Lederstrumpf left a comment

Choose a reason for hiding this comment

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

lgtm - just please also test recovery of the thing you actually wanna recover: secrets, not pubkeys ;)

substrate/primitives/core/src/bls.rs Outdated Show resolved Hide resolved
substrate/primitives/core/src/ecdsa.rs Show resolved Hide resolved
substrate/primitives/core/src/ecdsa.rs Show resolved Hide resolved
substrate/primitives/core/src/ecdsa.rs Show resolved Hide resolved
substrate/primitives/core/src/ed25519.rs Show resolved Hide resolved
substrate/primitives/core/src/sr25519.rs Show resolved Hide resolved
Copy link
Contributor

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

great work @drskalman !

pub(crate) struct BeefyKeystore(Option<KeystorePtr>);
pub(crate) struct BeefyKeystore<AuthorityId: AuthorityIdBound>(
Option<KeystorePtr>,
PhantomData<fn() -> AuthorityId>,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:
can this just be

Suggested change
PhantomData<fn() -> AuthorityId>,
PhantomData<AuthorityId>,

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The code I believe is from @davxy so he can explain better perhaps.

But my understanding is that PhantomData<AuthorityId> tells the compiler that BeefyKeystore owns an element of AuthorityId type and the compiler should perform a drop check for that type when it is destrying a BeefyKeystore. On the other hand PhantomData<fn() -> AuthorityId> doesn't mandate that check.

We have opted for the later, because BeefyKeystore in fact at no time owns an AuthorityId element ( in form of pionter or otherwise) (See this and this for ref.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or maybe the code is mine but it was inspired by @davxy using the same pattern before...

.filter(|k| {
store
.has_keys(&[(<AuthorityId as RuntimeAppPublic>::to_raw_vec(k), BEEFY_KEY_TYPE)])
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes having multiple validator keys is an node-operator error, and this just helps to warn the operator.

@acatangiu acatangiu added this pull request to the merge queue Feb 8, 2024
Merged via the queue into paritytech:master with commit 0a94124 Feb 8, 2024
101 of 125 checks passed
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
…itytech#2258)

This is the significant step to make BEEFY client able to handle both
ECDSA and (ECDSA, BLS) type signature. The idea is having BEEFY Client
generic on crypto types makes migration to new types smoother.

This makes the BEEFY Keystore generic over AuthorityId and extends its
tests to cover the case when the AuthorityId is of type (ECDSA,
BLS12-377)

---------

Co-authored-by: Davide Galassi <[email protected]>
Co-authored-by: Robert Hambrock <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
R0-silent Changes should not be mentioned in any release notes T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants