Implement Bip32 for seed-phrase/passphrase signing #16942

CriesofCarrots · 2021-04-29T07:49:20Z

Problem

Solana non-remote-wallet signers do not support bip32 hierarchical derivation, preventing a lot of interoperability between client tools like solana-cli and 3rd-party wallets.

Summary of Changes

Implement solana bip44 derivation for SignerSourceKind::Ask signers

Toward #5246

Todo:

Implement bip44 solana derivations
Implement query pair for absolute path derivations
Add documentation
~~Consider whether to enable any aspect of this for solana-keygen recover~~ not now, reconsider in future

Follow-up work:

Rework Keypair serialized format & solana-keygen to store chain code, and support bip32 for File and Stdin signing sources

Milerius · 2021-04-30T15:47:41Z

Problem

Solana non-remote-wallet signers do not support bip32 hierarchical derivation, preventing a lot of interoperability between client tools like solana-cli and 3rd-party wallets.

Summary of Changes
* Implement solana bip44 derivation for `SignerSourceKind::Ask` signers
Toward #5246

Todo:
* [x]  Implement bip44 solana derivations

* [ ]  Implement query pair for absolute path derivations

* [ ]  Add documentation

* [ ]  Consider whether to enable any aspect of this for `solana-keygen recover`
Follow-up work:
* Rework Keypair serialized format & `solana-keygen` to store chain code, and support bip32 for File and Stdin signing sources

Excellent until now i was forced to do it this way:

use bip39::core::str::FromStr;
use bip39;
use ed25519_dalek_bip32::ExtendedSecretKey;
use solana_sdk::signature::Signer;

fn main() {
    let derivation_path = ed25519_dalek_bip32::DerivationPath::from_str("m/44'/501'/0'").unwrap();

    let mnemonic = "shoot island position soft burden budget tooth cruel issue economy destroy above";
    let res = bip39::Mnemonic::from_str(mnemonic).unwrap();
    let seed = res.to_seed("");

    let ext = ExtendedSecretKey::from_seed(&seed).unwrap().derive(&derivation_path).unwrap();
    let ref priv_key = ext.secret_key;
    let pub_key = ext.public_key();

    println!("derivation_path: {}", derivation_path);
    println!("priv_key: {:?}", priv_key);
    println!("priv_key len: {:?}", priv_key.as_bytes().len());

    let public_address = bitcoin::util::base58::encode_slice(pub_key.as_bytes());
    println!("public_address: {}", public_address);
    assert_eq!(public_address.len(), 44);
    assert_eq!(public_address, "2bUBiBNZyD29gP1oV6de7nxowMLoDBtopMMTGgMvjG5m");

    let private_key = bitcoin::util::base58::encode_slice(priv_key.as_bytes());
    println!("private key: {}", private_key);

    let pair = ed25519_dalek::Keypair{secret: ext.secret_key, public: pub_key};

    let fin = solana_sdk::signature::keypair_from_seed(pair.to_bytes().as_ref()).unwrap();
    println!("pub_key: {}", fin.pubkey().to_string());
    println!("from_sdk_priv: {}", bitcoin::util::base58::encode_slice(fin.secret().as_bytes()));
}

Looking forward to use the new functions to simplify my approach.

CriesofCarrots · 2021-04-30T22:07:52Z

@t-nelson , do you mind starting to look at this code while I work up some documentation?

sdk/src/derivation_path.rs

CriesofCarrots · 2021-04-30T22:37:11Z

sdk/src/signature.rs

+    if let Some(derivation_path) = derivation_path {
+        bip32_derived_keypair(seed, derivation_path).map_err(|err| err.to_string().into())
+    } else {
+        keypair_from_seed(seed)
+    }


Here is the if clause I mentioned to retain compatibility with current keypair creation. Worth considering if we need to do this. We could transition all creation-from-seed to bip32_derived_keypair with derivation_path of length 0

codecov · 2021-05-01T00:03:28Z

Codecov Report

Merging #16942 (0a7bb48) into master (7cea2c4) will decrease coverage by 0.0%.
The diff coverage is 79.7%.

@@            Coverage Diff            @@
##           master   #16942     +/-   ##
=========================================
- Coverage    82.8%    82.8%   -0.1%     
=========================================
  Files         416      416             
  Lines      117032   117189    +157     
=========================================
+ Hits        96928    97047    +119     
- Misses      20104    20142     +38

t-nelson

Looking good! Just a couple nits so far. Let me know when it's ready for another pass

sdk/src/derivation_path.rs

CriesofCarrots · 2021-05-03T22:45:31Z

@t-nelson ready for another look!

t-nelson

lgtm. Thanks for driving this home!

* Add Keypair helpers for bip32 derivation * Plumb bip32 for SignerSourceKind::Ask * Support full-path querystring * Use as_ref * Add public wrappers for from_uri cases * Support master root derivations (and fix too-deep print * Add ask:// HD documentation * Update ASK elsewhere in docs (cherry picked from commit 694c674) # Conflicts: # programs/bpf/Cargo.lock

…17018) * Implement Bip32 for seed-phrase/passphrase signing (#16942) * Add Keypair helpers for bip32 derivation * Plumb bip32 for SignerSourceKind::Ask * Support full-path querystring * Use as_ref * Add public wrappers for from_uri cases * Support master root derivations (and fix too-deep print * Add ask:// HD documentation * Update ASK elsewhere in docs (cherry picked from commit 694c674) # Conflicts: # programs/bpf/Cargo.lock * Fix conflict Co-authored-by: Tyera Eulberg <teulberg@gmail.com> Co-authored-by: Tyera Eulberg <tyera@solana.com>

garious · 2021-05-04T16:39:59Z

docs/src/cli/conventions.md


 To display the wallet address of a Paper Wallet:

 ```bash
-solana-keygen pubkey ASK
+solana-keygen pubkey ask://


Do these generate the same address? At a glance, I'd guess that ask:// would generate the key at ASK/44'/501'.

Yes, these do generate the same address. Have you read this section yet? https://github.com/solana-labs/solana/pull/16942/files#diff-74038ef7d737ad7ebedf6ddb1f940e6f15337de6cba7d331a3621cccdd81cd3aR123
Definitely would love suggestions to make docs more clear.

I'm confused, do I have to write ask://?full-path=""? As I recall, usb:// gets the Solana-specific address at 44'/501'. I don't see why ask:// wouldn't do the same (which is different than what ASK would have output).

No, you would write ask://?key=m to use solana's bip44 derivation path root, 44'/501'.
Now that we're depending on the derivation-path crate, usb://ledger?key=m actually also derives the solana bip44 root, although of course it's easier to just not supply the query string at all.

I can see the argument for making ask:// no query behave the same as usb:// no query, but I was concerned about retaining compatibility for current seed phrase users. They would otherwise have to continue using ASK or we would have to build in a special query string for ask://, since the current output of ASK is a key that is not anywhere in the bip32 derivation.
cc @t-nelson

We're going to work toward documenting everything in URI form going forward. ask: (I'm somewhat dubious as to whether we should support the network prefix ://) is a direct replacement for ASK, which should be considered deprecated. usb:// is overloaded in that it implicitly derives BIP44 keypairs. This is the bug IMO, but we keep the special-case for backward compatibility. So: ask: gives the raw ed25519 keypair, ask:?key=X[/Y] gives the BIP44 account[/change] address, ask:?full-path=m/X[/Y[/Z[/...]]] gives the corresponding BIP32 address. This is consistent across all other schemes

How about:

ask:// means ASK, does not accept a query string, and does not imply any key derivation

ask://solana is an alias for ask://?prefix=44'/501'

usb://ledger is an alias for usb://ledger?prefix=44'/501'

Also, how about renaming ask:// to seedphrase:// or stdin://seedphrase?

@CriesofCarrots, wdyt?

@garious I rather like stdin://seedphrase. We do already have a stdin scheme implementation for piping in json keypair (same as passing -). What host do you like for that?

For seedphrase, I would envision:

stdin://seedphrase => ASK (raw ed25519 keypair)

stdin://seedphrase?key=1/2 => solana bip44 m/44/501/1/2

stdin://seedphrase?full-path=m/44/2017/1/2

Or would you want to use the solana alias as well?

Hmm, I don't care for 'full-path' because it conflicts with 'key' (unless 'key' always appends, in which case 'full-path' is not the "full" path). A 'prefix' parameter that defaults to "m/44'/501'" seems more intuitive to me. To get ASK, you can then write "stdin://seedphrase?prefix=''"

* Add Keypair helpers for bip32 derivation * Plumb bip32 for SignerSourceKind::Ask * Support full-path querystring * Use as_ref * Add public wrappers for from_uri cases * Support master root derivations (and fix too-deep print * Add ask:// HD documentation * Update ASK elsewhere in docs

CriesofCarrots force-pushed the bip32-ask branch from b3ae9af to 46c3096 Compare April 29, 2021 08:03

CriesofCarrots force-pushed the bip32-ask branch from 46c3096 to 09c51e7 Compare April 30, 2021 22:06

CriesofCarrots commented Apr 30, 2021

View reviewed changes

sdk/src/derivation_path.rs Show resolved Hide resolved

CriesofCarrots force-pushed the bip32-ask branch from 09c51e7 to f42737f Compare April 30, 2021 22:29

CriesofCarrots commented Apr 30, 2021

View reviewed changes

t-nelson reviewed May 1, 2021

View reviewed changes

sdk/src/derivation_path.rs Outdated Show resolved Hide resolved

sdk/src/derivation_path.rs Show resolved Hide resolved

sdk/src/derivation_path.rs Outdated Show resolved Hide resolved

sdk/src/derivation_path.rs Show resolved Hide resolved

Tyera Eulberg added 5 commits May 3, 2021 14:04

Add Keypair helpers for bip32 derivation

2929b26

Plumb bip32 for SignerSourceKind::Ask

29f01e2

Support full-path querystring

8359bb4

Use as_ref

2534b1d

Add public wrappers for from_uri cases

d8dbdf3

CriesofCarrots force-pushed the bip32-ask branch from 6b375d0 to d8dbdf3 Compare May 3, 2021 20:05

Tyera Eulberg added 3 commits May 3, 2021 15:27

Support master root derivations (and fix too-deep print

724467c

Add ask:// HD documentation

7f85d8c

Update ASK elsewhere in docs

0a7bb48

CriesofCarrots marked this pull request as ready for review May 3, 2021 22:43

CriesofCarrots added the v1.6 label May 3, 2021

t-nelson approved these changes May 4, 2021

View reviewed changes

CriesofCarrots merged commit 694c674 into solana-labs:master May 4, 2021

mergify bot mentioned this pull request May 4, 2021

Implement Bip32 for seed-phrase/passphrase signing (backport #16942) #17018

Merged

CriesofCarrots mentioned this pull request May 4, 2021

solana-keygen doesn't generate keys from keys (BIP32 support) #5246

Closed

garious reviewed May 4, 2021

View reviewed changes

CriesofCarrots deleted the bip32-ask branch May 10, 2021 20:09

brooksprumo mentioned this pull request Aug 23, 2021

backport 19361 v16 #19379

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Bip32 for seed-phrase/passphrase signing #16942

Implement Bip32 for seed-phrase/passphrase signing #16942

CriesofCarrots commented Apr 29, 2021 •

edited

Loading

Milerius commented Apr 30, 2021

Problem

Summary of Changes

CriesofCarrots commented Apr 30, 2021

CriesofCarrots Apr 30, 2021

codecov bot commented May 1, 2021 •

edited

Loading

t-nelson left a comment

CriesofCarrots commented May 3, 2021

t-nelson left a comment

garious May 4, 2021

CriesofCarrots May 4, 2021

garious May 4, 2021 •

edited

Loading

CriesofCarrots May 4, 2021

t-nelson May 4, 2021

garious May 4, 2021

garious May 5, 2021

CriesofCarrots May 6, 2021

garious May 6, 2021

Implement Bip32 for seed-phrase/passphrase signing #16942

Implement Bip32 for seed-phrase/passphrase signing #16942

Conversation

CriesofCarrots commented Apr 29, 2021 • edited Loading

Problem

Summary of Changes

Milerius commented Apr 30, 2021

Problem

Summary of Changes

CriesofCarrots commented Apr 30, 2021

Choose a reason for hiding this comment

codecov bot commented May 1, 2021 • edited Loading

Codecov Report

t-nelson left a comment

Choose a reason for hiding this comment

CriesofCarrots commented May 3, 2021

t-nelson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garious May 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CriesofCarrots commented Apr 29, 2021 •

edited

Loading

codecov bot commented May 1, 2021 •

edited

Loading

garious May 4, 2021 •

edited

Loading