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

[BUG][Wallet] Restore AddressBook when marking used keys in the keypool #2117

Merged

Conversation

random-zebra
Copy link

Problem
If a HD wallet uses some keys, and is later restored to a previous state (initial backup), it correctly recognizes the used keys and relative transactions, and spending from the relative addresses works fine.
Although, these addresses are not shown in the receive widget of the GUI, so their label cannot be edited, and they cannot be easily selected/copied.

Cause
When adding a transaction in AddToWalletIfInvolvingMe, the wallet loops over the outputs and checks if it affects any key from the keypool (supposed to be "unused"), in which case, it removes the key from the pool (ScriptPubKeyMan::MarkUnusedAddresses).

After restoring the old backup, when initializing the receive widget (and address table model), with getAddressToShow, there is no unused address in the AddressBook, and a new one is created.
getNewAddress accesses the key pool and returns the first unused key.
Since the used keys have been removed, this is a brand new address.
All addresses relative to the keys removed from the pool in MarkUnusedAddresses, therefore, never enter the AddressBook, and thus are never shown in the receive widget.

Solution
When removing the key from the pool (in MarkReserveKeysAsUsed), also add the missing address to the AddressBook, with receive/coldstaking purpose.

This way, when restoring an old backup, the user loses only the labels (that were added after the wallet.dat backup) but can still repopulate his AddressBook with all the previously used addresses.

@random-zebra random-zebra self-assigned this Dec 31, 2020
@random-zebra random-zebra added this to the 5.1.0 milestone Jan 17, 2021
furszy
furszy previously approved these changes Jan 21, 2021
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

code review ACK 67cc427.

Future: would be good to have a simple test covering this. Just need to restore the wallet from an old backup file and call getaddressesbylabel, verifying that the address is in the returned list with an empty label.

@random-zebra
Copy link
Author

👍 done.
Rebased on master, and added functional test at the beginning (which fails before the patch and passes after).

**Problem:**
If a HD wallet uses some keys, and is later restored to a previous state
(initial backup), it correctly recognizes the used keys and relative
transactions, and spending from the relative addresses works fine.
Although, these addresses are not shown in the receive widget of the
GUI, so their label cannot be edited, and they cannot be easily
selected/copied.

**Cause**
When adding a transaction in `AddToWalletIfInvolvingMe`, the wallet
loops over the outputs and checks if it affects any key from the keypool
(supposed to be "unused"), in which case, it removes the key from the
pool (`ScriptPubKeyMan::MarkUnusedAddresses`).

After restoring the old backup, when initializing the receive widget
(and address table model), with `getAddressToShow`, there is no unused
address in the AddressBook, and a new one is created.
`getNewAddress` accesses the key pool and returns the first unused key.
Since the used keys have been removed, this is a brand new address.
All addresses relative to the keys removed from the pool in
`MarkUnusedAddresses`, therefore, never enter the AddressBook, and thus
are never shown in the receive widget.

**Solution**
When removing the key from the pool (in `MarkReserveKeysAsUsed`), also
add the missing address to the AddressBook, with receive/coldstaking
purpose.

This way, when restoring an old backup, the user loses only the labels
(that were added after the wallet.dat backup) but can still repopulate
his AddressBook with all the previously used addresses.
Copy link

@furszy furszy left a comment

Choose a reason for hiding this comment

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

ACK 390adc1

Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

ACK 390adc1

@furszy furszy merged commit 995e35d into PIVX-Project:master Jan 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants