Skip to content

refactor wallet address caching into its own public method #537

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

Merged
merged 2 commits into from
Mar 2, 2022
Merged

refactor wallet address caching into its own public method #537

merged 2 commits into from
Mar 2, 2022

Conversation

a5an0
Copy link
Contributor

@a5an0 a5an0 commented Feb 2, 2022

Description

Currently, the only way to ensure that a wallet's internal database has addresses loaded and cached is through Wallet::sync: that function generates and caches up to a number of addresses if they aren't already in the database, and then uses the wallet's blockchain client to sync those addresses. If you are using an offline wallet, there is no mechanism to ensure that the database has addresses loaded. This is a problem for usecases like an offline wallet being used as a multisig signer and wanting to validate change addresses as Wallet::is_mine will only work properly if the owned-address is loaded in the database.

This PR takes the address caching functionality out of Wallet::sync and puts it in a new public method that is available to offline wallets. Wallet::sync uses this method internally.

Checklists

All Submissions:

  • I've signed all my commits
  • I followed the contribution guidelines
  • I ran cargo fmt and cargo clippy before committing
  • I've added docs for the new feature
  • I've updated CHANGELOG.md

@LLFourn
Copy link
Contributor

LLFourn commented Feb 3, 2022

Concept ACK. Like @a5an0 mentions, we need this when you give your descriptors over to another machine which generates addresses without your knowledge and then hands you a PSBT to sign. HW wallets like coldcard do this kind of thing too.

An important side note is that caching addresses is not even technically necessary for sync I think. You can just generate the new ones on the fly (up until stop_gap) as you look them up. Right now our sync logic depends on pre-caching but I want to fix this.

@afilini
Copy link
Member

afilini commented Feb 7, 2022

Try rebasing on the lastest master, it should fix the CI

@afilini
Copy link
Member

afilini commented Feb 7, 2022

Concept ACK btw, this seems useful.

Right now our sync logic depends on pre-caching but I want to fix this.

Yes, this would be great. The current sync/setup behavior is also very inconsistent, we should think about what to do there.

@notmandatory notmandatory self-assigned this Feb 8, 2022
@notmandatory
Copy link
Member

Once #502 is merged you'll need to rebase to pickup some CI changes that are in that PR. Then this one looks good to go.

@a5an0
Copy link
Contributor Author

a5an0 commented Feb 22, 2022

rebased onto master to pick up CI changes

Copy link
Contributor

@LLFourn LLFourn left a comment

Choose a reason for hiding this comment

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

This LGTM just a few cosmetic things that I think we should fix up.

@a5an0
Copy link
Contributor Author

a5an0 commented Feb 24, 2022

Thanks for the feedback! updated.

Copy link
Contributor

@LLFourn LLFourn left a comment

Choose a reason for hiding this comment

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

ACK 66c6158 squash and merge

Copy link
Member

@notmandatory notmandatory left a comment

Choose a reason for hiding this comment

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

ACK edf2f0c

@notmandatory notmandatory merged commit adf7d0c into bitcoindevkit:master Mar 2, 2022
@a5an0 a5an0 deleted the offline-address-cache branch March 2, 2022 12:56
@LLFourn LLFourn mentioned this pull request Mar 7, 2022
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants