fix: spks_of_all_keychains() shouldn't return an infinite iterator for non-wildcard descriptors#1093
Conversation
fa41c7c to
ef01631
Compare
ef01631 to
6e8ae26
Compare
evanlinjin
left a comment
There was a problem hiding this comment.
Thank you for spotting and fixing this @danielabrozzoni!
I think SpkIterator::new_with_range should also detect non-wildcard descriptors, and the implementation of SpkIterator::new can be changed to this:
/// Creates a new script pubkey iterator starting at 0 from a descriptor.
pub fn new(descriptor: D) -> Self {
SpkIterator::new_with_range(descriptor, 0..=BIP32_MAX_INDEX)
}This means, SpkIterator:new_with_range will sometimes return empty iterators.
|
+1 for what @evanlinjin suggested above. |
I tried to implement this by setting |
..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 ```
6e8ae26 to
cc1a43c
Compare
Turns out that this was wrong. I'm now shortening whichever range passed to |
evanlinjin
left a comment
There was a problem hiding this comment.
Thanks for fixing this. This was my oversight.
As discussed (via DM), it'll be better to have non-wildcard descriptors only returns something on derivation index 0.
The follow tests should pass:
// 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);..spk only if index is equal to 0
evanlinjin
left a comment
There was a problem hiding this comment.
ACK e48b911
Tested with example_electrum with non-wildcard descriptor.
Left a non-critical nit.
Description
When you pass a non-wildcard descriptor in
new_with_range, we makesure that the range length is at most 1; if that's not the case, we
shorten it.
We would previously use
new_with_rangewithout this check and with anon-wildcard descriptor in
spks_of_all_keychains, this meant creatinga 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:Changelog notice
KeychainTxOutIndex::spks_of_all_keychains/KeychainTxOutIndex::spks_of_keychainwould return an iterator yielding infinite spks even for non-wildcard descriptors.Checklists
All Submissions:
cargo fmtandcargo clippybefore committing