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

Keystore overhaul (final) #13683

Merged
merged 15 commits into from
Mar 24, 2023
Merged

Conversation

davxy
Copy link
Member

@davxy davxy commented Mar 22, 2023

Closes #13556

polkadot companion: paritytech/polkadot#6944

cumulus companion: paritytech/cumulus#2371


Within this PR:

  • Removal of CryptoTypePublicId type
  • Introduce explicit signing methods to the Keystore trait
  • Keystore::sign_with method has been retained. But a default implementation has been provided in the trait definition. In the end this is a convenience method that can be used in some specific use cases (to prevent code duplication in all the cases where the key is generic such as AURA).
    • nevertheless in all the places where we can use the explicit keystore methods we save the casts to and from raw byte vectors that by definition are fallible (but in our use cases were infallible)
  • Trivial renaming of AppKey trait to AppCrypto. The trait is implemented by types (signatures included) from whom the whole set of application crypto associated types and consts can be retrieved. Thus this new name fits better
  • CRYPTO_ID in AppCrypto has been retained because without it sign_with can't work
  • Wherever is possible prefer infallible sign methods over fallible sign_with

@davxy davxy self-assigned this Mar 23, 2023
@davxy davxy marked this pull request as ready for review March 23, 2023 08:34
@davxy davxy requested a review from andresilva as a code owner March 23, 2023 08:34
@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 Mar 23, 2023
@davxy davxy requested review from bkchr and a team March 23, 2023 08:37
client/authority-discovery/src/error.rs Show resolved Hide resolved
primitives/keystore/src/lib.rs Outdated Show resolved Hide resolved
@davxy davxy requested review from koute and a team March 23, 2023 16:52
@davxy davxy requested a review from a team March 24, 2023 08:56
client/consensus/babe/src/lib.rs Outdated Show resolved Hide resolved
client/authority-discovery/src/error.rs Outdated Show resolved Hide resolved
@davxy
Copy link
Member Author

davxy commented Mar 24, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit bf395c8 into master Mar 24, 2023
@paritytech-processbot paritytech-processbot bot deleted the davxy-keystore-refactory-iter3 branch March 24, 2023 13:46
use codec::Encode;

let signature = match crypto_id {
sr25519::CRYPTO_ID => {
Copy link
Member

Choose a reason for hiding this comment

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

I thought you wanted to remove CRYPTO_ID ?

Copy link
Member Author

@davxy davxy Mar 25, 2023

Choose a reason for hiding this comment

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

@bkchr

Yes, that was indeed the awesome original idea.

Unfortunately, though, considering that AURA is generic over the crypto scheme, we need a way to select the correct scheme to use.

The ideal solution (IIRC something like what you suggested during our chat), is to have a sign-with method generic over an object implementing Pair.
That is, something like this

Thus a generic sign_with method would look like:

fn sign_with<P: Pair>(&self, id: AppCryptoId, public: &P::Public, msg: &[u8]) -> P::Signature;

Sadly a trait with a method like this can't be made into a trait-objejct (as it depends on a generic type) and we use the keystore as a Arc<dyn Keystore> all over the project.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sadly a trait with a method like this can't be made into a trait-objejct (as it depends on a generic type) and we use the keystore as a Arc<dyn Keystore> all over the project.

Well, if you'd go through &dyn then you could, e.g.:

use std::sync::Arc;

trait Public {}
trait Pair {
    type Public: Public;
    type Signature: 'static;
}

trait Keystore {
    fn yo_sign_this_plz_impl(&self, pair: &dyn Public) -> Box<dyn std::any::Any>;
}

impl dyn Keystore {
    fn yo_sign_this_plz<P: Pair>(&self, pair: &P::Public) -> P::Signature {
        match self.yo_sign_this_plz_impl(pair).downcast() {
            Ok(sig) => *sig,
            Err(_) => unreachable!()
        }
    }
}

fn it_works<P: Pair>(keystore: Arc<dyn Keystore>, pubkey: P::Public) -> P::Signature {
    keystore.yo_sign_this_plz::<P>(&pubkey)
}

But (in this example) this would require that Public is now object safe, so you'd have to use similar tricks for it, and so on until you make everything object safe all the way to the bottom. It's turtles all the way down. Probably not worth it though?

@nazar-pc nazar-pc mentioned this pull request Apr 2, 2023
1 task
breathx pushed a commit to gear-tech/substrate that referenced this pull request Apr 22, 2023
* Introduce keystore specialized sign methods

* Get rid of 'AppKey::UntypedGeneric' associated type.

Untyped generics are accessible using associated types 'Generic' associated type.
I.e. <T as AppKey>::Public::Generic

* Get rid of 'CryptoTypePublicPair'

* Trivial fix

* Small refactory of local keystore implementations

* Remove 'crypto_id' method from 'Public'

* Trivial rename of 'AppKey' to 'AppCrypto'

* Remove unused import

* Improve docs

* Better signature related errors for authority-discovery

* Apply review suggestion

* Apply review suggestions

Co-authored-by: Koute <koute@users.noreply.github.com>

* Authority discoverty signing error revisited

* Signing error revisited for babe and aura as well

* Further cleanup

---------

Co-authored-by: Koute <koute@users.noreply.github.com>
AurevoirXavier added a commit to darwinia-network/darwinia that referenced this pull request Jul 11, 2023
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* Introduce keystore specialized sign methods

* Get rid of 'AppKey::UntypedGeneric' associated type.

Untyped generics are accessible using associated types 'Generic' associated type.
I.e. <T as AppKey>::Public::Generic

* Get rid of 'CryptoTypePublicPair'

* Trivial fix

* Small refactory of local keystore implementations

* Remove 'crypto_id' method from 'Public'

* Trivial rename of 'AppKey' to 'AppCrypto'

* Remove unused import

* Improve docs

* Better signature related errors for authority-discovery

* Apply review suggestion

* Apply review suggestions

Co-authored-by: Koute <koute@users.noreply.github.com>

* Authority discoverty signing error revisited

* Signing error revisited for babe and aura as well

* Further cleanup

---------

Co-authored-by: Koute <koute@users.noreply.github.com>
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.

Keystore overhaul
4 participants