Caravan export by notmandatory · Pull Request #594 · bitcoindevkit/bdk

notmandatory · 2022-04-29T05:00:06Z

Description

This is a new module for importing and exporting a wallet descriptor and network from and to a Caravan config JSON file.

Notes to the reviewers

I only have minimal happy path tests now. More negative test cases are needed.

Open issues:

What is the client.type field? are there enum values?
Is it OK to put the descriptor key hash in the .extendedPublicKeys[].name fields when exporting?
I'm only supporting "sortedmulti" inner expressions, are any others expected?
Change descriptors aren't supported but could be, need to confirm derivation path
Add tests for all example configs
Do private key wallets/vaults need to be supported? No.

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
I've added tests to reproduce the issue which are now passing
I'm linking the issue being fixed by this PR

notmandatory · 2022-04-29T05:03:45Z

@bucko13 here's a first cut at a import/export tool. Happy to walk through it with you some time. I have a few open questions, see above.

afilini

This looks very good! I've added a few minor comments, mostly suggestions to better use the rust language.

afilini · 2022-04-29T09:29:28Z

+
+/// A caravan client
+#[derive(Debug, Serialize, Deserialize)]
+pub struct CaravanClient {


I guess this could be made into an enum? Is there a list of valid values for the client?

I don't know what the values should be but this question is on my list.

So I think this is a good argument in favor of the generalized approach detailed in my comment where these details could/should be left up to the consumer of this import/export utility. In Caravan, the client can be public and then we just assume that points to blockstream.info. There's the private one which requires some access info for your node (host, rpcuser, rpcpass), but i don't think bdk needs to care about that.

afilini · 2022-04-29T09:31:46Z

+        name: String,
+        client_type: String,
+        network: Network,
+        descriptor: &Descriptor<DescriptorPublicKey>,


I think it would be better if you could take a reference to the wallet and get the descriptor yourself. It would also give you a chance to inspect the internal descriptor like we do for fully noded.

I'm guessing the caravan wallet only supports similar descriptors for the internal keychain

I also have it on my issues list to determine if Caravan can support change descriptors, and if so how. I expect all I need to do is change the derivation from m/0/* to m/1/* but need to confirm.

Caravan will create a change and a receive descriptor based on the settings imported into it. We call this data structure a "braid", since once it's not a string anymore it's not really a descriptor anymore. Basically nearly all wallets today contain two braids, one at index 0 and one at 1. I'd also like to support BIP88 style, but this should be done separate from this utility imo. i.e. in JS expand the single bip88 descriptor into two (or more?) descriptors and then feed that through bdk.

afilini · 2022-04-29T09:49:29Z

As a sidenote: the way this is coded it only supports importing/exporting public keys. Is this the goal of caravan, giving you a watch-only descriptor?

If you want to support private keys as well it get a bit more complicated, but still very doable!

notmandatory · 2022-04-29T16:31:33Z

Thanks for the suggestions! will get those fixed. AFAIK caravan only supports pub keys but will add that to my list of issues to get an answer on.

bucko13 · 2022-04-30T00:19:05Z

🎉 exciting to see progress on this!

One general comment that will probably impact the further development of this is that I think it might be easier, especially for long term maintenance, to keep this as general as possible and maybe not even Caravan specific at all. One of the primary things that I think is missing from the space is a way to effectively parse and encode descriptors in JavaScript based applications. And to do it in a way that is relying on the canonical implementations of the descriptor/miniscript tooling would be ideal so we know we have all the guarantees that the reference implementation(s) provide (without having to maintain).

The primary use case I envision for bdk bindings in wasm is that we would be able to feed some function a descriptor string (maybe even support for more complex policies for future extensibility, but really multisig descriptors is all Caravan and nearly every other coordinator in the space really cares about) to a function and get a parseable object/JSON of the Extended Key Derivation/Origin information (XKey from bdk kind of I think? There are so many names for this amalgamation of information). The rest, such as client information, is totally ancillary and implementation specific. If you think about where this would be used in Caravan, it would be in the main wallet loading area. A descriptor string text input could supersede most of the fields on this screen, and that's what I think would be most useful for bdk to handle.

The nice thing about generalizing it this way too is that your team doesn't have to try to keep up maintainability with whatever the caravan config may or may not need to support. So considering the screenshot above, a possible user flow would look like:

input descriptor string(s) (could be one if BIP 88 style), into a text input at the top of the window
in the background bdk wasm bindings would: i) validate descriptor(s) ii) pull out all relevant wallet information
the UI might then prefill all relevant details it can pull from the descriptor: xpubs, network (?), quorum size
user would fill out any other extraneous information that caravan might care about like client info and name

The nice thing about this too imo is that it could be used as a common interface to convert to any other wallet config pretty easily once we have this to/from descriptor api, e.g. Descriptor -> Caravan -> Coldcard. The final step could be facilitated completely in the JS side with something that looks like this:

const coldcardConfig = '...'
const wallet = ColdcardConfig.deserialize(coldcardConfig)

wallet.getDescriptors() // ["wsh(sortedmulti(2,[3h8280]xpub.../0/*, [748d8s0]xpub.../0/*))", "..."]
const descriptor = wallet.toBip88Descriptor() // "wsh(sortedmulti(2,[3h8280]xpub.../{0,1}/*, [748d8s0]xpub.../{0,1}/*))"

const carvanConfig = CaravanWalletConfig.fromBip88Descriptor(
		descriptor,
		{ 
			...wallet, // wallet.name, wallet.uuid, wallet.network
			startingAddressIndex: 10 
		} 
)

caravanConfig.serialize() // { ... } -> caravan compatible json
wallet.serialize() // "..." -> coldcard wallet config text

// these should be the same and export standard descriptors
// that can be imported to bitcoind (for example)
assert_equal(caravanConfig.getDescriptors(), wallet.getDescriptors())

Under the hood, the getDescriptors and toBip88Descriptors would just be using the bdk utilities. Everything else is just implementation detail. (the bip88 stuff would just expand the bip88 {0,1} style wildcards to two separate descriptor strings).

Anyway, that's just how I've been thinking about it and I feel like should make your work easier.

For priv keys:
yes, caravan itself will only ever be watch only as all signing should be delegated out to some external signer. Currently we support Coldcard, trezor, ledger, and manual. We intend on adding bcr2 QR support soon which would open up support for many other signers. That said, given the approach described above of this being a generalized tool, there's no reason this couldn't support priv keys and then in other applications that might need it and want that capability, it would be there (I'll probably build these tools with the bindings into unchained-bitcoin to use in Caravan, but also make available to other applications that wish to use it). It's also not an urgent requirement for our goals at least.

(other questions answered inline)

bucko13 · 2022-04-30T00:32:12Z

+//!   "quorum": {
+//!     "requiredSigners": 2,
+//!     "totalSigners": 2
+//!   },


Ugh, yeah just seeing this in here, I'd hate to have you have to support our janky serialization standard. This is really redundant information. We should just have requiredSigners and then "total" can be inferred from the number of extended public keys. We have to support this for backwards compatibility but I don't think it's worth bdk having to take on that code debt.

bucko13 · 2022-04-30T00:55:37Z

+            .map(|k| {
+                let fingerprint = k.xfp;
+                let key_path = k.bip32_path.clone();
+                let key_source = fingerprint.zip(key_path);


Forgive the rust ignorance, but what's the advantage of doing this rather than

let key_source = (fingerprint, key_path)

fingerprint and key_path are both optional items, but what you want is not a tuple of options but an optional tuple!

doing what you described would yield the type (Option<Fingerprint>, Option<DerivationPath>), but what we want here is a Option<(Fingerprint, DerivationPath)>.

Essentially you want either to have both of them or none of them. Having a tuple of options allows you to have just one but not the other, but if you change it to an optional tuple you either have the tuple or don't.

Fantastic. Thanks for the explanation! Still new to rust :). Learning that Options are an important building block.

bucko13 · 2022-04-30T00:57:54Z

+    ) -> Result<Self, Error> {
+        let (address_type, quorum, descriptor_public_keys) = match descriptor {
+            Descriptor::Sh(sh) => match sh.as_inner() {
+                ShInner::SortedMulti(smv) => {


what does smv stand for?

I guess it's an abbreviation of "sorted multi value" (?).

It doesn't matter too much, in that context it's basically used as a variable name, you could call it anything.

Yep, makes sense. Just curious as it helps to follow the code imo.

bucko13 · 2022-04-30T00:59:07Z

+            Self::export(network, descriptor, name, client_type)
+        } else {
+            Err(Error::Generic(
+                "Can not export a wallet with a change descriptor to Caravan.".to_string(),


So, I think this would need to change right? Caravan does have change as well as receive index support.

notmandatory · 2022-04-30T02:37:32Z

For the larger discussion about using BDK for generic parsing of descriptors for use in a TypeScript app I think we can do this with the existing (but still experimental) descriptor::policy module. This module will parse any descriptor into a common Policy structure that can be easily serialized to JSON. I originally intended to use the Policy parser for this PR but since the Caravan config is so constrained it was easier to pull what I needed directly from the Descriptor structure.

If you want to see what the Policy JSON output looks like you can use the bdk-cli tool to parse any wallet descriptor. Installation instruction are here. Once installed the command to parse a descriptor is:

bdk-cli wallet -d $ALICE_DESCRIPTOR policies

If this looks like something you can use I'll make an example for doing the same parsing from TypeScript using the bdk-wasm pkg.

Ultimately I believe descriptors (with public keys) will be the "lingua franca" for all wallet configs since they contain everything a user needs (besides the master keys) to restore a wallet. Even if you include other ancillary info in your config that other wallets won't understand such as wallet name, mapping user entered names to key fingerprints, the blockchain client configs, etc. the user can still always use the descriptors to monitor and spend using any descriptor based wallet software.

bucko13 · 2022-05-03T04:04:52Z

Ultimately I believe descriptors (with public keys) will be the "lingua franca" for all wallet configs since they contain everything a user needs (besides the master keys) to restore a wallet. Even if you include other ancillary info in your config that other wallets won't understand such as wallet name, mapping user entered names to key fingerprints, the blockchain client configs, etc. the user can still always use the descriptors to monitor and spend using any descriptor based wallet software.

Yep, this is exactly my thinking and what my larger goal for using this tool would be.

So, I tested out bdk-cli locally:

$ ➜ ALICE="wsh(sortedmulti(1,[9d120b19/48'/0'/0'/2']xpub6FDrnnUsgQSwRFazYbVDs9eadQaNV13f5dtQDoWrCuMNq2qgMH7GevctMAm3PeHq3KBkh9BgA8iPfaHYACHFpfueYdeAUtjjEH3vMJWEKfu,[5c9e228d/48'/0'/0'/2']xpub6EgGHjcvovyN3nK921zAGPfuB41cJXkYRdt3tLGmiMyvbgHpss4X1eRZwShbEBb1znz2e2bCkCED87QZpin3sSYKbmCzQ9Sc7LaV98ngdeX))#dh7v9p4x"
$ ➜ bdk-cli -n bitcoin wallet -d $ALICE policies

With the following return:

{
  "external": {
    "contribution": {
      "items": [],
      "m": 1,
      "n": 2,
      "sorted": true,
      "type": "PARTIAL"
    },
    "id": "hw3hgusf",
    "keys": [
      {
        "fingerprint": "9d120b19"
      },
      {
        "fingerprint": "5c9e228d"
      }
    ],
    "satisfaction": {
      "items": [],
      "m": 1,
      "n": 2,
      "sorted": true,
      "type": "PARTIAL"
    },
    "threshold": 1,
    "type": "MULTISIG"
  },
  "internal": null
}

Information it looks like we do get from policies is:

m (required signers) and n (total signers)
sorted and multi
xfps for the keys involved

Unless there's something in my command that I'm missing (didn't see any additional options in the help though), info we'd still need returned:

checksums
key origins (path, extended public key, as well as the xfp)
script type

inferring the network from the xpub version as well would be nice, but as discussed elsewhere, it's not a guarantee (could let that be the responsibility of the consuming application to validate or allow to be overwritten).

It looks like most of this work is being done in this PR by the parse_descriptor function

notmandatory · 2022-05-03T16:37:29Z

Unless there's something in my command that I'm missing (didn't see any additional options in the help though), info we'd still need returned:

checksums

key origins (path, extended public key, as well as the xfp)

script type

No there's no other options right now to get the above extra info. But I agree if we added that info somewhere in the Policy structure it would be generally useful for parsing a Descriptor and displaying it's spending requirements. I created #597 to track this suggestion, but it might take some time to get to as the team is now focused on getting Taproot support added (which also impacts the policy module).

evanlinjin · 2022-07-05T03:35:31Z

+        if wallet.change_descriptor.is_none() {
+            return Err(Error::Generic(
+                "Wallet much have an internal descriptor".to_string(),
+            ));
+        }
+        let internal_descriptor = wallet
+            .change_descriptor
+            .as_ref()
+            .expect("internal descriptor");


Would it better to use a match statement here?

Also there seems to be a typo on line 206: "much" -> "must"

notmandatory · 2023-08-20T23:45:35Z

@bucko13 I've extracted this code into a new standalone project that uses the new bdk 1.0-alpha as a dependency and is compiled to wasm for use in a web page. I also created a very basic web page example. Everything should work for converting between descriptors and caravan config json. But much docs and cleanup needed before publishing it as a npm package.

https://github.com/notmandatory/caravan-rs
https://github.com/notmandatory/caravan-rs-example

notmandatory · 2024-03-24T23:19:13Z

Very old PR and not part of 1.0 milestone so closing for now. May take it back up post 1.0 as an example or new add-on module.

notmandatory force-pushed the caravan_export branch from 1e84248 to 6d52e69 Compare April 29, 2022 05:39

afilini reviewed Apr 29, 2022

View reviewed changes

notmandatory marked this pull request as ready for review April 29, 2022 21:53

notmandatory self-assigned this Apr 29, 2022

bucko13 reviewed Apr 30, 2022

View reviewed changes

notmandatory force-pushed the caravan_export branch from e6155cd to c71a9a6 Compare April 30, 2022 01:41

notmandatory force-pushed the caravan_export branch from c71a9a6 to 7f1e05a Compare April 30, 2022 07:23

notmandatory mentioned this pull request May 3, 2022

Add additional information to Policy structure #597

Closed

evanlinjin reviewed Jul 5, 2022

View reviewed changes

notmandatory force-pushed the caravan_export branch 2 times, most recently from 46199c5 to 0343ab6 Compare July 8, 2022 17:50

notmandatory added 6 commits July 8, 2022 10:53

Reorganize wallet::export into wallet::export::fully_noded submodule

4cd9f03

Add wallet::export::caravan module

3052b65

Add basic docs for wallet::export::caravan module

7bb1672

Add suggested changes from code review

04a9b05

Cleanup and add more wallet::export::caravan tests

5b33a59

Add internal descriptor and validation to wallet::export::caravan

8a39387

notmandatory force-pushed the caravan_export branch from 0343ab6 to 8a39387 Compare July 8, 2022 17:54

notmandatory marked this pull request as draft August 20, 2023 23:49

notmandatory closed this Mar 24, 2024

notmandatory deleted the caravan_export branch May 26, 2025 21:38

Conversation

notmandatory commented Apr 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Notes to the reviewers

Checklists

All Submissions:

New Features:

Bugfixes:

Uh oh!

notmandatory commented Apr 29, 2022

Uh oh!

afilini left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

afilini commented Apr 29, 2022

Uh oh!

notmandatory commented Apr 29, 2022

Uh oh!

bucko13 commented Apr 30, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notmandatory commented Apr 30, 2022

Uh oh!

bucko13 commented May 3, 2022

Uh oh!

notmandatory commented May 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

notmandatory commented Aug 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

notmandatory commented Mar 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

notmandatory commented Apr 29, 2022 •

edited

Loading

notmandatory commented Aug 20, 2023 •

edited

Loading