Merge #628: rpc: use importdescriptors with Core >= 0.21

e1a1372 rpc: use `importdescriptors` with Core >= 0.21 (Alekos Filini)

Pull request description:

  ### Description

  Only use the old `importmulti` with Core versions that don't support descriptor-based (sqlite) wallets.

  Add an extra feature to test against Core 0.20 called `test-rpc-legacy`.

  This also makes us compatible with Core 23.0 and is thus a replacement for #613, which actually looking back at it was adding support for 23.0 but probably breaking older wallets by adding the extra argument to `createwallet`.

  I believe #613 should now only focus on getting the tests to work against 23.0, which is still important but not such a high priority as being compatible with the latest version of Core.

  Also fixes #598

  ### Checklists

  #### All Submissions:

  * [x] I've signed all my commits
  * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md)
  * [x] I ran `cargo fmt` and `cargo clippy` before committing

  #### New Features:

  * [x] I've added tests for the new feature
  * [x] I've added docs for the new feature
  * [x] I've updated `CHANGELOG.md`

ACKs for top commit:
  notmandatory:
    ACK e1a1372

Tree-SHA512: 7891b8ab2fc900ea2a186f64a32aea970f28a50339063ed0e1a8d13248e5c038b8fff3d9e26b93cb7daafd0c873379e64a28836dbe4e4b82f1983577a88971ff

.github/workflows/cont_integration.yml

@@ -95,6 +95,9 @@ jobs:
           - name: rpc
             testprefix: blockchain::rpc::test
             features: test-rpc
+          - name: rpc-legacy
+            testprefix: blockchain::rpc::test
+            features: test-rpc-legacy
           - name: esplora
             testprefix: esplora
             features: test-esplora,use-esplora-reqwest,verify

CHANGELOG.md

@@ -18,6 +18,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - Creation of Taproot PSBTs (BIP-371)
   - Signing Taproot PSBTs (key spend and script spend)
   - Support for `tr()` descriptors in the `descriptor!()` macro
+- Add support for Bitcoin Core 23.0 when using the `rpc` blockchain
 
 ## [v0.18.0] - [v0.17.0]
 

Cargo.toml

@@ -88,16 +88,17 @@ reqwest-default-tls = ["reqwest/default-tls"]
 
 # Debug/Test features
 test-blockchains = ["bitcoincore-rpc", "electrum-client"]
-test-electrum = ["electrum", "electrsd/electrs_0_8_10", "test-blockchains"]
-test-rpc = ["rpc", "electrsd/electrs_0_8_10", "test-blockchains"]
-test-esplora = ["electrsd/legacy", "electrsd/esplora_a33e97e1", "test-blockchains"]
+test-electrum = ["electrum", "electrsd/electrs_0_8_10", "electrsd/bitcoind_22_0", "test-blockchains"]
+test-rpc = ["rpc", "electrsd/electrs_0_8_10", "electrsd/bitcoind_22_0", "test-blockchains"]
+test-rpc-legacy = ["rpc", "electrsd/electrs_0_8_10", "electrsd/bitcoind_0_20_0", "test-blockchains"]
+test-esplora = ["electrsd/legacy", "electrsd/esplora_a33e97e1", "electrsd/bitcoind_22_0", "test-blockchains"]
 test-md-docs = ["electrum"]
 
 [dev-dependencies]
 lazy_static = "1.4"
 env_logger = "0.7"
 clap = "2.33"
-electrsd = { version= "0.19.1", features = ["bitcoind_22_0"] }
+electrsd = "0.19.1"
 
 [[example]]
 name = "address_validator"

README.md

@@ -166,7 +166,7 @@ Integration testing require testing features, for example:
 cargo test --features test-electrum
 ```
 
-The other options are `test-esplora` or `test-rpc`.
+The other options are `test-esplora`, `test-rpc` or `test-rpc-legacy` which runs against an older version of Bitcoin Core.
 Note that `electrs` and `bitcoind` binaries are automatically downloaded (on mac and linux), to specify you already have installed binaries you must use `--no-default-features` and provide `BITCOIND_EXE` and `ELECTRS_EXE` as environment variables.
 
 ## License

src/blockchain/rpc.rs

@@ -32,15 +32,17 @@
 //! ```
 
 use crate::bitcoin::consensus::deserialize;
