bitcoindevkit / bdk-cli

A CLI wallet library and REPL tool to demo and test the BDK library
Other
108 stars 64 forks source link

extended bdk-cli key derive to output xprv #25

Closed i5hi closed 3 years ago

i5hi commented 3 years ago

Output:

{
  "xprv": "tprv8ZgxMBicQKsPeCmJHoy1pwYwgTyWz2WG1W9MAuqwTFbj7AtiPVdy1C6oZJFMLMyZskVCXsf9XcG2mSVm9twaKkqhKXCh96fbuaPUxBz5Sr4/84'/1'/0'/*",
  "xpub": "[33b0970f/84'/1'/0']tpubDCyioiPrh9dHkBdomkQhvTFeFnKec2XU9a3BM4Z9Y4vVXGWjMj9je3dcV7HjqN21PMH8M98ejEEQwoof8FE4JkD1jqKJhxFwJAxYtAgeieN/*"
}

I've been using the same format for xprv as xpub [fingerprint/hardened/path]xprv/*, not sure how to get the xprv in that format - or if it is required.

i5hi commented 3 years ago

Sorry, this is wrong. Please ignore. Will push an update.

i5hi commented 3 years ago

As you said, the derivation path isnt being separated now, so the the resulting outputs now show the entire path within the prefix [ ] as below:

{
  "xprv": "[566844c5/84'/1'/0'/0]tprv8ixoZoiRo3LDHENAysmxxFaf6nhmnNA582inQFbtWdPMivf7B7pjuYBQVuLC5bkM7tJZEDbfoivENsGZPBnQg1n52Kuc1P8X2Ei3XJuJX7c/*",
  "xpub": "[566844c5/84'/1'/0'/0]tpubDFeqiDkfwR1tAhPxsXSZMfEmfpDhwhLyhLKZgmeBvuBkZQusoWeL62oGg2oTNGcENeKdwuGepAB85eMvyLemabYe9PSqv6cr5mFXktHc3Ka/*"
}
notmandatory commented 3 years ago

As you said, the derivation path isnt being separated now, so the the resulting outputs now show the entire path within the prefix [ ] as below:

Ya I'm not sure if this is a bug or a feature. Is this OK for your use case?

Here's a way to fix it (borrowed some code from rust-miniscript) , may be what most users would expect:

            let deriv_path = DerivationPath::from_str(path.as_str())?;

            let deriv_path_len = (&deriv_path).as_ref().len();
            let normal_suffix_len = (&deriv_path)
                .into_iter()
                .rev()
                .take_while(|c| c.is_normal())
                .count();

            let deriv_path_hardened = DerivationPath::from(&deriv_path[..(deriv_path_len - normal_suffix_len)]);
            let deriv_path_normal = DerivationPath::from(&deriv_path[(deriv_path_len - normal_suffix_len)..]);

            let derived_xprv = &xprv.derive_priv(&secp, &deriv_path_hardened)?;
            let origin:KeySource = (xprv.fingerprint(&secp), deriv_path_hardened);

            let derived_xprv_desc_key: DescriptorKey<Segwitv0> =
                derived_xprv.into_descriptor_key(Some(origin), deriv_path_normal)?;

            if let Secret(desc_seckey, _, _) = derived_xprv_desc_key {
                let desc_pubkey = desc_seckey.as_public(&secp).unwrap();
                Ok(json!({"xpub": desc_pubkey.to_string(), "xprv": desc_seckey.to_string()}))
            } else {
                Err(Error::Key(Message("Invalid key variant".to_string())))
            }
notmandatory commented 3 years ago

One final thing, when you're all done be sure to run cargo fmt, it looks like that check is failing.

i5hi commented 3 years ago

I think it should be fine for now. I tried running the code you shared and changed the tests but they are failing. I'll spend a little more time with it and get back. Might be better to display it the most commonly expected format.

notmandatory commented 3 years ago

I think it is better to split the path and then the original test should pass without any changes except to also verify the xprv.

notmandatory commented 3 years ago

