Make BEEFY client keystore generic over BEEFY AuthorityId type
#2258
Conversation
…test - update `substrate/primitives/consensus/beefy/src/lib.rs` accordingly
Co-authored-by: Davide Galassi <[email protected]>
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.
…nt-keystore-supporting-both-ecdsa-and-bls
Co-authored-by: Robert Hambrock <[email protected]>
…ttps://github.com/w3f/polkadot-sdk into skalman--beefy-client-keystore-supporting-both-ecdsa-and-bls
…ash of the message instead of Blake2
Co-authored-by: Davide Galassi <[email protected]>
…both-ecdsa-and-bls
…d-bls' of https://github.com/w3f/polkadot-sdk into skalman--beefy-client-keystore-supporting-both-ecdsa-and-bls
… skalman--beefy-client-keystore-supporting-both-ecdsa-and-bls
There was a problem hiding this comment.
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).
Co-authored-by: Robert Hambrock <[email protected]>
I made an issue: w3f#3 |
…both-ecdsa-and-bls
Co-authored-by: Robert Hambrock <[email protected]>
…ed initially. Co-authored-by: Robert Hambrock <[email protected]>
…nt-keystore-supporting-both-ecdsa-and-bls
| pub(crate) struct BeefyKeystore(Option<KeystorePtr>); | ||
| pub(crate) struct BeefyKeystore<AuthorityId: AuthorityIdBound>( | ||
| Option<KeystorePtr>, | ||
| PhantomData<fn() -> AuthorityId>, |
There was a problem hiding this comment.
nit:
can this just be
| PhantomData<fn() -> AuthorityId>, | |
| PhantomData<AuthorityId>, |
?
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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)]) | ||
| }) |
There was a problem hiding this comment.
Yes having multiple validator keys is an node-operator error, and this just helps to warn the operator.
…both-ecdsa-and-bls
…both-ecdsa-and-bls
0a94124
…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]>
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)