Adding `importdescriptors` for Descriptor-Based Wallets #199

sandipndev · 2021-09-13T15:48:29Z

This PR adds the following features:

importdescriptors RPC command
Bech32m address support
Updated createwallet to add in descriptors parameter
Updates version of rust-bitcoin to 0.27
The integration_test/run.sh script was updated to support the latest versioning of Bitcoin Core.

I figure this would be very useful for anyone creating descriptor based wallets, especially with Taproot coming in!

…scriptors parameter and added bech32m address type

xraid · 2022-03-15T23:27:14Z

Running bitcoind (master) and a BDK descriptor-based wallet

wallet.sync gives me Rpc(JsonRpc(Rpc(RpcError { code: -4, message: "This type of wallet does not support this command", data: None })))'

that seems come from /bitcoin/src/wallet/rpc/util.cpp line 109

and have mention in bitcoin 23946
"This occurs when your wallet is a descriptor wallet. It is expected behavior.
For builds of the master branch (and the future 23.0 release), newly created wallets will default to being descriptor wallets."

Running bitcoind v22.0 and a BDK descriptor-based wallet works but will break at v23.0 onward it seems

so maybe this PR after review can make us progress forward ...

RCasatta · 2022-03-16T13:19:46Z

so maybe this PR after review can make us progress forward ...

importdescriptor was avoided in BDK RPCBlockchain because at that time the descriptor supported by core was a subset of the descriptors supported by BDK.

We should verify if it's not the case anymore with 23.0 and use importdescriptor or otherwise BDK should specify the non-default option during wallet creation (non-descriptor wallet)

Sorry for the BDK off-topic, this PR should be worked on anyway

danielabrozzoni · 2022-03-17T11:33:51Z

Concept ACK - the code looks good, but there are conflicts with master. I'll review once is rebased :)

dunxen · 2022-06-05T06:35:20Z

integration_test/Cargo.toml

@@ -5,6 +5,6 @@ authors = ["Steven Roose <steven@stevenroose.org>"]

 [dependencies]
 bitcoincore-rpc = { path = "../client" }
-bitcoin = { version = "0.26", features = [ "use-serde", "rand" ] }
+bitcoin = { version = "0.27", features = [ "use-serde", "rand" ] }


Currently master is at 0.28.0, but I believe we should drop the patch specification as before so users of the library can resolve the latest patch.

So just a rebase is in order and then we can drop the patch version :)

Suggested change

bitcoin = { version = "0.27", features = [ "use-serde", "rand" ] }

bitcoin = { version = "0.28", features = [ "use-serde", "rand" ] }

dunxen · 2022-06-05T06:36:42Z

json/Cargo.toml

@@ -21,4 +21,4 @@ path = "src/lib.rs"
 serde = { version = "1", features = [ "derive" ] }
 serde_json = "1"

-bitcoin = { version = "0.26", features = [ "use-serde" ] }
+bitcoin = { version = "0.27", features = [ "use-serde" ] }


Suggested change

bitcoin = { version = "0.27", features = [ "use-serde" ] }

bitcoin = { version = "0.28", features = [ "use-serde" ] }

dunxen · 2022-06-05T06:47:25Z

We'll also need to make the following an Option<usize> for descriptor wallet compatibility:

rust-bitcoincore-rpc/json/src/lib.rs

Line 148 in 1ab603e

pub keypool_oldest: usize,

andrewtoth · 2022-06-05T14:24:58Z

integration_test/src/main.rs

