Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .github/workflows/cont_integration.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Add support for Bitcoin Core 23.0 when using the `rpc` blockchain
- Add support for Bitcoin Core 22.0 and pre-22.0 when using the `rpc` blockchain

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

22.0 and before were already supported previously, since we were always creating legacy wallets and using importmulti.

With this PR we add support for 23 which uses descriptor wallets by default, and change a bit the way we interact with wallets 0.21 and up.. but they were already supported before, so from the user's perspective I guess the only change is that now they can use it with 23 while previously they couldn't

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But in this PR the CI is testing with electrsd/bitcoind_22_0 for test-electrum, test-rpc and test-esplora , and when I tried changing these to bitcoind_23_0 the tests hung and failed for test-electrum and test-rpc, I have some notes in the (resolved) comment above.

Since we're not adding any new support for rpc with core 23 how about I remove the changelog message instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see what you mean. I would keep it there because we've actually added support for 23, it's just the automated tests that are still failing.

I guess the confusion comes from the fact that my original issue was referring to the tests against 23 failing, but in fact it was not just the tests, it was BDK being completely broken with 23.0. So now this PR fixed BDK so that it can properly interact with newer nodes, but since the tests are still failing somewhere in our mechanism to automate them, I kept the tests running against 22.

I tested it manually by switching the bitcoind feature to use bitcoin 23 and running the rpcwallet example, which internally spawns a core node with electrsd, and it works just fine. Same thing for older versions compared to what we test for, it works fine all the way back to 0.18.1. It's just again the tests hanging with core < 0.20 for some reason.

I'm attaching here the logs for the 23.0 and 0.18.1 run, they are a bit messy because there's the stdout from Core itself, plus logging from bdk and bitcoincore_rpc so that you can see all the calls being made. You'll notice that with the old node we use importmulti, while with the newer one we use importdescriptors which is what I implemented in this PR.

So long story short, BDK works with 23 now, we just have to figure out how to run automated tests reliably against it!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok that's great! I won't take your changelog message out and @rajarshimaitra and I are going to keep working on how to fix the automated tests.


## [v0.18.0] - [v0.17.0]

Expand Down
9 changes: 5 additions & 4 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
117 changes: 91 additions & 26 deletions src/blockchain/rpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
Expand Down Expand Up @@ -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()?;
Expand Down Expand Up @@ -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,
Expand All @@ -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") {
Expand All @@ -416,6 +466,7 @@ impl ConfigurableBlockchain for RpcBlockchain {
Ok(RpcBlockchain {
client,
capabilities,
is_descriptors,
_storage_address: storage_address,
skip_blocks: config.skip_blocks,
})
Expand All @@ -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`]
Expand Down Expand Up @@ -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;
Expand All @@ -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()
Expand Down
16 changes: 12 additions & 4 deletions src/testutils/blockchain_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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)
Expand All @@ -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();
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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! {
Expand All @@ -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);

Expand All @@ -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);

Expand Down Expand Up @@ -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;
Expand Down