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.

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

wallet: Remove mainchain tip check on NeedsAccountsSync #2327

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

matheusd
Copy link
Member

@matheusd matheusd commented Feb 2, 2024

This check could prevent a wallet from going through address discovery if it had not completed the initial chain sync prior to being restarted.

One instance this could happen is when restoring SPV wallets under an instable network connection or having a power loss during the initial sync of a restored wallet.

Detected while debugging issues with #2318.

@jrick
Copy link
Member

jrick commented Feb 2, 2024

I'm pretty sure i've relied on this as a hack to get around the initial discovery when i planned to avoid it anyways by manually setting the address indexes, e.g. on a voter with mixed ticketbuying and large gaps.

Seems fine to fix this, but I do think we want a proper way to intentionally opt out, probably with some additional prompts during wallet creation or more flags.

@jrick
Copy link
Member

jrick commented Feb 2, 2024

Wait, hold on. This is for account discovery, not address discovery.

The only location this is used in dcrwallet itself (not importing an embedded wallet) is https://github.com/decred/dcrwallet/blob/master/dcrwallet.go#L485-L500, and all this does is cause the wallet to be unlocked at the time of discovery (it will run regardless of what this function returns or not), and because the wallet is unlocked, it will opportunistically also discover accounts and not just addresses.

@matheusd
Copy link
Member Author

matheusd commented Feb 5, 2024

Seems fine to fix this, but I do think we want a proper way to intentionally opt out, probably with some additional prompts during wallet creation or more flags.

You mean, opt out of address/account discovery on a given wallet? Something like --noaccountdiscovery and/or --noaddressdiscovery? If so, I can these on this PR.

@jrick
Copy link
Member

jrick commented Feb 5, 2024

They aren't needed on this pr, but yeah flags like that would be nice.

This still looks like a bug worth fixing as is, but I just don't think the explanation is accurate. Is some other software e.g. dcrlnd using this return result to trigger address discovery?

@matheusd
Copy link
Member Author

matheusd commented Feb 5, 2024

No, you are correct this is regarding account discovery, not address discovery. I'll fix the description in the commit.

This check could prevent a wallet from going through account discovery
if it had not completed the initial chain sync prior to being restarted.

One instance this could happen is when restoring SPV wallets under an
instable network connection or having a power loss during the initial
sync of a restored wallet.
@matheusd matheusd force-pushed the acct-discovery-partial-download branch from 796fb76 to 2264d0f Compare February 6, 2024 10:42
@jrick jrick merged commit 2264d0f into decred:master Feb 6, 2024
2 checks passed
@matheusd matheusd deleted the acct-discovery-partial-download branch February 6, 2024 13:59
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.

2 participants