Bugfix: Wallet: Safely deal with change in the address book #18192

luke-jr · 2020-02-22T04:38:48Z

In many places, our code assumes that presence in the address book indicates a non-change key, and absence of an entry in mapAddressBook indicates change.

This no longer holds true after #13756 (first released in 0.19) since it added a "used" DestData populated even for change addresses. Only avoid-reuse wallets should be affected by this issue.

Thankfully, populating DestData does not write a label to the database, so we can retroactively fix this (so long as the user didn't see the change address and manually assign it a real label).

Fixing it is accomplished by:

Adding a new bool to CAddressBookData to track if the label has ever been assigned, either by loading one from the database, or by assigning one at runtime.
CAddressBookData::IsChange and CWallet::FindAddressBookEntry are new methods to assist in excluding change from code that doesn't expect to see them.
For safety in merging, CAddressBookData::name has been made read-only (the actual data is stored in m_label, a new private member, and can be changed only with setLabel which updates the m_change flag), and mapAddressBook has been renamed to m_address_book (to force old code to be rebased to compile).

A final commit also does some minor optimisation, avoiding redundant lookups in m_address_book when we already have a pointer to the CAddressBookData.

DrahtBot · 2020-02-22T09:25:33Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

Replace -upgradewallet startup option with upgradewallet RPC #15761 (Replace -upgradewallet startup option with upgradewallet RPC by achow101)
Wallet passive startup #15719 (Drop Chain::requestMempoolTransactions method by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

achow101 · 2020-02-24T19:43:36Z

src/wallet/wallet.h

 public:
-    std::string name;
+    const std::string& name;


Why keep name around at all? I would prefer we get replace it with m_label and have a Getlabel() as well.

Maybe as a follow-up refactor. Just trying to minimally fix the bug here. :)

I agree that we should remove name in this PR. Leaving it here is confusing and makes the code less readable.

It's also a pain to review because we can't just sed it...

Ultimately, it's unrelated to this PR... All confusion/unreadability from it predates this.

See here: c16a57a

Looks good. Please tag me when you open a PR for that branch.

Looks good. Please tag me when you open a PR for that branch.

I agree. Happy to review that change.

src/wallet/wallet.h

instagibbs · 2020-02-25T16:01:24Z

concept ACK

luke-jr · 2020-04-02T16:39:05Z

Rebased, nit addressed

promag

Concept ACK.

promag · 2020-04-02T17:09:36Z

src/wallet/wallet.cpp

+    if ((!allow_change) && address_book_it->second.IsChange()) {
+        return nullptr;
+    }
+    return &address_book_it->second;


Maybe we should return a const reference instead considering we do m_address_book.erase(address); and that would render this pointer invalid.

References have the same validity lifetime as pointers AFAIK?

I mean that if the caller keeps the pointer after releasing cs_wallet then you can get trouble. With reference that wouldn't happen because the reference would be in the lock scope.

ryanofsky

Code review ACK b5795a7. Pretty clever and nicely implemented fix!

jonatack

ACK b5795a7 nice improvements -- code review, built/ran tests rebased on current master ff53433 and tested manually with rpc/cli

src/wallet/wallet.cpp

maflcko · 2020-04-02T22:32:01Z

Would it be possible to write a test that fails before the bugfix?

luke-jr · 2020-04-03T03:05:58Z

Test here: 86d6059

Should I update this PR with it, or leave it for a follow-up?

jonatack · 2020-04-03T15:41:45Z

Test here: 86d6059

Should I update this PR with it, or leave it for a follow-up?

I'm happy to re-review with the test pulled in.

jnewbery

Good fix. utACK b5795a7.

I think the comment for CAddressBookData could be updated to explicitly say that any address that doesn't have an address book entry or that has an entry where the m_label hasn't been set is implicitly change.

There's a related issue where setlabel can be called for a change address and it silently converts it to a 'receive' address. I think that can be fixed in a follow-up PR, perhaps by preventing setlabel to be called for a change address.

src/wallet/wallet.cpp

…oid-reuse wallet

maflcko · 2020-04-06T19:52:13Z

Going to merge this, please submit the test as a follow up. Style cleanups can also go in as follow-ups.

