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

Overhaul crypto (Schnorr/Ristretto, HDKD, BIP39) #1795

Merged
merged 43 commits into from
Mar 13, 2019
Merged

Overhaul crypto (Schnorr/Ristretto, HDKD, BIP39) #1795

merged 43 commits into from
Mar 13, 2019

Conversation

gavofyork
Copy link
Member

@gavofyork gavofyork commented Feb 14, 2019 •
edited
Loading

  • Make Sr25519 the default crypto for accounts/transaction signing.
  • Introduces a new "Secret URI" format for specifying secret keys <phrase>/<soft-key>//<hard-key>///<password> (the ///password may be omitted, and /<soft-key> and //<hard-key> maybe repeated and mixed).
    • Derivation path items may be numeric (encoded as 256-bit LE) or textual (zero-padded to the right unless greater than 32-characters, in which case hashed using blake2).
    • Sr25519 supports BIP39 phrases, a password and a derivation path of any number of soft and hard keys.
    • Ed25519 supports BIP39 phrases, a password and a derivation path of any number of hard keys.
    • Sr25519's Public key type can also be derived now (though nothing uses it yet).
    • SURIs can be used in subkey and on the CLI for the --key parameter.
  • Untangles Authority signing (which remains Ed25519) from Account (extrinsic) signing (which is Sr25519).
    • The two different keyrings and named AccountKeyring and AuthorityKeyring accordingly.
    • Some dependent hashes in tests have been removed in order to minimise faff when moving between signing schemes.
    • Alice, Bob &c. keys are now formed as SURIs with a single well known seed phrase and different paths for each identity, which is somewhat cleaner than the whitespace padded seeds from before.
  • This also revamps the use of the crypto:
    • ed25519/sr25519 Pairs are now trait impls.
    • AuthorityId and AccountId are now type-safe throughout the codebase (ed25519::Public or sr25519::Public) and not just opaque, weakly typed H256s.
    • This removes the need for a lot of crap code when dealing with Keyring; a lot less .to_raw_public, .0 and .into.
    • Removed all mentions of the Ed25519AuthorityId non-type, replacing with local aliasing of ed25519::Public to AuthorityId, ready for a later refactoring that can replace specific sets of instances with BLS or other crypto.
    • Remove a lot of the highly dodgy coercions between strictly typed public keys and weakly typed arrays (that happen to be the same length).
    • Introduce UncheckedFrom and UncheckedInto traits for the times (actually, there's only one instance) where we do need to break strong typing and force coercion.

TODO:

  • Rejig Keyring::One and ::Two.
  • Remove hard-coded hashes from test-runtime/system.rs tests.
  • Make subkey support public key derivation querying.
  • Make subkey support signing.

CC @jacogr

@gavofyork gavofyork added A2-insubstantial Pull request requires no code review (e.g., a sub-repository hash update). A1-onice labels Feb 14, 2019
@pepyakin
Copy link
Contributor

This doesn't rebuild runtime wasm artifacts. Should it?

@burdges
Copy link

burdges commented Feb 14, 2019

I added an attach_rng function in w3f/schnorrkel@9f20351#diff-e0934bb6f70a4f8c3baf22a773c14ec7R331 in case the bindings need manual control over the randomness.

@jacogr
Copy link
Contributor

jacogr commented Feb 14, 2019

@pepyakin yes, probably along with a version bump as well so detection of which version to use is possible for APIs.

Thank you though, was wondering about local tests not yielding the expected results, I needed to rebuild.

@gavofyork gavofyork added A0-please_review Pull request needs code review. and removed A4-gotissues labels Feb 15, 2019
@gguoss
Copy link
Contributor

gguoss commented Mar 1, 2019

Not compatible with consensus and finality-grandpa modules？ Because these modules are hardcoded ed25519.

@gavofyork gavofyork mentioned this pull request Mar 6, 2019
Merged
tomusdrw
tomusdrw approved these changes Mar 7, 2019
View reviewed changes
node/cli/src/chain_spec.rs Outdated
let endowed_accounts: Vec<AccountId> = vec![
hex!["f295940fa750df68a686fcf4abd4111c8a9c5a5a5a83c4c8639c451a94a7adfd"].into(), // 5HYmsxGRAmZMjyZYmf7uGPL2YDQGHEt6NjGrfUuxNEgeGBRN TODO: change once we switch to sr25519
hex!["343df6f04ffae0840f214f6cb0da00b612c7e9347f980e7afafc520582f79136"].into(), // 5DFCkiP9vky31C1ZP3LpuQYinLAFwQqq6vda7NXa8ALCpq5D TODO: change once we switch to sr25519
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the TODO still valid?

Copy link
Member Author

Choose a reason for hiding this comment

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

no...

gavofyork
gavofyork commented Mar 13, 2019
View reviewed changes
core/primitives/src/sr25519.rs

// signing context
const SIGNING_CTX: &'static [u8] = b"substrate transaction";
Copy link
Member Author

Choose a reason for hiding this comment

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

@kianenigma I changed this, so JS will need update.

@gavofyork
Copy link
Member Author

@tomusdrw I addressed the grumbles. @kianenigma If you're also happy with the code, then I'd like to merge today.

@gavofyork gavofyork mentioned this pull request Mar 13, 2019
Merged
tomusdrw
tomusdrw approved these changes Mar 13, 2019
View reviewed changes
core/primitives/src/crypto.rs
}
}