+use crate::bitcoin::hashes::hex::ToHex;
 use crate::bitcoin::{Address, Network, OutPoint, Transaction, TxOut, Txid};
 use crate::blockchain::*;
 use crate::database::{BatchDatabase, DatabaseUtils};
+use crate::descriptor::get_checksum;
 use crate::{BlockTime, Error, FeeRate, KeychainKind, LocalUtxo, TransactionDetails};
 use bitcoincore_rpc::json::{
     GetAddressInfoResultLabel, ImportMultiOptions, ImportMultiRequest,
     ImportMultiRequestScriptPubkey, ImportMultiRescanSince,
 };
-use bitcoincore_rpc::jsonrpc::serde_json::Value;
+use bitcoincore_rpc::jsonrpc::serde_json::{json, Value};
 use bitcoincore_rpc::Auth as RpcAuth;
 use bitcoincore_rpc::{Client, RpcApi};
 use log::debug;
@@ -54,6 +56,8 @@ use std::str::FromStr;
 pub struct RpcBlockchain {
     /// Rpc client to the node, includes the wallet name
     client: Client,
+    /// Whether the wallet is a "descriptor" or "legacy" wallet in Core
+    is_descriptors: bool,
     /// Blockchain capabilities, cached here at startup
     capabilities: HashSet<Capability>,
     /// Skip this many blocks of the blockchain at the first rescan, if None the rescan is done from the genesis block
@@ -177,22 +181,53 @@ impl WalletSync for RpcBlockchain {
             "importing {} script_pubkeys (some maybe already imported)",
             scripts_pubkeys.len()
         );
-        let requests: Vec<_> = scripts_pubkeys
-            .iter()
-            .map(|s| ImportMultiRequest {
-                timestamp: ImportMultiRescanSince::Timestamp(0),
-                script_pubkey: Some(ImportMultiRequestScriptPubkey::Script(s)),
-                watchonly: Some(true),
-                ..Default::default()
-            })
-            .collect();
-        let options = ImportMultiOptions {
-            rescan: Some(false),
-        };
-        // Note we use import_multi because as of bitcoin core 0.21.0 many descriptors are not supported
-        // https://bitcoindevkit.org/descriptors/#compatibility-matrix
-        //TODO maybe convenient using import_descriptor for compatible descriptor and import_multi as fallback
-        self.client.import_multi(&requests, Some(&options))?;
+
+        if self.is_descriptors {
+            // Core still doesn't support complex descriptors like BDK, but when the wallet type is
+            // "descriptors" we should import individual addresses using `importdescriptors` rather
+            // than `importmulti`, using the `raw()` descriptor which allows us to specify an
+            // arbitrary script
+            let requests = Value::Array(
+                scripts_pubkeys
+                    .iter()
+                    .map(|s| {
+                        let desc = format!("raw({})", s.to_hex());
+                        json!({
+                            "timestamp": "now",
+                            "desc": format!("{}#{}", desc, get_checksum(&desc).unwrap()),
+                        })
+                    })
+                    .collect(),
+            );
+
+            let res: Vec<Value> = self.client.call("importdescriptors", &[requests])?;
+            res.into_iter()
+                .map(|v| match v["success"].as_bool() {
+                    Some(true) => Ok(()),
+                    Some(false) => Err(Error::Generic(
+                        v["error"]["message"]
+                            .as_str()
+                            .unwrap_or("Unknown error")
+                            .to_string(),
+                    )),
+                    _ => Err(Error::Generic("Unexpected response from Core".to_string())),
+                })
+                .collect::<Result<Vec<_>, _>>()?;
+        } else {
+            let requests: Vec<_> = scripts_pubkeys
+                .iter()
+                .map(|s| ImportMultiRequest {
+                    timestamp: ImportMultiRescanSince::Timestamp(0),
+                    script_pubkey: Some(ImportMultiRequestScriptPubkey::Script(s)),
+                    watchonly: Some(true),
+                    ..Default::default()
+                })
+                .collect();
+            let options = ImportMultiOptions {
+                rescan: Some(false),
+            };
+            self.client.import_multi(&requests, Some(&options))?;
+        }
 
         loop {
             let current_height = self.get_height()?;
@@ -369,20 +404,36 @@ impl ConfigurableBlockchain for RpcBlockchain {
         debug!("connecting to {} auth:{:?}", wallet_url, config.auth);
 
         let client = Client::new(wallet_url.as_str(), config.auth.clone().into())?;
+        let rpc_version = client.version()?;
+
         let loaded_wallets = client.list_wallets()?;
         if loaded_wallets.contains(&wallet_name) {
             debug!("wallet already loaded {:?}", wallet_name);
+        } else if list_wallet_dir(&client)?.contains(&wallet_name) {
+            client.load_wallet(&wallet_name)?;
+            debug!("wallet loaded {:?}", wallet_name);
         } else {
-            let existing_wallets = list_wallet_dir(&client)?;
-            if existing_wallets.contains(&wallet_name) {
-                client.load_wallet(&wallet_name)?;
-                debug!("wallet loaded {:?}", wallet_name);
-            } else {
+            // pre-0.21 use legacy wallets
+            if rpc_version < 210_000 {
                 client.create_wallet(&wallet_name, Some(true), None, None, None)?;
-                debug!("wallet created {:?}", wallet_name);
+            } else {
+                // TODO: move back to api call when https://github.com/rust-bitcoin/rust-bitcoincore-rpc/issues/225 is closed
+                let args = [
+                    Value::String(wallet_name.clone()),
+                    Value::Bool(true),
+                    Value::Bool(false),
+                    Value::Null,
+                    Value::Bool(false),
+                    Value::Bool(true),
+                ];
+                let _: Value = client.call("createwallet", &args)?;
             }
+
+            debug!("wallet created {:?}", wallet_name);
         }
 
+        let is_descriptors = is_wallet_descriptor(&client)?;
+
         let blockchain_info = client.get_blockchain_info()?;
         let network = match blockchain_info.chain.as_str() {
             "main" => Network::Bitcoin,
@@ -399,7 +450,6 @@ impl ConfigurableBlockchain for RpcBlockchain {
         }
 
         let mut capabilities: HashSet<_> = vec![Capability::FullHistory].into_iter().collect();
-        let rpc_version = client.version()?;
         if rpc_version >= 210_000 {
             let info: HashMap<String, Value> = client.call("getindexinfo", &[]).unwrap();
             if info.contains_key("txindex") {
@@ -416,6 +466,7 @@ impl ConfigurableBlockchain for RpcBlockchain {
         Ok(RpcBlockchain {
             client,
             capabilities,
+            is_descriptors,
             _storage_address: storage_address,
             skip_blocks: config.skip_blocks,
         })
@@ -438,6 +489,20 @@ fn list_wallet_dir(client: &Client) -> Result<Vec<String>, Error> {
     Ok(result.wallets.into_iter().map(|n| n.name).collect())
 }
 
+/// Returns whether a wallet is legacy or descriptors by calling `getwalletinfo`.
+///
+/// This API is mapped by bitcoincore_rpc, but it doesn't have the fields we need (either
+/// "descriptors" or "format") so we have to call the RPC manually
+fn is_wallet_descriptor(client: &Client) -> Result<bool, Error> {
+    #[derive(Deserialize)]
+    struct CallResult {
+        descriptors: Option<bool>,
+    }
+
+    let result: CallResult = client.call("getwalletinfo", &[])?;
+    Ok(result.descriptors.unwrap_or(false))
+}
+
 /// Factory of [`RpcBlockchain`] instances, implements [`BlockchainFactory`]
 ///
 /// Internally caches the node url and authentication params and allows getting many different [`RpcBlockchain`]
@@ -500,7 +565,7 @@ impl BlockchainFactory for RpcBlockchainFactory {
 }
 
 #[cfg(test)]
-#[cfg(feature = "test-rpc")]
+#[cfg(any(feature = "test-rpc", feature = "test-rpc-legacy"))]
 mod test {
     use super::*;
     use crate::testutils::blockchain_tests::TestClient;
@@ -514,7 +579,7 @@ mod test {
                 url: test_client.bitcoind.rpc_url(),
                 auth: Auth::Cookie { file: test_client.bitcoind.params.cookie_file.clone() },
                 network: Network::Regtest,
-                wallet_name: format!("client-wallet-test-{:?}", std::time::SystemTime::now() ),
+                wallet_name: format!("client-wallet-test-{}", std::time::SystemTime::now().duration_since(std::time::UNIX_EPOCH).unwrap().as_nanos() ),
                 skip_blocks: None,
             };
             RpcBlockchain::from_config(&config).unwrap()

src/testutils/blockchain_tests.rs

@@ -387,6 +387,7 @@ macro_rules! bdk_blockchain_tests {
                 Wallet::new(&descriptors.0.to_string(), descriptors.1.as_ref(), Network::Regtest, MemoryDatabase::new()).unwrap()
             }
 
+            #[allow(dead_code)]
             enum WalletType {
                 WpkhSingleSig,
                 TaprootKeySpend,
@@ -421,7 +422,7 @@ macro_rules! bdk_blockchain_tests {
                 let wallet = get_wallet_from_descriptors(&descriptors);
 
                 // rpc need to call import_multi before receiving any tx, otherwise will not see tx in the mempool
-                #[cfg(feature = "test-rpc")]
+                #[cfg(any(feature = "test-rpc", feature = "test-rpc-legacy"))]
                 wallet.sync(&blockchain, SyncOptions::default()).unwrap();
 
                 (wallet, blockchain, descriptors, test_client)
@@ -446,7 +447,7 @@ macro_rules! bdk_blockchain_tests {
 
                 // the RPC blockchain needs to call `sync()` during initialization to import the
                 // addresses (see `init_single_sig()`), so we skip this assertion
-                #[cfg(not(feature = "test-rpc"))]
+                #[cfg(not(any(feature = "test-rpc", feature = "test-rpc-legacy")))]
                 assert!(wallet.database().deref().get_sync_time().unwrap().is_none(), "initial sync_time not none");
 
                 wallet.sync(&blockchain, SyncOptions::default()).unwrap();
@@ -933,7 +934,7 @@ macro_rules! bdk_blockchain_tests {
             #[test]
             fn test_sync_bump_fee_add_input_no_change() {
                 let (wallet, blockchain, descriptors, mut test_client) = init_single_sig();
-                                let node_addr = test_client.get_node_address(None);
+                let node_addr = test_client.get_node_address(None);
 
                 test_client.receive(testutils! {
                     @tx ( (@external descriptors, 0) => 50_000, (@external descriptors, 1) => 25_000 ) (@confirmations 1)
@@ -1021,6 +1022,7 @@ macro_rules! bdk_blockchain_tests {
             }
 
             #[test]
+            #[cfg(not(feature = "test-rpc-legacy"))]
             fn test_send_to_bech32m_addr() {
                 use std::str::FromStr;
                 use serde;
@@ -1207,7 +1209,7 @@ macro_rules! bdk_blockchain_tests {
                 let blockchain = get_blockchain(&test_client);
 
                 let wallet = get_wallet_from_descriptors(&descriptors);
-                #[cfg(feature = "test-rpc")]
+                #[cfg(any(feature = "test-rpc", feature = "test-rpc-legacy"))]
                 wallet.sync(&blockchain, SyncOptions::default()).unwrap();
 
                 let _ = test_client.receive(testutils! {
@@ -1231,6 +1233,7 @@ macro_rules! bdk_blockchain_tests {
             }
 
             #[test]
+            #[cfg(not(feature = "test-rpc-legacy"))]
             fn test_taproot_key_spend() {
                 let (wallet, blockchain, descriptors, mut test_client) = init_wallet(WalletType::TaprootKeySpend);
 
@@ -1251,6 +1254,7 @@ macro_rules! bdk_blockchain_tests {
             }
 
             #[test]
+            #[cfg(not(feature = "test-rpc-legacy"))]
             fn test_taproot_script_spend() {
                 let (wallet, blockchain, descriptors, mut test_client) = init_wallet(WalletType::TaprootScriptSpend);
 
@@ -1279,20 +1283,24 @@ macro_rules! bdk_blockchain_tests {
             }
 
             #[test]
+            #[cfg(not(feature = "test-rpc-legacy"))]
             fn test_sign_taproot_core_keyspend_psbt() {
                 test_sign_taproot_core_psbt(WalletType::TaprootKeySpend);
             }
 
             #[test]
+            #[cfg(not(feature = "test-rpc-legacy"))]
             fn test_sign_taproot_core_scriptspend2_psbt() {
                 test_sign_taproot_core_psbt(WalletType::TaprootScriptSpend2);
             }
 
             #[test]
+            #[cfg(not(feature = "test-rpc-legacy"))]
             fn test_sign_taproot_core_scriptspend3_psbt() {
                 test_sign_taproot_core_psbt(WalletType::TaprootScriptSpend3);
             }
 
+            #[cfg(not(feature = "test-rpc-legacy"))]
             fn test_sign_taproot_core_psbt(wallet_type: WalletType) {
                 use std::str::FromStr;
                 use serde_json;