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

pull luke-jr wants to merge 2 commits into bitcoin:master from luke-jr:bugfix_addressbook_change changing 3 files +36 −1
  1. 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.

  2. luke-jr requested review from kallewoof on Feb 22, 2020
  3. luke-jr requested review from meshcollider on Feb 22, 2020
  4. luke-jr requested review from achow101 on Feb 22, 2020
  5. luke-jr requested review from jnewbery on Feb 22, 2020
  6. luke-jr requested review from laanwj on Feb 22, 2020
  7. luke-jr requested review from MarcoFalke on Feb 22, 2020
  8. fanquake added the label Wallet on Feb 22, 2020
  9. 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.

  10. 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.
    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.

  11. 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
  12. instagibbs commented at 4:01 pm on February 25, 2020: member
    concept ACK
  13. DrahtBot added the label Needs rebase on Mar 7, 2020
  14. luke-jr force-pushed on Apr 2, 2020
  15. luke-jr commented at 4:39 pm on April 2, 2020: member
    Rebased, nit addressed
  16. 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.
  17. promag commented at 5:09 pm on April 2, 2020: member
    Concept ACK.
  18. DrahtBot removed the label Needs rebase on Apr 2, 2020
  19. laanwj added this to the milestone 0.20.0 on Apr 2, 2020
  20. ryanofsky approved
  21. ryanofsky commented at 7:26 pm on April 2, 2020: member
    Code review ACK b5795a788639305bab86a8b3f6b75d6ce81be083. Pretty clever and nicely implemented fix!
  22. 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
  23. 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
  24. MarcoFalke commented at 10:32 pm on April 2, 2020: member
    Would it be possible to write a test that fails before the bugfix?
  25. 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?

  26. 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
  27. 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.

  28. 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.

  29. Wallet: Add warning comments and check to CWallet::DelAddressBook 79677bed54
  30. QA: Test that change doesn't turn into non-change when spent in an avoid-reuse wallet 61af5901a1
  31. 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.
  32. MarcoFalke referenced this in commit c5966a87d1 on Apr 6, 2020
  33. luke-jr force-pushed on Apr 6, 2020
  34. MarcoFalke commented at 7:54 pm on April 6, 2020: member
    This has been merged, but somehow GitHub is broken (as usual).
  35. MarcoFalke closed this on Apr 6, 2020
  36. MarcoFalke referenced this in commit 63dad67348 on Apr 7, 2020
  37. sidhujag referenced this in commit 9fe8a19478 on Apr 8, 2020
  38. ryanofsky referenced this in commit 02c9d2c78d on Apr 12, 2020
  39. ryanofsky referenced this in commit 5a9bbfe5e7 on Apr 13, 2020
  40. sidhujag referenced this in commit c064c8cba7 on Apr 19, 2020
  41. ryanofsky referenced this in commit 6847561ce1 on Apr 27, 2020
  42. ryanofsky referenced this in commit 0502b35473 on Apr 27, 2020
  43. ryanofsky referenced this in commit 4eea4ac8f3 on May 4, 2020
  44. ryanofsky referenced this in commit 4855c6271c on May 4, 2020
  45. ryanofsky referenced this in commit 6e48213820 on May 27, 2020
  46. ryanofsky referenced this in commit 3a58f2ae17 on May 27, 2020
  47. ryanofsky referenced this in commit f8d7abef76 on Jun 17, 2020
  48. ryanofsky referenced this in commit 005f0dfd16 on Jun 17, 2020
  49. ryanofsky referenced this in commit e44e5a2e33 on Jul 1, 2020
  50. ryanofsky referenced this in commit 6fe1199ee5 on Jul 1, 2020
  51. ryanofsky referenced this in commit 36aa5fee72 on Jul 10, 2020
  52. ryanofsky referenced this in commit 11b08858f3 on Jul 10, 2020
  53. ryanofsky referenced this in commit 487684727b on Jul 13, 2020
  54. ryanofsky referenced this in commit da12e67616 on Jul 14, 2020
  55. ryanofsky referenced this in commit 7aaf995e81 on Jul 14, 2020
  56. ryanofsky referenced this in commit 4ca322b86d on Aug 28, 2020
  57. ryanofsky referenced this in commit 12462bcd54 on Aug 28, 2020
  58. ryanofsky referenced this in commit e04daaeb92 on Sep 13, 2020
  59. ryanofsky referenced this in commit 7fbfefab87 on Sep 13, 2020
  60. jasonbcox referenced this in commit 082ae84501 on Oct 8, 2020
  61. ryanofsky referenced this in commit 793f7e8118 on Jan 21, 2021
  62. ryanofsky referenced this in commit 75897d46a5 on Jan 21, 2021
  63. ryanofsky referenced this in commit 7a05b1dee2 on Mar 3, 2021
  64. ryanofsky referenced this in commit 14b1d5f3e9 on Mar 3, 2021
  65. DrahtBot locked this on Feb 15, 2022


luke-jr DrahtBot achow101 jnewbery jonatack instagibbs promag ryanofsky MarcoFalke


laanwj kallewoof meshcollider MarcoFalke

Labels
Wallet

Milestone
0.20.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-11-09 15:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me