Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix derivable-descriptor detection during wallet sync #692

Closed
wants to merge 1 commit into from
Closed

Fix derivable-descriptor detection during wallet sync #692

wants to merge 1 commit into from

Conversation

evanlinjin
Copy link
Member

@evanlinjin evanlinjin commented Jul 30, 2022 •
edited
Loading

Description

This fix is for electrum-based blockchains.

Previously, it was assumed that containing > 1 spks in database means the descriptor is derivable, however each database tracks two descriptors so it should really check > 2.

Notes to the reviewers

This is a touch-up on work done in #672

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing

Bugfixes:

* [ ] This pull request breaks the existing API
* [ ] I've added tests to reproduce the issue which are now passing
* [ ] I'm linking the issue being fixed by this PR

@evanlinjin
Fix derivable-descriptor detection during wallet sync
9d2bea2 
This fix is for electrum-based blockchains.

Previously, it was assumed that containing > 1 spks in database means
the descriptor is derivable, however each database tracks two
descriptors so it should really check > 2.
@evanlinjin evanlinjin force-pushed the fix-non-derivable-desc-detection branch from 9349d3c to 9d2bea2 Compare July 30, 2022 13:54
@evanlinjin evanlinjin marked this pull request as ready for review July 30, 2022 14:02
danielabrozzoni
danielabrozzoni approved these changes Jul 30, 2022
View reviewed changes
Copy link
Member

@danielabrozzoni danielabrozzoni left a comment

Choose a reason for hiding this comment

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

utACK 9d2bea2

@danielabrozzoni
Copy link
Member

Can you add a test to reproduce the bug? I suppose that having one wallet with two non-derivable descriptors should do it

@danielabrozzoni danielabrozzoni mentioned this pull request Aug 1, 2022
Closed
20 tasks
@evanlinjin
Copy link
Member Author

@danielabrozzoni Thanks for the tip! I realized I confused myself. 😅

I forgot that ScriptReq::satisfy works with "one keychain at a time" so let is_derivable = self.initial_scripts_needed > 1; (the original) is correct.

I'll close this PR.

@evanlinjin evanlinjin closed this Aug 2, 2022
@notmandatory notmandatory removed this from the Release 0.21.0 Feature Freeze milestone Aug 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danielabrozzoni danielabrozzoni danielabrozzoni approved these changes

Assignees

@evanlinjin evanlinjin

Labels
None yet
Projects
BDK Maintenance
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@evanlinjin @danielabrozzoni @notmandatory