[keys] Improve the API of DerivableKey

[afilini]

Description

A new ExtendedKey type has been added, which is basically an enum of bip32::ExtendedPubKey and bip32::ExtendedPrivKey, with some extra metadata regarding the ScriptContext.

This type has some methods that make it very easy to extract its content as either an xprv or xpub.

The DerivableKey trait has been updated so that the user now only has to implement a method (DerivableKey::into_extended_key()) to perform the conversion into an ExtendedKey.

The method that was previously called add_metadata() has now been renamed to into_descriptor_key(), and it has a blanket implementation.

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature
• I've updated CHANGELOG.md

Bugfixes:

• This pull request breaks the existing API

[notmandatory]

This looks like a nice improvement, I'll take a stab at fixing the tests and updating #272 .

[notmandatory]

Is there a proper way to suggest a commit for a PR? Anyway I created notmandatory@65ba408 in my repo from a clone of your PR branch. It fixes the unit tests that are failing with --features all-keys. If it looks OK maybe you can pull it in, or just re-implement the fixes in a new commit if you want to make any changes.

I also changed the default network to Testnet in Seed.into_extended_key() but I'm not sure if it matters either way.

[afilini]

I don't think you can suggest an entire commit but you can suggest changes when adding comments to the code, and GitHub gives you an option to then commit those changes directly (though I don't really like it because making commits from the web UI means that they sign it with their GPG key)

I also changed the default network to Testnet in Seed.into_extended_key() but I'm not sure if it matters either way.

Yeah this shouldn't really matter, because we have our own way of keeping track of which networks are valid for a key.. we just need something there as a "placeholder", but all our traits like ToDescriptorKey and ToWalletDescriptor correctly look at our set of ValidNetworks.

And actually that was the bug that was making my tests fail: I forgot to make a custom implementation for DerivableKey::to_descriptor_key() in some of the BIP39 types, which meant that it was not overriding like it should the set of valid networks, like I'm correctly doing here:

bdk/src/keys/bip39.rs

Lines 55 to 62 in 6a4b7bb

	let descriptor_key = self
	.into_extended_key()?
	.into_descriptor_key(source, derivation_path)?;
	// here we must choose one network to build the xpub, but since the bip39 standard doesn't
	// encode the network, the xpub we create is actually valid everywhere. so we override the
	// valid networks with `any_network()`.
	Ok(descriptor_key.override_valid_networks(any_network()))

So the fix in this case would be to add the same code to the other BIP39 types as well, but I don't like how verbose it gets. I'll try to see if I can improve this with some Rust magic, and also try to document it better

[notmandatory]

need to add ``` between line 476 and 477

[afilini]

I was getting errors when adding that so I assumed they were being added automatically. I tried building the docs locally and the code snippet was looking good.

[notmandatory]

With 1.48.0 tool chain this test also passed for me. But I was only able to get the doc tests to work with 1.49.0 if I added the first ``` on the same line as the opening r##" and then didn't seem to matter where i put the closing ``` as long as it was before the closing "##

Can you verify your default toolchain is stable 1.49.0 ?

[afilini]

Ah yeah, it looks like I was also using 1.48 locally

[afilini]

Getting these errors on 1.49.0:

failures:

---- src/keys/mod.rs - keys::DerivableKey::into_extended_key (line 461) stdout ----
error: unknown start of token: `
  --> src/keys/mod.rs:475:5
   |
16 |     ```
   |     ^
   |
help: Unicode character '`' (Grave Accent) looks like ''' (Single Quote), but it is not
   |
16 |     '``
   |     ^

error: unknown start of token: `
  --> src/keys/mod.rs:475:6
   |
16 |     ```
   |      ^
   |
help: Unicode character '`' (Grave Accent) looks like ''' (Single Quote), but it is not
   |
16 |     `'`
   |      ^

error: unknown start of token: `
  --> src/keys/mod.rs:475:7
   |
16 |     ```
   |       ^
   |