@vmenond can you also setup gpg signing for your commits? It's not a big deal but it is best practice and good thing to have for other projects you're working on. Github provides good instructions for signing commits. And there's also a blog post on how to retroactively sign git commits. I'm happy to help if you run into any problems.

i5hi commented 3 years ago

@notmandatory surely! I will get on this next session and push a final commit with a signature. been a bit caught up with work, hence the delayed responses.

i5hi commented 3 years ago

GOT IT! Small typo.

let derived_xprv = &xprv.derive_priv(&secp, &deriv_path_hardened)?;

TO

let derived_xprv = &xprv.derive_priv(&secp, &deriv_path)?;

All tests passing. Also double checked at https://bip32jp.github.io/english/

i5hi commented 3 years ago

Sorry about the messy commit history. Had a few issues getting a valid signature.

Commit 078b75d should be all good now.

thunderbiscuit commented 3 years ago

I'm trying to understand the workflow for this. Does the user provide the path as a new argument? Could you post an example of the use of the command? Cheers!

Edit: Oh wait. I might have misunderstood the PR. Is this only about the format of the returned json-ish object printed by bdk-cli? The first post jumps right into it, it feels like I might have missed part of the conversation that might have happened elsewhere and lead to this PR.

i5hi commented 3 years ago

the command used to originally only output the child xpub.

If you run bdk-cli key help, you will notice

derive      Derive an xpub from an xprv and a derivation path string (ie. "m/84'/1'/0'/0")

Which also needs an update

For the given input:

cargo run  key derive --path m/84h/0h/1h --xprv tprv8ZgxMBicQKsPdZn4NNm7gjMMnLxq443NoZYPZ4a46wC7ZE9Q6EMTudm6az7HYW37uWGgJ428cmgg6E16n28Scn8hfCPCr9po5j6FcNboXFY

Old output:

{  "xpub": "[94430292/84'/0'/1']tpubDCgN8DyJc9eKe7oncvGeXUmFPmUUKYykXvhNvEV8gjSQjgASPUbeX6A37NoqGYt2bDWDM1Kz3HfEdRAJpUCM9JvNmH8HxKN8eh87LN5DdER/*"

This part of the PR was discussed over Discord. Then, after making the change to output the xprv as well, this thread started with the first post - unable to get the child key pair to output in the same descriptor format.

New output:

{
  "xprv": "[94430292/84'/0'/1']tprv8fzKyow4TmxekemzjGc48578pjxYADnqxd6bdiSqGTe1uBufm5n4LbYAwGZJ68GmMKH9qTUbgCwTK8EyALCe2m4KGG1sPxQsFE5zLbACr71/*",
  "xpub": "[94430292/84'/0'/1']tpubDCgN8DyJc9eKe7oncvGeXUmFPmUUKYykXvhNvEV8gjSQjgASPUbeX6A37NoqGYt2bDWDM1Kz3HfEdRAJpUCM9JvNmH8HxKN8eh87LN5DdER/*"
}
thunderbiscuit commented 3 years ago

Oh nice! The new derive subcommand with the --path argument totally slipped by me. I didn't know it had been added.

Great stuff. 🔥

notmandatory commented 3 years ago

Hi thanks for getting everything signed! I'm traveling but will be able to re-review when I'm back in a few days.

notmandatory commented 3 years ago

I took a look and would like to discuss pro/con of not splitting the hardened/non-hardened path. Looks like you went with just deriving from the whole path including any non-hardened parts at the end. But I think most users would expect to just derive up to the last hardened part of the path with the unhardened part of the path added after the derived key part. This way also the test remains the same for the xpub with the addition of an assert for the xprv.

I have a code suggestion https://github.com/bitcoindevkit/bdk-cli/pull/25#issuecomment-829588162 for how to do it. Also I'd like to help you do an interactive rebase to cleanup your commit history. We can chat on Discord about how to do it if you like.

i5hi commented 3 years ago

@notmandatory Made changes based on your suggestions.

Should be good to go now.