Skip to content

Add API for generating master keys directly - #218

Closed
thunderbiscuit wants to merge 2 commits into
bitcoindevkit:masterfrom
thunderbiscuit:feat/generate-master-keys-directly
Closed

Add API for generating master keys directly#218
thunderbiscuit wants to merge 2 commits into
bitcoindevkit:masterfrom
thunderbiscuit:feat/generate-master-keys-directly

Conversation

@thunderbiscuit

@thunderbiscuit thunderbiscuit commented Oct 27, 2022

Copy link
Copy Markdown
Member

Rough draft of the new API for generating DescriptorSecretKey without going through the mnemonics step.

A few questions still left to figure out:

  1. Do we want the generate_from_entropy() method? I added it as a bonus but we should still discuss if there is a need for it.
  2. I'd like to make sure the random number generation is handled properly; this is the very core of the security of the keys. I tried to reproduce what is done for the mnemonic, see the code here.
  3. I'm not sure if I really need to have the rand crate imported here (we don't need it on the mnemonic step for example). There might be a way to get this done internally in bdk without generating the entropy at the ffi layer.

@thunderbiscuit
thunderbiscuit force-pushed the feat/generate-master-keys-directly branch from 504671d to be2eec1 Compare October 27, 2022 15:35
@thunderbiscuit thunderbiscuit self-assigned this Oct 27, 2022
@thunderbiscuit thunderbiscuit added this to the Release 0.11.0 milestone Oct 27, 2022
This API allows developers to build master extended keys directly,
without going through the mnemonics step
@thunderbiscuit
thunderbiscuit force-pushed the feat/generate-master-keys-directly branch from be2eec1 to d8cdad2 Compare October 27, 2022 15:42
@thunderbiscuit

Copy link
Copy Markdown
Member Author

Would be happy to hear initial feedback from @kirillzh and @jesseposner. Does this address your need? Did you have another API shape in mind?

In Kotlin the call site currently looks like this:

val bip32RootKey: DescriptorSecretKey = DescriptorSecretKey.generate(Network.TESTNET)
val bip84ExtendedKey: DescriptorSecretKey = bip32RootKey.extend(DerivationPath("m/84h/1h/0h/0"))
val descriptor = "wpkh(${bip84ExtendedKey.asString()})"
println(descriptor)
// wpkh(tprv8ZgxMBicQKsPeSUx4F4fC6Vc4rWBxfJV1FVBWWLxqapQpSdZ2huNuhcRUdWPjNFbQmQ285bjXV7jayWU7RyL5XSNMSC5J6wf6gj1RCCh43G/84'/1'/0'/0/*)

Comment thread src/bdk.udl Outdated
@thunderbiscuit
thunderbiscuit force-pushed the feat/generate-master-keys-directly branch 2 times, most recently from a87c0f4 to 7b12b60 Compare October 27, 2022 17:55
@thunderbiscuit

thunderbiscuit commented Oct 27, 2022

Copy link
Copy Markdown
Member Author

I wonder if the names new_master() and new_master_from_entropy() are maybe more explicit than my current generate() and generate_from_entropy(), partly because they are constructors on the DescriptorSecretKey type, but that type can have a lot more than just master keys. new_master() makes it clear that you're generating master keys.

We don't have a direct match for these methods in BDK on the DescriptorSecretKey type (there is a bit more complexity on the pure Rust side than we want to expose in the bindings), but the method we actually use to create the keys is in rust-bitcoin and is called ExtendedPrivKey::new_master(), and so I think using that name would be fair. Happy to hear what others think about this.

@notmandatory notmandatory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ACK 7b12b60

Looks good!

@thunderbiscuit
thunderbiscuit force-pushed the feat/generate-master-keys-directly branch from 7b12b60 to 4270f9c Compare October 27, 2022 19:27
Comment thread src/lib.rs Outdated
Balance as BdkBalance, BlockTime, Error as BdkError, FeeRate, KeychainKind, SignOptions,
SyncOptions as BdkSyncOptions, Wallet as BdkWallet,
};
use rand::{thread_rng, Rng};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it be okay to use thread_rng, Rng this way?

use bdk::bitcoin::secp256k1::rand::{thread_rng, Rng};