let re = Regex::new(r"^(?P<phrase>\w+( \w+)*)(?P<path>(//?[^/]+)*)(///(?P<password>.*))?$")
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, but my point is that it introduces ambiguity in representation (two different strings representing the same derived key), but if we don't care about it then I'm fine.

@jacogr jacogr mentioned this pull request Mar 13, 2019
Merged
@gavofyork gavofyork merged commit 8930f29 into master Mar 13, 2019
@gavofyork gavofyork deleted the gav-enable-ristretto branch March 13, 2019 13:08
@fleupold fleupold mentioned this pull request Mar 13, 2019
Closed
@kianenigma kianenigma mentioned this pull request Mar 14, 2019
Open
@shawntabrizi shawntabrizi mentioned this pull request Mar 31, 2019
Closed
MTDK1 pushed a commit to bdevux/substrate that referenced this pull request Apr 12, 2019
@gavofyork @MTDK1
Overhaul crypto (Schnorr/Ristretto, HDKD, BIP39) (paritytech#1795)
f3d135c 
* Rijig to Ristretto

* Rebuild wasm

* adds compatibility test with the wasm module

* Add Ed25519-BIP39 support

* Bump subkey version

* Update CLI output

* New keys.

* Standard phrase/password/path keys.

* Subkey uses S-URI for secrets

* Move everything to use new HDKD crypto.

* Test fixes

* Ignore old test vector.

* fix the ^^ old test vector.

* Fix tests

* Test fixes

* Cleanups

* Fix broken key conversion logic in grandpa

CC @rphmeier

* Remove legacy Keyring usage

* Traitify `Pair`

* Replace Ed25519AuthorityId with ed25519::Public

* Expunge Ed25519AuthorityId type!

* Replace Sr25519AuthorityId with sr25519::Public

* Remove dodgy crypto type-punning conversions

* Fix some tests

* Avoid trait

* Deduplicate DeriveJunction string decode

* Remove cruft code

* Fix test

* Minor removals

* Build fix

* Subkey supports sign and verify

* Inspect works for public key URIs

* Remove more crypto type-punning

* Fix typo

* Fix tests
This was referenced Jun 12, 2019
Closed
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rphmeier rphmeier rphmeier left review comments

@tomusdrw tomusdrw tomusdrw approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@gavofyork @pepyakin @burdges @jacogr @gguoss @kianenigma @tomusdrw @rphmeier @devops-parity