…ddress book b5795a7 Wallet: Add warning comments and assert to CWallet::DelAddressBook (Luke Dashjr) 6d2905f Wallet: Avoid unnecessary/redundant m_address_book lookups (Luke Dashjr) c751d88 Wallet: Avoid treating change-in-the-addressbook as non-change everywhere (Luke Dashjr) 8e64b8c Wallet: New FindAddressBookEntry method to filter out change entries (and skip ->second everywhere) (Luke Dashjr) 65b6bdc Wallet: Add CAddressBookData::IsChange which returns true iff label has never been set (Luke Dashjr) 144b2f8 Wallet: Require usage of new CAddressBookData::setLabel to change label (Luke Dashjr) b86cd15 scripted-diff: Wallet: Rename mapAddressBook to m_address_book (Luke Dashjr) Pull request description: In many places, our code assumes that presence in the address book indicates a non-change key, and absence of an entry in mapAddressBook indicates change. This no longer holds true after bitcoin#13756 (first released in 0.19) since it added a "used" DestData populated even for change addresses. Only avoid-reuse wallets should be affected by this issue. Thankfully, populating DestData does not write a label to the database, so we can retroactively fix this (so long as the user didn't see the change address and manually assign it a real label). Fixing it is accomplished by: * Adding a new bool to CAddressBookData to track if the label has ever been assigned, either by loading one from the database, or by assigning one at runtime. * `CAddressBookData::IsChange` and `CWallet::FindAddressBookEntry` are new methods to assist in excluding change from code that doesn't expect to see them. * For safety in merging, `CAddressBookData::name` has been made read-only (the actual data is stored in `m_label`, a new private member, and can be changed only with `setLabel` which updates the `m_change` flag), and `mapAddressBook` has been renamed to `m_address_book` (to force old code to be rebased to compile). A final commit also does some minor optimisation, avoiding redundant lookups in `m_address_book` when we already have a pointer to the `CAddressBookData`. ACKs for top commit: ryanofsky: Code review ACK b5795a7. Pretty clever and nicely implemented fix! jonatack: ACK b5795a7 nice improvements -- code review, built/ran tests rebased on current master ff53433 and tested manually with rpc/cli jnewbery: Good fix. utACK b5795a7. Tree-SHA512: 40525185a0bcc1723f602243c269499ec86ecb298fecb5ef24d626bbdd5e3efece86cdb1084ad7eebf7eeaf251db4a6e056bcd25bc8457b417fcbb53d032ebf0

This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs

…ddress book b5795a7 Wallet: Add warning comments and assert to CWallet::DelAddressBook (Luke Dashjr) 6d2905f Wallet: Avoid unnecessary/redundant m_address_book lookups (Luke Dashjr) c751d88 Wallet: Avoid treating change-in-the-addressbook as non-change everywhere (Luke Dashjr) 8e64b8c Wallet: New FindAddressBookEntry method to filter out change entries (and skip ->second everywhere) (Luke Dashjr) 65b6bdc Wallet: Add CAddressBookData::IsChange which returns true iff label has never been set (Luke Dashjr) 144b2f8 Wallet: Require usage of new CAddressBookData::setLabel to change label (Luke Dashjr) b86cd15 scripted-diff: Wallet: Rename mapAddressBook to m_address_book (Luke Dashjr) Pull request description: In many places, our code assumes that presence in the address book indicates a non-change key, and absence of an entry in mapAddressBook indicates change. This no longer holds true after bitcoin#13756 (first released in 0.19) since it added a "used" DestData populated even for change addresses. Only avoid-reuse wallets should be affected by this issue. Thankfully, populating DestData does not write a label to the database, so we can retroactively fix this (so long as the user didn't see the change address and manually assign it a real label). Fixing it is accomplished by: * Adding a new bool to CAddressBookData to track if the label has ever been assigned, either by loading one from the database, or by assigning one at runtime. * `CAddressBookData::IsChange` and `CWallet::FindAddressBookEntry` are new methods to assist in excluding change from code that doesn't expect to see them. * For safety in merging, `CAddressBookData::name` has been made read-only (the actual data is stored in `m_label`, a new private member, and can be changed only with `setLabel` which updates the `m_change` flag), and `mapAddressBook` has been renamed to `m_address_book` (to force old code to be rebased to compile). A final commit also does some minor optimisation, avoiding redundant lookups in `m_address_book` when we already have a pointer to the `CAddressBookData`. ACKs for top commit: ryanofsky: Code review ACK b5795a7. Pretty clever and nicely implemented fix! jonatack: ACK b5795a7 nice improvements -- code review, built/ran tests rebased on current master ff53433 and tested manually with rpc/cli jnewbery: Good fix. utACK b5795a7. Tree-SHA512: 40525185a0bcc1723f602243c269499ec86ecb298fecb5ef24d626bbdd5e3efece86cdb1084ad7eebf7eeaf251db4a6e056bcd25bc8457b417fcbb53d032ebf0

This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs

