-
Notifications
You must be signed in to change notification settings - Fork 295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: spks_of_all_keychains() shouldn't return an infinite iterator for non-wildcard descriptors #1093
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_range
without 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_keychain
would return an iterator yielding infinite spks even for non-wildcard descriptors.Checklists
All Submissions:
cargo fmt
andcargo clippy
before committing