From cc1a43c495639f0dbe6da52501cb48f17b8cbbd2 Mon Sep 17 00:00:00 2001 From: Daniela Brozzoni Date: Thu, 24 Aug 2023 16:53:50 +0200 Subject: [PATCH 1/3] fix: SpkIterator::new_with_range takes wildcards.. ..into account When you pass a non-wildcard descriptor in `new_with_range`, we make sure that the range length is at most 1; if that's not the case, we shorten it. We would previously use `new_with_range` without this check and with a non-wildcard descriptor in `spks_of_all_keychains`, this meant creating a spkiterator that would go on producing the same spks over and over again, causing some issues with syncing on electrum/esplora. To reproduce the bug, run in `example-crates/example_electrum`: ``` cargo run "sh(wsh(or_d(c:pk_k(cPGudvRLDSgeV4hH9NUofLvYxYBSRjju3cpiXmBg9K8G9k1ikCMp),c:pk_k(cSBSBHRrzqSXFmrBhLkZMzQB9q4P9MnAq92v8d9a5UveBc9sLX32))))#zp9pcfs9" scan ``` --- crates/chain/src/spk_iter.rs | 47 ++++++++++++++----- .../chain/tests/test_keychain_txout_index.rs | 8 ++++ 2 files changed, 43 insertions(+), 12 deletions(-) diff --git a/crates/chain/src/spk_iter.rs b/crates/chain/src/spk_iter.rs index 1e09df36..bce436ed 100644 --- a/crates/chain/src/spk_iter.rs +++ b/crates/chain/src/spk_iter.rs @@ -45,34 +45,41 @@ where { /// Creates a new script pubkey iterator starting at 0 from a descriptor. pub fn new(descriptor: D) -> Self { - let end = if descriptor.borrow().has_wildcard() { - BIP32_MAX_INDEX - } else { - 0 - }; - - SpkIterator::new_with_range(descriptor, 0..=end) + SpkIterator::new_with_range(descriptor, 0..=BIP32_MAX_INDEX) } // Creates a new script pubkey iterator from a descriptor with a given range. + // If the descriptor doesn't have a wildcard, we shorten whichever range you pass in + // to have length <= 1. This means that if you pass in 0..0 or 0..1 the range will + // remain the same, but if you pass in 0..10, we'll shorten it to 0..1 pub(crate) fn new_with_range(descriptor: D, range: R) -> Self where R: RangeBounds, { + let start = match range.start_bound() { + Bound::Included(start) => *start, + Bound::Excluded(start) => *start + 1, + Bound::Unbounded => u32::MIN, + }; + let mut end = match range.end_bound() { Bound::Included(end) => *end + 1, Bound::Excluded(end) => *end, Bound::Unbounded => u32::MAX, }; + // Because `end` is exclusive, we want the maximum value to be BIP32_MAX_INDEX + 1. end = end.min(BIP32_MAX_INDEX + 1); + if !descriptor.borrow().has_wildcard() { + // The length of the range should be at most 1 + if end != start { + end = start + 1; + } + } + Self { - next_index: match range.start_bound() { - Bound::Included(start) => *start, - Bound::Excluded(start) => *start + 1, - Bound::Unbounded => u32::MIN, - }, + next_index: start, end, descriptor, secp: Secp256k1::verification_only(), @@ -195,6 +202,22 @@ mod test { let mut external_spk = SpkIterator::new(&no_wildcard_descriptor); + assert_eq!(external_spk.nth(0).unwrap(), (0, external_spk_0.clone())); + assert_eq!(external_spk.nth(0), None); + + let mut external_spk = SpkIterator::new_with_range(&no_wildcard_descriptor, 0..0); + + assert_eq!(external_spk.next(), None); + + let mut external_spk = SpkIterator::new_with_range(&no_wildcard_descriptor, 0..1); + + assert_eq!(external_spk.nth(0).unwrap(), (0, external_spk_0.clone())); + assert_eq!(external_spk.next(), None); + + // We test that using new_with_range with range_len > 1 gives back an iterator with + // range_len = 1 + let mut external_spk = SpkIterator::new_with_range(&no_wildcard_descriptor, 0..10); + assert_eq!(external_spk.nth(0).unwrap(), (0, external_spk_0)); assert_eq!(external_spk.nth(0), None); } diff --git a/crates/chain/tests/test_keychain_txout_index.rs b/crates/chain/tests/test_keychain_txout_index.rs index 96a1afd1..817b0349 100644 --- a/crates/chain/tests/test_keychain_txout_index.rs +++ b/crates/chain/tests/test_keychain_txout_index.rs @@ -384,4 +384,12 @@ fn test_non_wildcard_derivations() { txout_index.reveal_to_target(&TestKeychain::External, 200); assert_eq!(revealed_spks.count(), 0); assert!(revealed_changeset.is_empty()); + + // we check that spks_of_keychain returns a SpkIterator with just one element + assert_eq!( + txout_index + .spks_of_keychain(&TestKeychain::External) + .count(), + 1, + ); } From a7a1d9b2fb9fda4d86fe17cf90d9c0d46c4d8609 Mon Sep 17 00:00:00 2001 From: Daniela Brozzoni Date: Thu, 31 Aug 2023 17:48:56 +0200 Subject: [PATCH 2/3] fix: non-wildcard descriptors should return an.. ..spk only if index is equal to 0 --- crates/chain/src/spk_iter.rs | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/crates/chain/src/spk_iter.rs b/crates/chain/src/spk_iter.rs index bce436ed..a7331ff9 100644 --- a/crates/chain/src/spk_iter.rs +++ b/crates/chain/src/spk_iter.rs @@ -52,6 +52,8 @@ where // If the descriptor doesn't have a wildcard, we shorten whichever range you pass in // to have length <= 1. This means that if you pass in 0..0 or 0..1 the range will // remain the same, but if you pass in 0..10, we'll shorten it to 0..1 + // Also note that if the descriptor doesn't have a wildcard, passing in a range starting + // from n > 0, will return an empty iterator. pub(crate) fn new_with_range(descriptor: D, range: R) -> Self where R: RangeBounds, @@ -100,6 +102,11 @@ where return None; } + // If the descriptor is non-wildcard, only index 0 will return an spk. + if !self.descriptor.borrow().has_wildcard() && self.next_index != 0 { + return None; + } + let script = self .descriptor .borrow() @@ -220,6 +227,24 @@ mod test { assert_eq!(external_spk.nth(0).unwrap(), (0, external_spk_0)); assert_eq!(external_spk.nth(0), None); + + // non index-0 should NOT return an spk + assert_eq!( + SpkIterator::new_with_range(&no_wildcard_descriptor, 1..1).next(), + None + ); + assert_eq!( + SpkIterator::new_with_range(&no_wildcard_descriptor, 1..=1).next(), + None + ); + assert_eq!( + SpkIterator::new_with_range(&no_wildcard_descriptor, 1..2).next(), + None + ); + assert_eq!( + SpkIterator::new_with_range(&no_wildcard_descriptor, 1..=2).next(), + None + ); } // The following dummy traits were created to test if SpkIterator is working properly. From e48b911c8d23cceab2071cc92085fe6675d55ca9 Mon Sep 17 00:00:00 2001 From: Daniela Brozzoni Date: Fri, 1 Sep 2023 09:13:33 +0200 Subject: [PATCH 3/3] refactor: Make test errors more readable --- crates/chain/src/spk_iter.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/crates/chain/src/spk_iter.rs b/crates/chain/src/spk_iter.rs index a7331ff9..a80cd608 100644 --- a/crates/chain/src/spk_iter.rs +++ b/crates/chain/src/spk_iter.rs @@ -174,18 +174,18 @@ mod test { let mut external_spk = SpkIterator::new(&external_desc); let max_index = BIP32_MAX_INDEX - 22; - assert_eq!(external_spk.next().unwrap(), (0, external_spk_0)); - assert_eq!(external_spk.nth(15).unwrap(), (16, external_spk_16)); - assert_eq!(external_spk.nth(3).unwrap(), (20, external_spk_20.clone())); - assert_eq!(external_spk.next().unwrap(), (21, external_spk_21)); + assert_eq!(external_spk.next(), Some((0, external_spk_0))); + assert_eq!(external_spk.nth(15), Some((16, external_spk_16))); + assert_eq!(external_spk.nth(3), Some((20, external_spk_20.clone()))); + assert_eq!(external_spk.next(), Some((21, external_spk_21))); assert_eq!( - external_spk.nth(max_index as usize).unwrap(), - (BIP32_MAX_INDEX, external_spk_max) + external_spk.nth(max_index as usize), + Some((BIP32_MAX_INDEX, external_spk_max)) ); assert_eq!(external_spk.nth(0), None); let mut external_spk = SpkIterator::new_with_range(&external_desc, 0..21); - assert_eq!(external_spk.nth(20).unwrap(), (20, external_spk_20)); + assert_eq!(external_spk.nth(20), Some((20, external_spk_20))); assert_eq!(external_spk.next(), None); let mut external_spk = SpkIterator::new_with_range(&external_desc, 0..21); @@ -204,12 +204,12 @@ mod test { let mut external_spk = SpkIterator::new(&no_wildcard_descriptor); - assert_eq!(external_spk.next().unwrap(), (0, external_spk_0.clone())); + assert_eq!(external_spk.next(), Some((0, external_spk_0.clone()))); assert_eq!(external_spk.next(), None); let mut external_spk = SpkIterator::new(&no_wildcard_descriptor); - assert_eq!(external_spk.nth(0).unwrap(), (0, external_spk_0.clone())); + assert_eq!(external_spk.nth(0), Some((0, external_spk_0.clone()))); assert_eq!(external_spk.nth(0), None); let mut external_spk = SpkIterator::new_with_range(&no_wildcard_descriptor, 0..0); @@ -218,14 +218,14 @@ mod test { let mut external_spk = SpkIterator::new_with_range(&no_wildcard_descriptor, 0..1); - assert_eq!(external_spk.nth(0).unwrap(), (0, external_spk_0.clone())); + assert_eq!(external_spk.nth(0), Some((0, external_spk_0.clone()))); assert_eq!(external_spk.next(), None); // We test that using new_with_range with range_len > 1 gives back an iterator with // range_len = 1 let mut external_spk = SpkIterator::new_with_range(&no_wildcard_descriptor, 0..10); - assert_eq!(external_spk.nth(0).unwrap(), (0, external_spk_0)); + assert_eq!(external_spk.nth(0), Some((0, external_spk_0))); assert_eq!(external_spk.nth(0), None); // non index-0 should NOT return an spk