wallet: rescan and recover addresses in all relevant key scopes #682
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Roasbeef
requested changes
Mar 23, 2020
wallet/wallet.go
Outdated
| // Wallets with a birthday after this height are not susceptible | ||
| // to the bug described, so they can safely only scan the | ||
| // default key scopes. | ||
| mainNetHeight = 623000 |
There was a problem hiding this comment.
How were these heights obtained? The heights when the prior active addr change landed in master?
There was a problem hiding this comment.
Instead, I think we should just have this be based off of a database migration version which detects if a change address was ever created under a non-default scope. If so, then we need to check for everything constantly, otherwise we can only start to watch the default scopes from there.
|
@Roasbeef I've added a migration with some unit tests. I'll be testing this over the next few days on some nodes, but let me know if this is what you had in mind. |
Previously, the wallet would determine the key scope to use for change addresses by locating the one compatible with P2WPKH addresses, but this wasn't always safe like in the case when multiple key scopes that supported these addresses existed within the address manager, leading the change address to be created outside of the intended key scope.
The commit being reverted resulted in the discovery of a bug in which change addresses could at times be created outside of the default key scopes, causing us to not properly determine their spends.
Due to a no longer existing bug within the wallet, it was possible for change addresses to be created outside of their intended key scope (the default), so wallets affected by this now need to ensure they scan the chain for all addresses within the default key scopes (as expected), and all _internal_ addresses (branch used for change addresses) within any other registered key scopes to reflect their proper balance.
In similar fashion to the previous commit, due to a no longer existing bug within the wallet, it was possible for change addresses to be created outside of their intended key scope (the default), so wallets affected by this now need to ensure upon recovery that they scan the chain for _all_ existing key scopes, rather than just the default ones, to reflect their proper balance. Through manual testing, it was shown that the impact of recovering the additional key scopes is negligible in most cases for both full nodes and light clients.
Roasbeef
approved these changes
Mar 30, 2020
cfromknecht
approved these changes
Mar 31, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, the wallet would determine the key scope to use for change addresses by locating the one compatible with P2WPKH addresses, but this wasn't always safe like in the case when multiple key scopes that supported these addresses existed within the address manager, leading the change address to be created outside of the intended key scope.
Wallets affected by this now need to ensure they scan the chain when performing rescans or recovery attempts for addresses within all existing key scopes, rather than just the default ones, to reflect their proper balance. For newer wallets not affected by this, they should only scan for addresses within their default key scopes.