This way we don't have to add rand to Cargo.toml

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Oh good idea. @notmandatory do I understand correctly that if this is coming from re-exports it's good to go? I think in this case bdk re-exports bitcoin which re-exports secp256k1 which re-exports rand... lol.

@notmandatory notmandatory Oct 30, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The latest version of bdk (0.24) updated rand version to 0.8, and I confirmed rand is exported by the secp256k1 lib as @dhruv-1001 shows above should work, and we don't need to add rand as a dependency to bdk-ffi.

@dhruv-1001 dhruv-1001 mentioned this pull request Oct 28, 2022
5 tasks
@notmandatory

notmandatory commented Oct 30, 2022

Copy link
Copy Markdown
Member

I just took another look at how entropy and Mnemonics work in bdk and now I think the right way to go is to go ahead with a #219 but add Mnemonic.from_entropy() constructor function also. The advantage to using a full Mnemonic structure created from entropy, is it gives on-chain wallets created from some other entropy the option to still back it up with Mnemonic seed words. Also this means we'll stay closer to how the bdk API works.

This API allows developers to build master extended keys directly,
without going through the mnemonics step
@thunderbiscuit
thunderbiscuit force-pushed the feat/generate-master-keys-directly branch from 4270f9c to 869a193 Compare November 1, 2022 15:20
@jesseposner

Copy link
Copy Markdown

@notmandatory Does the bdk API recommend a mnemonic to generate an xprv? I thought something like this was okay:

    fn test_keys_generate_xprv() {
        let generated_xprv: GeneratedKey<_, miniscript::Segwitv0> =
            bip32::ExtendedPrivKey::generate_with_entropy_default(TEST_ENTROPY).unwrap();


        assert_eq!(generated_xprv.valid_networks, any_network());
        assert_eq!(generated_xprv.to_string(), "xprv9s21ZrQH143K4Xr1cJyqTvuL2FWR8eicgY9boWqMBv8MDVUZ65AXHnzBrK1nyomu6wdcabRgmGTaAKawvhAno1V5FowGpTLVx3jxzE5uk3Q");
    }

@notmandatory

notmandatory commented Nov 2, 2022

Copy link
Copy Markdown
Member

@notmandatory Does the bdk API recommend a mnemonic to generate an xprv? I thought something like this was okay:

@jesseposner , for the Rust lib you're correct that bdk supports more ways to generate your xprv, including without creating a Mnemonic first. But for the language bindings I guess I was thinking that always starting with a Mnemonic would be a reasonable simplification to cover both use cases of generating keys with or without backing up via a Mnemonic (ie. you don't need to save the Mnemonic words).

We could certainly also give users both options, to create SecretDescriptorKey from a Mnemonic as in #219 or directly with user provided entropy or generated entropy. Any ideas on why users may not want to create a mnemonic first, besides that if they're not going to use it then it is a bit more code and temporary memory allocation?

@jesseposner

Copy link
Copy Markdown

@notmandatory The only other reason not to create a mnemonic first is that a mnemonic is limited to 256 bits of entropy, whereas BIP32 allows for 512 bits of entropy. However, anything over 256 bits is overkill and BIP32 recommends 256 bits, so as a practical matter I don't think much is being lost with the 256 limit.

So the benefits of the additional flexibility of not requiring a mnemonic are pretty minor. It's nice to have but I also agree that it's a reasonable simplification to require a mnemonic.

@thunderbiscuit

Copy link
Copy Markdown
Member Author

So we've merged #219, which allows us to create entropy/mnemonics, or use custom entropy to generate those mnemonics, and for now we'll keep that mnemonic as a required input for the DescriptorSecretKey.

If you don't wish to store the mnemonic, the new call-site looks like this:

val descriptorSecretKey = DescriptorSecretKey(
    network = Network.TESTNET, 
    mnemonic = Mnemonic(WordCount.WORDS12),
    password = "",
)

// generate from custom entropy
val entropy: List<UByte> = listOf<UByte>(...)
val descriptorSecretKey = DescriptorSecretKey(
    network = Network.TESTNET, 
    mnemonic = Mnemonic.fromEntropy(entropy),
    password = "",
)

@thunderbiscuit thunderbiscuit mentioned this pull request Nov 10, 2022
@thunderbiscuit
thunderbiscuit deleted the feat/generate-master-keys-directly branch November 14, 2023 15:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants