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

luke-jr commented at 4:38 am on February 22, 2020: member

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.

luke-jr requested review from kallewoof on Feb 22, 2020

luke-jr requested review from meshcollider on Feb 22, 2020

luke-jr requested review from achow101 on Feb 22, 2020

luke-jr requested review from jnewbery on Feb 22, 2020

luke-jr requested review from laanwj on Feb 22, 2020

luke-jr requested review from MarcoFalke on Feb 22, 2020

fanquake added the label Wallet on Feb 22, 2020

DrahtBot commented at 9:25 am on February 22, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15761 (Replace -upgradewallet startup option with upgradewallet RPC by achow101)
#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.

in src/wallet/wallet.h:188 in 23c482efba outdated

177@@ -178,14 +178,20 @@ class ReserveDestination
178 /** Address book data */
179 class CAddressBookData
180 {
181+private:
182+    std::string m_label;
183 public:
184-    std::string name;
185+    const std::string& name;

achow101 commented at 7:43 pm on February 24, 2020:

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

luke-jr commented at 9:51 pm on February 24, 2020:

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

jnewbery commented at 7:12 pm on April 2, 2020:

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

luke-jr commented at 2:46 am on April 3, 2020:

It’s also a pain to review because we can’t just sed it…

luke-jr commented at 2:57 am on April 3, 2020:

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

luke-jr commented at 3:05 am on April 3, 2020:

See here: https://github.com/bitcoin/bitcoin/commit/c16a57a2a9f694feaf0020e057fa9468e99045db

jnewbery commented at 3:19 pm on April 3, 2020:

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

jonatack commented at 3:37 pm on April 3, 2020:

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

I agree. Happy to review that change.

in src/wallet/wallet.h:192 in 23c482efba outdated

189+    CAddressBookData() : name(m_label), purpose("unknown") {}
190 
191     typedef std::map<std::string, std::string> StringMap;
192     StringMap destdata;
193+
194+    void setLabel(const std::string& label) {

achow101 commented at 7:45 pm on February 24, 2020:

nit: capital S. SetLabel

instagibbs commented at 4:01 pm on February 25, 2020: member

concept ACK

DrahtBot added the label Needs rebase on Mar 7, 2020

luke-jr force-pushed on Apr 2, 2020

luke-jr commented at 4:39 pm on April 2, 2020: member

Rebased, nit addressed

in src/wallet/wallet.cpp:4124 in b5795a7886 outdated

4117+    const auto& address_book_it = m_address_book.find(dest);
4118+    if (address_book_it == m_address_book.end()) return nullptr;
4119+    if ((!allow_change) && address_book_it->second.IsChange()) {
4120+        return nullptr;
4121+    }
4122+    return &address_book_it->second;

promag commented at 5:09 pm on April 2, 2020:

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

luke-jr commented at 6:29 pm on April 2, 2020:

References have the same validity lifetime as pointers AFAIK?

promag commented at 7:41 pm on April 2, 2020:

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.

promag commented at 5:09 pm on April 2, 2020: member

Concept ACK.

DrahtBot removed the label Needs rebase on Apr 2, 2020

laanwj added this to the milestone 0.20.0 on Apr 2, 2020

ryanofsky approved

ryanofsky commented at 7:26 pm on April 2, 2020: member

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

in src/wallet/wallet.cpp:3216 in b5795a7886 outdated

3212@@ -3212,16 +3213,21 @@ bool CWallet::SetAddressBook(const CTxDestination& address, const std::string& s
3213 
3214 bool CWallet::DelAddressBook(const CTxDestination& address)
3215 {
3216+    // If we want to delete receiving addresses, we need to take care that DestData "used" (and possibly newer DestData) gets preserved (and the "deleted" address transformed into a change entry instead of actually being deleted)

jonatack commented at 8:41 pm on April 2, 2020:

nit: a line wrap here would be welcome if you retouch

jonatack commented at 8:50 pm on April 2, 2020: member

ACK b5795a788639305bab86a8b3f6b75d6ce81be083 nice improvements – code review, built/ran tests rebased on current master ff53433fe4ed06893d7c4 and tested manually with rpc/cli

MarcoFalke commented at 10:32 pm on April 2, 2020: member

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

luke-jr commented at 3:05 am on April 3, 2020: member

Test here: https://github.com/bitcoin/bitcoin/commit/86d6059a10502338e844f6d2330e22c1540b8b0e

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

in src/wallet/wallet.cpp:3219 in b5795a7886 outdated

3212@@ -3212,16 +3213,21 @@ bool CWallet::SetAddressBook(const CTxDestination& address, const std::string& s
3213 
3214 bool CWallet::DelAddressBook(const CTxDestination& address)
3215 {
3216+    // If we want to delete receiving addresses, we need to take care that DestData "used" (and possibly newer DestData) gets preserved (and the "deleted" address transformed into a change entry instead of actually being deleted)
3217+    // NOTE: This isn't a problem for sending addresses because they never have any DestData yet!
3218+    // When adding new DestData, it should be considered here whether to retain or delete it (or move it?).
3219+    assert(!IsMine(address));

jnewbery commented at 3:24 pm on April 3, 2020:

I think this would be safer as a return false. The interface method delAddressBook() doesn’t enforce any conditions on the address book entry type to be deleted, so a buggy wallet client could cause the node to assert. We have a bool return value (which an already be used if the database fails to update the entry), so lets use it.

luke-jr commented at 7:52 pm on April 6, 2020:

Done

jonatack commented at 3:41 pm on April 3, 2020: member

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 commented at 7:45 pm on April 3, 2020: member

Good fix. utACK b5795a788.

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.

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

QA: Test that change doesn't turn into non-change when spent in an avoid-reuse wallet 61af5901a1

MarcoFalke commented at 7:52 pm on April 6, 2020: member

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

MarcoFalke referenced this in commit c5966a87d1 on Apr 6, 2020

luke-jr force-pushed on Apr 6, 2020

MarcoFalke commented at 7:54 pm on April 6, 2020: member

This has been merged, but somehow GitHub is broken (as usual).

MarcoFalke closed this on Apr 6, 2020

MarcoFalke referenced this in commit 63dad67348 on Apr 7, 2020

sidhujag referenced this in commit 9fe8a19478 on Apr 8, 2020

ryanofsky referenced this in commit 02c9d2c78d on Apr 12, 2020

ryanofsky referenced this in commit 5a9bbfe5e7 on Apr 13, 2020

sidhujag referenced this in commit c064c8cba7 on Apr 19, 2020

ryanofsky referenced this in commit 6847561ce1 on Apr 27, 2020

ryanofsky referenced this in commit 0502b35473 on Apr 27, 2020

ryanofsky referenced this in commit 4eea4ac8f3 on May 4, 2020

ryanofsky referenced this in commit 4855c6271c on May 4, 2020

ryanofsky referenced this in commit 6e48213820 on May 27, 2020

ryanofsky referenced this in commit 3a58f2ae17 on May 27, 2020

ryanofsky referenced this in commit f8d7abef76 on Jun 17, 2020

ryanofsky referenced this in commit 005f0dfd16 on Jun 17, 2020

ryanofsky referenced this in commit e44e5a2e33 on Jul 1, 2020

ryanofsky referenced this in commit 6fe1199ee5 on Jul 1, 2020

ryanofsky referenced this in commit 36aa5fee72 on Jul 10, 2020

ryanofsky referenced this in commit 11b08858f3 on Jul 10, 2020

ryanofsky referenced this in commit 487684727b on Jul 13, 2020

ryanofsky referenced this in commit da12e67616 on Jul 14, 2020

ryanofsky referenced this in commit 7aaf995e81 on Jul 14, 2020

ryanofsky referenced this in commit 4ca322b86d on Aug 28, 2020

ryanofsky referenced this in commit 12462bcd54 on Aug 28, 2020

ryanofsky referenced this in commit e04daaeb92 on Sep 13, 2020

ryanofsky referenced this in commit 7fbfefab87 on Sep 13, 2020

jasonbcox referenced this in commit 082ae84501 on Oct 8, 2020

ryanofsky referenced this in commit 793f7e8118 on Jan 21, 2021

ryanofsky referenced this in commit 75897d46a5 on Jan 21, 2021

ryanofsky referenced this in commit 7a05b1dee2 on Mar 3, 2021

ryanofsky referenced this in commit 14b1d5f3e9 on Mar 3, 2021

DrahtBot locked this on Feb 15, 2022

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

Conflicts