…book Summary: b5795a788639305bab86a8b3f6b75d6ce81be083 Wallet: Add warning comments and assert to CWallet::DelAddressBook (Luke Dashjr) 6d2905f57aaeb3ec3b63d31043f7673ca10003f2 Wallet: Avoid unnecessary/redundant m_address_book lookups (Luke Dashjr) c751d886f499257627b308b11ffaa51c22db6cc0 Wallet: Avoid treating change-in-the-addressbook as non-change everywhere (Luke Dashjr) 8e64b8c84bcbd63caea06f3af087af1f0609eaf5 Wallet: New FindAddressBookEntry method to filter out change entries (and skip ->second everywhere) (Luke Dashjr) 65b6bdc2b164343ec3cc3d32a0297daff9e24fec Wallet: Add CAddressBookData::IsChange which returns true iff label has never been set (Luke Dashjr) 144b2f85da4d51bf7d72b987888ddcaf5b429eed Wallet: Require usage of new CAddressBookData::setLabel to change label (Luke Dashjr) b86cd155f6f661052042048aa7cfc2a397afe4f7 scripted-diff: Wallet: Rename mapAddressBook to m_address_book (Luke Dashjr) Pull request description: In many places, our code assumes that presence in the address book indicates a non-change key, and absence of an entry in mapAddressBook indicates change. This no longer holds true after #13756 (first released in 0.19) since it added a "used" DestData populated even for change addresses. Only avoid-reuse wallets should be affected by this issue. Thankfully, populating DestData does not write a label to the database, so we can retroactively fix this (so long as the user didn't see the change address and manually assign it a real label). Fixing it is accomplished by: * Adding a new bool to CAddressBookData to track if the label has ever been assigned, either by loading one from the database, or by assigning one at runtime. * `CAddressBookData::IsChange` and `CWallet::FindAddressBookEntry` are new methods to assist in excluding change from code that doesn't expect to see them. * For safety in merging, `CAddressBookData::name` has been made read-only (the actual data is stored in `m_label`, a new private member, and can be changed only with `setLabel` which updates the `m_change` flag), and `mapAddressBook` has been renamed to `m_address_book` (to force old code to be rebased to compile). A final commit also does some minor optimisation, avoiding redundant lookups in `m_address_book` when we already have a pointer to the `CAddressBookData`. Backport of Core [[bitcoin/bitcoin#18192 | PR18192]] Tests for this change to follow in [[bitcoin/bitcoin#18546 | PR18546]] Test Plan: `ninja check check-functional` for sanity Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D7810

This is cleanup that doesn't change external behavior. - Removes awkward `StringMap` intermediate representation - Deals with receive request "rr" keys in walletdb.cpp instead of all over qt, wallet, and interfaces code - Deals with destination "used" keys in walletdb.cpp instead of all over wallet code - Adds test coverage - Reduces code (+85/-138 lines) - Reduces memory usage This PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the `StringMap` is gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookups Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs

luke-jr requested review from kallewoof, meshcollider, achow101, jnewbery, laanwj and maflcko February 22, 2020 04:39

fanquake added the Wallet label Feb 22, 2020

DrahtBot mentioned this pull request Feb 22, 2020

Replace -upgradewallet startup option with upgradewallet RPC #15761

Merged

achow101 reviewed Feb 24, 2020

View reviewed changes

DrahtBot mentioned this pull request Mar 2, 2020

wallet/refactor: refer to CWallet immutably when possible #18241

Merged

luke-jr mentioned this pull request Mar 5, 2020

Wallet, GUI: Warn when sending to already-used Bitcoin addresses #15987

Closed

DrahtBot added the Needs rebase label Mar 7, 2020

luke-jr force-pushed the bugfix_addressbook_change branch from bd9d43c to b5795a7 Compare April 2, 2020 16:38

promag reviewed Apr 2, 2020

View reviewed changes

DrahtBot removed the Needs rebase label Apr 2, 2020

laanwj added this to the 0.20.0 milestone Apr 2, 2020

ryanofsky approved these changes Apr 2, 2020

View reviewed changes

jonatack reviewed Apr 2, 2020

View reviewed changes

src/wallet/wallet.cpp Show resolved Hide resolved

DrahtBot mentioned this pull request Apr 2, 2020

Use shared pointers only in validation interface #18354

Closed

DrahtBot mentioned this pull request Apr 3, 2020

Wallet passive startup #15719

Closed

jnewbery reviewed Apr 3, 2020

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

luke-jr added 2 commits April 6, 2020 19:51

Wallet: Add warning comments and check to CWallet::DelAddressBook

79677be

QA: Test that change doesn't turn into non-change when spent in an av…

61af590

…oid-reuse wallet

ryanofsky mentioned this pull request Apr 12, 2020

refactor: Remove CAddressBookData::destdata #18608

Closed

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Bugfix: Wallet: Safely deal with change in the address book #18192

Bugfix: Wallet: Safely deal with change in the address book #18192

Uh oh!

Conversation

luke-jr commented Feb 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Feb 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

instagibbs commented Feb 25, 2020

Uh oh!

luke-jr commented Apr 2, 2020

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Apr 2, 2020

Uh oh!

luke-jr commented Apr 3, 2020

Uh oh!

jonatack commented Apr 3, 2020

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Apr 6, 2020

Uh oh!

Uh oh!

luke-jr commented Feb 22, 2020 •

edited

Loading

DrahtBot commented Feb 22, 2020 •

edited

Loading