help: Unicode character '`' (Grave Accent) looks like ''' (Single Quote), but it is not
   |
16 |     ``'
   |       ^

error: aborting due to 3 previous errors

Couldn't compile the test.

failures:
    src/keys/mod.rs - keys::DerivableKey::into_extended_key (line 461)

[afilini]

Ah okay, the solution is to remove the tab before every line. I think it considers it code if there's a tab before the line

[notmandatory]

I noticed while trying to handle error results for GeneratableKey<Ctx> for Mnemonic that it uses Error = Option<bip39::ErrorKind> which you had to err_map and downcast from an anyhow Error to an Option<bip39::ErrorKind>. In my test bdk-cli code I then need to err_map and unwrap these errors to turn them into a bdk::error::KeyError so I can handle it cleanly with other bdk::error::Error based results.

I don't see a way to make this better in bdk except possibly to use thiserror to add the std::error::Error trait to all the bdk error types. Then the bdk-cli handler functions can also use std::error::Error in their results from bdk or the bip39 lib calls. It's something I'm going to try separately and will do a PR if it looks OK.

I also think the tiny-bip39 library kind of messed up and should only be using bip39:ErrorKind errors (enhanced with thiserror) in their results. My understanding is anyhow errors aren't meant to be returned from library code. I'll open an issue with tiny-bip39 and see what they think. 🤔

[notmandatory]

ACK ccbbad3

I verified docs and cfg_attr doc look good and new API work in the bdk-cli PR I'm working on to add commands to generate a new mnemonic and master xkeys and restore master xkeys from mnemonic.

[tcharding]

Networks (mainnet etc) are fast becoming one of my least liked parts of cryptocurrency software, code that deals with them is almost always contrived. It would be nice to improve this valid network stuff at some stage if we can ...

[tcharding]

Suggested change

	let (mnemonic, passphrase) = self;
	let seed = Seed::new(&mnemonic, passphrase.as_deref().unwrap_or(""));
	let (mnemonic, passphrase) = self;
	let passphrase = passphrase.unwrap_or_default();
	let seed = Seed::new(&mnemonic, &passphrase);

[tcharding]

Same as above if you like the suggestion.

[tcharding]

Might be slightly nicer to use matches!, depending on your point of view.

[notmandatory]

Networks (mainnet etc) are fast becoming one of my least liked parts of cryptocurrency software, code that deals with them is almost always contrived. It would be nice to improve this valid network stuff at some stage if we can ...

Do you mean you'd like to avoid validating against specific blockchains such as Bitcoin or just make it easier to expand the possibly instances of the Bitcoin blockchain such as to support new custom signet networks?

I do agree it would be nice to be able to easily use a custom genesis block for testing against custom signet networks which core and some other projects are beginning to use.

[tcharding]

Do you mean you'd like to avoid validating against specific blockchains such as Bitcoin or just make it easier to expand the possibly instances of the Bitcoin blockchain such as to support new custom signet networks?

I'm not well versed enough in the bdk codebase to be making any claims at the moment :) I was merely commenting that including the network in code seems to always lead to contrived code. My personal opinion is that software shouldn't care or know which network it is attached too. If the point of testnets is to test the code then there should be no difference in the code irrespective of the network, if the code is different depending on the network then we are not testing the same code that would run when connected to mainnet. I'm only 18 months deep in cryptocurrency software so don't put too much weight in my opinions but this came up last week in my day job too (and it happened in my last job too) so when I saw it again so soon in bdk I felt like commenting :)

[afilini]

I think the main point against reusing the same encoding for everything is to make sure users don't accidentally mix up addresses/keys/etc.

One thing I'd like to have is an extended key format that has a set of valid networks, or at least doesn't include one at all: most of the "hacks" we have to do in BDK come from the fact that the "base type" we reduce everything to is ExtendedPrivKey (or ExtendedPubKey) and they both contain a single network.

So when you take a different key type, like a mnemonic, and try to convert it to the "base type" you are forced to pick one single network, which means that you have to use a random one as a placeholder but also remember that the original key type didn't encode any network information, so while the key inside says that it's i.e. a Testnet key, it should also be considered valid on Mainnet.

Also it doesn't help that different key formats encode different information inside: some don't encode anything at all, just the key, (BIP39), some encode the network but not the type of script (xprv/tprv), others include whether the script is legacy or segwit as well (Electrum mnemonics).