@@ -914,20 +919,23 @@ fn test_create_wallet(cl: &Client) {
            blank: None,
            passphrase: None,
            avoid_reuse: None,
+            descriptors: None,


Bitcoin Core 23 now creates descriptor wallets by default. So in order for these tests to pass, this needs to be descriptors: Some(false) for each of these WalletParams.

andrewtoth · 2022-06-05T14:28:14Z

This should be an Option<usize> as well if we're updating getwalletinfo:

rust-bitcoincore-rpc/json/src/lib.rs

Line 152 in 1ab603e

pub keypool_size_hd_internal: usize,

andrewtoth · 2022-06-05T14:29:51Z

integration_test/src/main.rs


    test_get_network_info(&cl);
    unsafe { VERSION = cl.version().unwrap() };
    println!("Version: {}", version());

-    cl.create_wallet("testwallet", None, None, None, None).unwrap();
+    cl.create_wallet("testwallet", None, None, None, None, None).unwrap();


Needs to be Some(false) for Bitcoin Core 23 compatibility.

andrewtoth · 2022-06-05T14:33:09Z

client/src/client.rs

-            handle_defaults(&mut args, &[false.into(), false.into(), into_json("")?, false.into()]),
+            handle_defaults(
+                &mut args,
+                &[false.into(), false.into(), into_json("")?, false.into(), false.into()],


Last default option should be true.into() to be compatible with Bitcoin Core 23.

craigraw · 2022-11-08T08:25:07Z

I don't mean to seem impatient, but is there any hope of this getting merged? I find it amazing that the Rust Bitcoin community has no need to use descriptor wallets in Bitcoin Core, which have now been available for over 2 years and are required for Taproot.

dunxen · 2022-11-08T08:35:47Z

I don't mean to seem impatient, but is there any hope of this getting merged?

Are you going the route without BWT now, which also isn't getting much attention currently. Adding Taproot support there wouldn't do much other than just pass descriptors to Core directly (at least that's how I'd do it if this were merged). At least then I don't think Sparrow would need this if you're just going to do descriptor RPC directly. But agree that someone would definitely need to use descriptor wallets with this crate.

craigraw · 2022-11-08T08:49:20Z

Are you going the route without BWT now, which also isn't getting much attention currently.

Yes, I'm considering giving up with BWT. It doesn't seem like Rust Bitcoin has sufficient community to keep up. I'd prefer a Java library for Sparrow in any case.

sandipndev force-pushed the import-descriptors branch from ff2e1f5 to 2e87fa5 Compare September 13, 2021 15:49

notmandatory mentioned this pull request Sep 15, 2021

Adding importdescriptors for Descriptor-Based Wallets RCasatta/rust-bitcoincore-rpc#1

Merged

sandipndev added 2 commits September 15, 2021 23:25

updating to rust-bitcoin 0.27

1986e2d

added importdescriptors command, updated createwallet command with de…

1ab603e

…scriptors parameter and added bech32m address type

sandipndev force-pushed the import-descriptors branch from 2e87fa5 to 1ab603e Compare September 15, 2021 17:56

RCasatta mentioned this pull request Oct 8, 2021

Move to original rustbitcoincore-rpc rust-bitcoin/bitcoind#32

Closed

notmandatory mentioned this pull request Nov 1, 2021

Update GetPeerInfoResultNetwork for core 22.0 #206

Closed

dunxen mentioned this pull request May 27, 2022

Allow descriptor wallet creation #223

Open

notmandatory mentioned this pull request May 28, 2022

Add test parsing, signing, finalizing a psbt made by core afilini/bdk#4

Merged

9 tasks

dunxen reviewed Jun 5, 2022

View reviewed changes

andrewtoth reviewed Jun 5, 2022

View reviewed changes

dunxen mentioned this pull request Jun 6, 2022

Add Taproot support bwt-dev/bwt#91

Open

craigraw mentioned this pull request Nov 8, 2022

App does not accept Bitcoin-Core connection sparrowwallet/sparrow#737

Closed

tcharding added this to the v0.20.0 milestone Apr 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding `importdescriptors` for Descriptor-Based Wallets #199

Adding `importdescriptors` for Descriptor-Based Wallets #199

sandipndev commented Sep 13, 2021

xraid commented Mar 15, 2022

RCasatta commented Mar 16, 2022

danielabrozzoni commented Mar 17, 2022

dunxen Jun 5, 2022

dunxen Jun 5, 2022

dunxen commented Jun 5, 2022

andrewtoth Jun 5, 2022

andrewtoth commented Jun 5, 2022

andrewtoth Jun 5, 2022

andrewtoth Jun 5, 2022

craigraw commented Nov 8, 2022

dunxen commented Nov 8, 2022

craigraw commented Nov 8, 2022

	bitcoin = { version = "0.27", features = [ "use-serde", "rand" ] }
	bitcoin = { version = "0.28", features = [ "use-serde", "rand" ] }

Adding importdescriptors for Descriptor-Based Wallets #199

Are you sure you want to change the base?

Adding importdescriptors for Descriptor-Based Wallets #199

Conversation

sandipndev commented Sep 13, 2021

xraid commented Mar 15, 2022

RCasatta commented Mar 16, 2022

danielabrozzoni commented Mar 17, 2022

dunxen Jun 5, 2022

Choose a reason for hiding this comment

dunxen Jun 5, 2022

Choose a reason for hiding this comment

dunxen commented Jun 5, 2022

andrewtoth Jun 5, 2022

Choose a reason for hiding this comment

andrewtoth commented Jun 5, 2022

andrewtoth Jun 5, 2022

Choose a reason for hiding this comment

andrewtoth Jun 5, 2022

Choose a reason for hiding this comment

craigraw commented Nov 8, 2022

dunxen commented Nov 8, 2022

craigraw commented Nov 8, 2022

Adding `importdescriptors` for Descriptor-Based Wallets #199

Adding `importdescriptors` for Descriptor-Based Wallets #199