Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for wallet import #12

Closed
tnull opened this issue Sep 1, 2022 · 7 comments
Closed

Allow for wallet import #12

tnull opened this issue Sep 1, 2022 · 7 comments

Comments

@tnull
Copy link
Collaborator

tnull commented Sep 1, 2022

Currently we setup the underlying BDK/LDK wallets from a single random seed file.

We should however allow to import pre-existing on-chain wallets, e.g., from a Bip39 mnemonic.

@tnull tnull changed the title Allow for wallet import Enable wallet import Sep 1, 2022
@tnull tnull changed the title Enable wallet import Allow for wallet import Sep 1, 2022
@thunderbiscuit
Copy link

thunderbiscuit commented Sep 5, 2022

Hey @tnull!

I work on the BDK language bindings and I'm looking to understand better what would be a recommended way/location to source the entropy we provide to the LDK library, potentially to expose that in the bindings if the users need it.

I read what LdkLite is doing at the moment for the LDK entropy, and have questions regarding this approach (mainly because I want to enable mirroring this approach with the BDK bindings if users need to):

  1. What is the general idea for backing that entropy? Are you thinking of using BIP-39 mnemonics the way I think Sensei does?
  2. If the entropy used is the one generated prior to creating the BIP32 seed and you wish to back that entropy up through the use of BIP-39 words, then
    a. it must have exactly 24 words if you want to generate 32 bytes of entropy.
    b. it cannot contain any passphrase, the way BIP-39 allows.
    And so it feels to me like this is using BIP39 words, but not really the full BIP39 system (a sort of half-BIP39, which might lead to confusion). A user that knows what is going on won't have any troubles with this, but at the same time standardization of wallets is always an issue. If you also want to use that entropy to create the onchain wallet, what you end up with is the initial entropy being used in two very similar but also separate settings:
|         |               | 2048 rounds of          |                     | On-chain   |
|         | --- BIP39 --> | HMAC-SHA512             | --- BIP32 Seed ---> |   BDK      |
|         |               | (mnemonic + passphrase) |                     | descriptor |
| entropy |
|         |                |     |
|         | --- BIP39- --> | LDK | 
|         |     like       |     |
  1. One of the way these two uses of the entropy could be streamlined would be by giving LDK entropy that comes from inside the BIP32 tree used to generate the onchain descriptor (this would allow your "backup system" for LDK to be the same as your on-chain wallet, potentially 12/15/21/24 words and a passphrase). It could either be the actual root key/seed (the seed is actually 64 bytes but only the first 32 could be used), or it could be any entropy generated at any node/child in the tree (as long as it was standard/defined).

Overall, my question boils down to this:
Would it be possible for LDK to define a "best practice" or "standard LDK location" for the source of that entropy? If it is indeed the way described above that's totally fine, but I want to make sure we're following the recommended LDK approach if we work on enabling this in our bindings. From my perspective as an application dev who has worked on building an LDK node in Kotlin, the LDK approach of "bring your own entropy" is pretty cool because I can do what I want, but also very scary: I don't want to make a mistake and put people's funds at risk. I think it would be awesome if LDK could provide an "LDK approved" way to source/use/and backup the entropy we provide to LDK. Let me know if there is a better place to surface this question, as I understand it's more than just an LdkLite question.

Cheers!

@tnull
Copy link
Collaborator Author

tnull commented Sep 6, 2022

Hey @thunderbiscuit!

Thanks for outlining the different scenarios. I imagine LDK itself will keep relatively agnostic to any particular entropy source and just accept any [u8; 32] to derive its keys. However, I agree we should definitely expose some recommended ways of backing up/restoring the entropy and/or wallet descriptors in LdkLite.

I currently tend towards what you described under 3.: derive LDK's entropy from the same Bip32 seed as BDK, and offer a number of different ways in the API how to restore this Bip32 seed (e.g., simply providing the raw bytes, a String of Bip39 words, etc.). IIUC what you propose, wouldn't taking the first 32 bytes of the seed be the same as simply using the master secret key, i.e., SecretKey::secret_bytes() out of the bip32::ExtendedPrivKey?

@tnull
Copy link
Collaborator Author

tnull commented Sep 6, 2022

@thunderbiscuit Just added a first draft of this over at #14.

@thunderbiscuit
Copy link

thunderbiscuit commented Sep 7, 2022

Hey that sounds great! I'll keep an eye on this, and we'll make sure that users of bdk-ffi are offered the ability to derive the same entropy LDK uses for LdkLite (I hadn't thought of this at first but this will also ensure devs can migrate between the two LDKs (say you start with LdkLite and after a while want to migrate to ldk-java, you'll need to be able to recover the same wallets and all).

As for the question: wouldn't taking the first 32 bytes of the seed be the same as simply using the master secret key, i.e., SecretKey::secret_bytes() out of the bip32::ExtendedPrivKey?, I took some time to test some stuff out and re-read the BIP to try and make sure I understand correctly, and I think in fact those are not the same (but to be clear both of those would work perfectly). If you take a look at the second diagram on this page (see below), I think what the seed[0..32] would be what I identified at (1) below and what the SecretKey::secret_bytes() would be (2).

seed

I'm not 100% percent sure on this, but also in my local testing the two do not equate each other (using your code in #14)

let xprv = bitcoin::util::bip32::ExtendedPrivKey::new_master(config.network, &seed_bytes)?;
let ldk_seed: [u8; 32] = xprv.private_key.secret_bytes();

I think what is happening is that this xprv.private_key.secret_bytes() is actually the first child (child 0) derived from this seed. The seed does not seem to count as child 0. The ExtendedPrivKey::new_master() takes the seed and passes it through the HMAC before pulling the private key and chaincode out of it.

impl ExtendedPrivKey {
    /// Construct a new master key from a seed value
    pub fn new_master(network: Network, seed: &[u8]) -> Result<ExtendedPrivKey, Error> {
        let mut hmac_engine: HmacEngine<sha512::Hash> = HmacEngine::new(b"Bitcoin seed");
        hmac_engine.input(seed);
        let hmac_result: Hmac<sha512::Hash> = Hmac::from_engine(hmac_engine);

        Ok(ExtendedPrivKey {
            network,
            depth: 0,
            parent_fingerprint: Default::default(),
            child_number: ChildNumber::from_normal_idx(0)?,
            private_key: secp256k1::SecretKey::from_slice(&hmac_result[..32])?,
            chain_code: ChainCode::from(&hmac_result[32..]),
        })
    }

Sorry this is getting a bit long. In short, I think they are different, but I also think that using the private key is just as good, and it's easier to document than "take the first 32 bytes out of the seed bla bla bla".

@tnull
Copy link
Collaborator Author

tnull commented Sep 7, 2022

Great, thank you for the explanation! I agree they are different, but essentially only one HMAC away. 😆
So I think if we don't find a good argument why the two seeds should be identical, using the derived one for LDK is probably fine.

@thunderbiscuit
Copy link

Absolutely! They don't need to be the same at all, I was just curious because initially in my mind they were but I was wrong.

@tnull
Copy link
Collaborator Author

tnull commented Mar 24, 2023

Generally done in #11, Bip39 will follow (see #47).

@tnull tnull closed this as completed Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants