Skip to content

Commit

Permalink
Merge #1506: lianad: fix check that rescan has started
Browse files Browse the repository at this point in the history 
6b914fd bitcoind: parse descriptor before comparing strings (Michael Mallan)
652eb5b refactor(bitcoind): extract logic to function (Michael Mallan)

Pull request description:

  The `startrescan` command uses the response from bitcoind's `listdescriptors` RPC method to confirm that the rescan has started.

  This check includes a string comparison that can fail in some cases (Taproot descriptors with a spending path having only a single key) as the descriptor string returned by bitcoind may contain both `h` and `'` , whereas Liana's descriptor string only uses `'` (as per the `Descriptor::to_string()` method).

  This PR changes the check to first parse each descriptor string as a `Descriptor` object to avoid any `h` / `'` mismatch issues.

ACKs for top commit:
  edouardparis:
    utACK 6b914fd

Tree-SHA512: 14d3eaef950a490020af2157e03adcbd1cc821c92750f21e682ba74bc4fd3531552a7ba0f70144792d5bbc18546b919567f181e468e2b196c6c473ca45d9d9de
  • Loading branch information
@edouardparis
edouardparis committed Jan 2, 2025
2 parents d28dd76 + 6b914fd commit 31c4955
Show file tree
Hide file tree
Showing 3 changed files with 231 additions and 17 deletions.
183 changes: 169 additions & 14 deletions lianad/src/bitcoin/d/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ use jsonrpc::{

use miniscript::{
bitcoin::{self, address, hashes::hex::FromHex},
descriptor,
descriptor::{self, Descriptor, DescriptorPublicKey},
};

use serde_json::Value as Json;
Expand Down Expand Up @@ -1056,15 +1056,15 @@ impl BitcoinD {

// For the given descriptor strings check if they are imported at this timestamp in the
// watchonly wallet.
fn check_descs_timestamp(&self, descs: &[String], timestamp: u32) -> bool {
fn check_descs_timestamp(
&self,
descs: &[&Descriptor<DescriptorPublicKey>],
timestamp: u32,
) -> bool {
let current_descs = self.list_descriptors();

for desc in descs {
let present = current_descs
.iter()
.find(|entry| &entry.desc == desc)
.map(|entry| entry.timestamp == timestamp)
.unwrap_or(false);
let present = current_descs_contain_desc_timestamp(&current_descs, desc, timestamp);
if !present {
return false;
}
Expand Down Expand Up @@ -1112,15 +1112,15 @@ impl BitcoinD {
.fold(1_000, |range, entry| {
cmp::max(range, entry.range.map(|r| r[1]).unwrap_or(0))
});
let desc_str = [
desc.receive_descriptor().to_string(),
desc.change_descriptor().to_string(),
let descs = [
desc.receive_descriptor().as_descriptor_public_key(),
desc.change_descriptor().as_descriptor_public_key(),
];
let desc_json: Vec<Json> = desc_str
let desc_json: Vec<Json> = descs
.iter()
.map(|desc_str| {
.map(|desc| {
serde_json::json!({
"desc": desc_str,
"desc": desc.to_string(),
"timestamp": timestamp,
"active": false,
"range": max_range,
Expand Down Expand Up @@ -1154,7 +1154,7 @@ impl BitcoinD {
}

i += 1;
if self.check_descs_timestamp(&desc_str, timestamp) {
if self.check_descs_timestamp(&descs, timestamp) {
return Ok(());
} else if i >= NUM_RETRIES {
return Err(BitcoindError::StartRescan);
Expand Down Expand Up @@ -1281,6 +1281,29 @@ pub struct ListDescEntry {
pub timestamp: u32,
}

/// Whether `current_descs` contain the descriptor `desc` at `timestamp`.
///
/// Any descriptors in `current_descs` that cannot be parsed as
/// `Descriptor::<DescriptorPublicKey>` will be ignored.
fn current_descs_contain_desc_timestamp(
current_descs: &[ListDescEntry],
desc: &Descriptor<DescriptorPublicKey>,
timestamp: u32,
) -> bool {
current_descs
.iter()
.filter_map(|entry| {
if let Ok(entry_desc) = Descriptor::<DescriptorPublicKey>::from_str(&entry.desc) {
Some((entry_desc, entry.timestamp))
} else {
None
}
})
.find(|(entry_desc, _)| entry_desc.to_string() == desc.to_string())
.map(|(_, entry_timestamp)| entry_timestamp == timestamp)
.unwrap_or(false)
}

/// A 'received' entry in the 'listsinceblock' result.
#[derive(Debug, Clone)]
pub struct LSBlockEntry {
Expand Down Expand Up @@ -1592,4 +1615,136 @@ mod tests {
0.9999
);
}

#[test]
fn test_current_descs_contain_desc_timestamp() {
// For simplicity, I've removed the checksums in the following descriptors as these will change
// depending on whether `h` or `'` is used.

// Simulate that `listdescriptors` returns an entry for receive and change descriptors, respectively.
// Include one of the descriptors with two different timestamps and another invalid descriptor.
let current_descs = vec![
ListDescEntry {
desc: "this is not a descriptor and will be ignored".to_string(),
range: Some([0, 999]),
timestamp: 1598918410,
},
ListDescEntry {
desc: "tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/2/*),older(65535)))".to_string(),
range: Some([0, 999]),
timestamp: 1598918400,
},
ListDescEntry {
desc: "tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/1/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/3/*),older(65535)))".to_string(),
range: Some([0, 999]),
timestamp: 1598918380,
},
// same as receive descriptor above but with different timestamp.
ListDescEntry {
desc: "tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/2/*),older(65535)))".to_string(),
range: Some([0, 999]),
timestamp: 1598918410,
},
];

// Create the Liana wallet descriptor:
let desc = LianaDescriptor::from_str("tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/<0;1>/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/<2;3>/*),older(65535)))").unwrap();

// The receive and change descriptors contain only `'`:
assert_eq!(desc.receive_descriptor().to_string(), "tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/2/*),older(65535)))#xhrh0cvn".to_string());
assert_eq!(desc.change_descriptor().to_string(), "tr([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/1/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/3/*),older(65535)))#6yyu2dsu".to_string());

let recv_desc = desc.receive_descriptor().as_descriptor_public_key();
let change_desc = desc.change_descriptor().as_descriptor_public_key();
// For the receive descriptor, we don't get a match unless the timestamp matches the first occurrence.
assert!(!current_descs_contain_desc_timestamp(
&current_descs,
recv_desc,
1598918399
));
assert!(!current_descs_contain_desc_timestamp(
&current_descs,
recv_desc,
1598918401
));
assert!(!current_descs_contain_desc_timestamp(
&current_descs,
change_desc,
1598918381
));
assert!(!current_descs_contain_desc_timestamp(
&current_descs,
recv_desc,
1598918410 // this is the second timestamp for this descriptor
));
// We only get a match when we use the first timestamp for each descriptor.
assert!(current_descs_contain_desc_timestamp(
&current_descs,
recv_desc,
1598918400
));
assert!(current_descs_contain_desc_timestamp(
&current_descs,
change_desc,
1598918380
));

// If the `listdescriptors` response contains a mix of `h` and `'`, then there is still a match.
let current_descs = vec![
ListDescEntry {
desc: "this is not a descriptor and will be ignored".to_string(),
range: Some([0, 999]),
timestamp: 1598918410,
},
ListDescEntry {
desc: "tr([1dce71b2/48h/1h/0h/2h]tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/2/*),older(65535)))".to_string(),
range: Some([0, 999]),
timestamp: 1598918400,
},
ListDescEntry {
desc: "tr([1dce71b2/48h/1h/0h/2h]tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/1/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/3/*),older(65535)))".to_string(),
range: Some([0, 999]),
timestamp: 1598918380,
},
];
assert!(current_descs_contain_desc_timestamp(
&current_descs,
recv_desc,
1598918400
));
assert!(current_descs_contain_desc_timestamp(
&current_descs,
change_desc,
1598918380
));

// There is still a match if the checksum is included in the `listdescriptors` response.
let current_descs = vec![
ListDescEntry {
desc: "this is not a descriptor and will be ignored".to_string(),
range: Some([0, 999]),
timestamp: 1598918410,
},
ListDescEntry {
desc: "tr([1dce71b2/48h/1h/0h/2h]tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/0/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/2/*),older(65535)))#2h7g2wme".to_string(),
range: Some([0, 999]),
timestamp: 1598918400,
},
ListDescEntry {
desc: "tr([1dce71b2/48h/1h/0h/2h]tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/1/*,and_v(v:pk([1dce71b2/48'/1'/0'/2']tpubDEeP3GefjqbaDTTaVAF5JkXWhoFxFDXQ9KuhVrMBViFXXNR2B3Lvme2d2AoyiKfzRFZChq2AGMNbU1qTbkBMfNv7WGVXLt2pnYXY87gXqcs/3/*),older(65535)))#kyer0m8k".to_string(),
range: Some([0, 999]),
timestamp: 1598918380,
},
];
assert!(current_descs_contain_desc_timestamp(
&current_descs,
recv_desc,
1598918400
));
assert!(current_descs_contain_desc_timestamp(
&current_descs,
change_desc,
1598918380
));
}
}
57 changes: 54 additions & 3 deletions tests/fixtures.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -145,11 +145,20 @@ def xpub_fingerprint(hd):
return _pubkey_to_fingerprint(hd.pubkey).hex()


def single_key_desc(prim_fg, prim_xpub, reco_fg, reco_xpub, csv_value, is_taproot):
def single_key_desc(
prim_fg,
prim_xpub,
reco_fg,
reco_xpub,
csv_value,
is_taproot,
prim_deriv_path="",
reco_deriv_path="",
):
if is_taproot:
return f"tr([{prim_fg}]{prim_xpub}/<0;1>/*,and_v(v:pk([{reco_fg}]{reco_xpub}/<0;1>/*),older({csv_value})))"
return f"tr([{prim_fg}{prim_deriv_path}]{prim_xpub}/<0;1>/*,and_v(v:pk([{reco_fg}{reco_deriv_path}]{reco_xpub}/<0;1>/*),older({csv_value})))"
else:
return f"wsh(or_d(pk([{prim_fg}]{prim_xpub}/<0;1>/*),and_v(v:pkh([{reco_fg}]{reco_xpub}/<0;1>/*),older({csv_value}))))"
return f"wsh(or_d(pk([{prim_fg}{prim_deriv_path}]{prim_xpub}/<0;1>/*),and_v(v:pkh([{reco_fg}{reco_deriv_path}]{reco_xpub}/<0;1>/*),older({csv_value}))))"


@pytest.fixture
Expand Down Expand Up @@ -193,6 +202,48 @@ def lianad(bitcoin_backend, directory):
lianad.cleanup()


# This can currently only be used with Taproot if no signing is required.
@pytest.fixture
def lianad_with_deriv_paths(bitcoin_backend, directory):
datadir = os.path.join(directory, "lianad")
os.makedirs(datadir, exist_ok=True)

signer = SingleSigner(is_taproot=USE_TAPROOT)
(prim_fingerprint, primary_xpub), (reco_fingerprint, recovery_xpub) = (
(xpub_fingerprint(signer.primary_hd), signer.primary_hd.get_xpub()),
(xpub_fingerprint(signer.recovery_hd), signer.recovery_hd.get_xpub()),
)
csv_value = 10
main_desc = Descriptor.from_str(
single_key_desc(
prim_fingerprint,
primary_xpub,
reco_fingerprint,
recovery_xpub,
csv_value,
USE_TAPROOT,
"/48h/1h/0h/2h",
"/46h/12h/10h/72h",
)
)

lianad = Lianad(
datadir,
signer,
main_desc,
bitcoin_backend,
)

try:
lianad.start()
yield lianad
except Exception:
lianad.cleanup()
raise

lianad.cleanup()


def unspendable_internal_xpub(xpubs):
"""Deterministic, unique, unspendable internal key.
See See https://delvingbitcoin.org/t/unspendable-keys-in-descriptors/304/21
Expand Down
8 changes: 8 additions & 0 deletions tests/test_rpc.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -619,6 +619,14 @@ def test_broadcast_spend(lianad, bitcoind):
lianad.rpc.broadcastspend(txid)


# Use a descriptor that includes hardened derivation paths so that we can check
# there is no problem regarding the use of `h` and `'`.
def test_start_rescan_does_not_error(lianad_with_deriv_paths, bitcoind):
"""Test we can successfully start a rescan."""
tip_timestamp = bitcoind.rpc.getblockheader(bitcoind.rpc.getbestblockhash())["time"]
lianad_with_deriv_paths.rpc.startrescan(tip_timestamp - 1)


def test_start_rescan(lianad, bitcoind):
"""Test we successfully retrieve all our transactions after losing state by rescanning."""
initial_timestamp = int(time.time())
Expand Down

0 comments on commit 31c4955

Please sign in to comment.