refactor: Remove CAddressBookData::destdata #18608

pull ryanofsky wants to merge 5 commits into bitcoin:master from ryanofsky:pr/nodest changing 16 files +196 −140
  1. ryanofsky commented at 6:05 PM on April 12, 2020: contributor

    This PR is replaced by #27224 (because of a permissions issue it was closed and locked and couldn't be reopened)

    This is based on #21353. The non-base commits are:

    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 #18550, #18192, #13756 easier to reason about and less likely to result in unintended behavior and bugs

    <!-- *** Please remove the following help text before submitting: *** Pull requests without a rationale and clear improvement may be closed immediately. -->

    <!-- Please provide clear motivation for your patch and explain how it improves Bitcoin Core user experience or Bitcoin Core developer experience significantly: * Any test improvements or new tests that improve coverage are always welcome. * All other changes should have accompanying unit tests (see `src/test/`) or functional tests (see `test/`). Contributors should note which tests cover modified code. If no tests exist for a region of modified code, new tests should accompany the change. * Bug fixes are most welcome when they come with steps to reproduce or an explanation of the potential issue as well as reasoning for the way the bug was fixed. * Features are welcome, but might be rejected due to design or scope issues. If a feature is based on a lot of dependencies, contributors should first consider building the system outside of Bitcoin Core, if possible. * Refactoring changes are only accepted if they are required for a feature or bug fix or otherwise improve developer experience significantly. For example, most "code style" refactoring changes require a thorough explanation why they are useful, what downsides they have and why they *significantly* improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the [developer notes](/doc/developer-notes.md), stylistic code changes are usually rejected. -->

    <!-- Bitcoin Core has a thorough review process and even the most trivial change needs to pass a lot of eyes and requires non-zero or even substantial time effort to review. There is a huge lack of active reviewers on the project, so patches often sit for a long time. -->

  2. DrahtBot added the label GUI on Apr 12, 2020
  3. DrahtBot added the label Refactoring on Apr 12, 2020
  4. DrahtBot added the label Tests on Apr 12, 2020
  5. DrahtBot added the label Wallet on Apr 12, 2020
  6. fanquake removed the label GUI on Apr 12, 2020
  7. fanquake removed the label Tests on Apr 12, 2020
  8. luke-jr commented at 8:02 PM on April 12, 2020: member

    Concept NACK. This completely breaks down the layering/abstraction here... Simply adding new metadata shouldn't require touching the db layer, and the db layer shouldn't have visibility into stuff like CTxDestination

  9. ryanofsky commented at 9:12 PM on April 12, 2020: contributor

    Simply adding new metadata shouldn't require touching the db layer

    Adding new metadata should touch the walletdb layer because it changes the database format. The reason why #13756 introduced a bug is no one understood how it was changing the database format. I try to look at format changes very closely and I didn't pay any attention at all to that PR because it wasn't modifying walletdb. (Implementation of #13756 would also have been smaller and simpler if it was adding a new walletdb key, in addition to avoiding the bug.)

    the db layer shouldn't have visibility into stuff like CTxDestination

    Where's this coming from? I could pretty easily move the EncodeDestination/DecodeDestination calls out of walletdb, but it's already accepting and returning CKeyMetadata, CPubKey, CKeyMetadata, CMasterKey, CWalletTx values so CTxDestination hardly seems like a bridge too far.

    Concept NACK. This completely breaks down the layering/abstraction here...

    Let me know specifically what's broken. This PR gets rid of complexity and code and should make it safer and easier to make changes extending the database format in the future.

  10. DrahtBot commented at 10:08 PM on April 12, 2020: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  11. in src/qt/recentrequeststablemodel.cpp:148 in 02c9d2c78d outdated 
     141 | @@ -141,7 +142,7 @@ bool RecentRequestsTableModel::removeRows(int row, int count, const QModelIndex
 142 |          for (int i = 0; i < count; ++i)
 143 |          {
 144 |              const RecentRequestEntry* rec = &list[row+i];
 145 | -            if (!walletModel->saveReceiveRequest(rec->recipient.address.toStdString(), rec->id, ""))
 146 | +            if (!walletModel->wallet().saveReceiveRequest(DecodeDestination(rec->recipient.address.toStdString()), ToString(rec->id), ""))
    promag commented at 12:46 AM on April 13, 2020:

    Should #include <interfaces/wallet.h>.

    ryanofsky commented at 2:14 AM on April 13, 2020:

    re: #18608 (review)

    Should #include <interfaces/wallet.h>.

    Thanks, updated

  12. promag commented at 12:59 AM on April 13, 2020: member

    This approach makes it easy to remove circular dependency recentrequeststablemodel -> walletmodel -> recentrequeststablemodel, see 452ecd7c6048e2799d8ba0dedf6e41843e397614 (https://github.com/promag/bitcoin/tree/2020-04-review-18608).

  13. ryanofsky force-pushed on Apr 13, 2020
  14. ryanofsky commented at 2:30 AM on April 13, 2020: contributor

    Updated 02c9d2c78d8c361f247b33f4f09d0277ee8565c6 -> 5a9bbfe5e7f717426ee573e4a0059f38a834efb4 (pr/nodest.1 -> pr/nodest.2, compare) fixing memory leak from misuse of SafeDbt and adding missing #include

  15. luke-jr commented at 3:45 AM on April 13, 2020: member

    Adding new metadata should touch the walletdb layer because it changes the database format.

    Not usually, no.

    And do we really want db-level wrapper functions for every single piece of metadata?? That's exactly the kind of thing destdata was supposed to help avoid...

    The reason why #13756 introduced a bug is no one understood how it was changing the database format. I try to look at format changes very closely and I didn't pay any attention at all to that PR because it wasn't modifying walletdb. (Implementation of #13756 would also have been smaller and simpler if it was adding a new walletdb key, in addition to avoiding the bug.)

    The bug was completely unrelated to the database format. Even if the wallet was left in RAM, and never written to a database (not possible with our code, just hypothetical), the bug would have remained... And simply using a new database key would not have fixed it either...

  16. ryanofsky commented at 5:13 AM on April 13, 2020: contributor

    Adding new metadata should touch the walletdb layer because it changes the database format.

    Not usually, no.

    This discussion is too handwavy and general to be productive, but if you believe the PR creates problems here, you should be able to provide examples of database format changes that should bypass walletdb and that this PR will make more difficult.

    And do we really want db-level wrapper functions for every single piece of metadata?? That's exactly the kind of thing destdata was supposed to help avoid...

    There are various extension points in the wallet for metadata, and this PR doesn't remove them. This PR removes the CAddressBookData::destdata extension point because it was badly implemented and misused, already caused a bug, and is a loaded gun waiting to cause more bugs. Your PR #18550 removes the loaded gun by complicating the destdata implementation. But I objected to #18550 because of other changes it bundles, and the complexity it adds turns out not to be necessary here because we can just get rid of the destdata field while simplifying code and not changing behavior.

    The reason why #13756 introduced a bug is no one understood how it was changing the database format. I try to look at format changes very closely and I didn't pay any attention at all to that PR because it wasn't modifying walletdb. (Implementation of #13756 would also have been smaller and simpler if it was adding a new walletdb key, in addition to avoiding the bug.)

    The bug was completely unrelated to the database format. Even if the wallet was left in RAM, and never written to a database (not possible with our code, just hypothetical), the bug would have remained...

    I'd like to interpret this charitably since it is vague and talking about a hypothetical alternate implementation of #13756, but I think this is basically nonsense. If CAddressBookData::destdata field didn't exist, #13756 wouldn't have store used data in CAddressBookData. It would have added something like ("useddata" << EncodeDestionation(dest)) rows and a std::set<CTxDestination> m_used_dests CWallet member, avoiding the bug and requiring less code to implement.

    And simply using a new database key would not have fixed it either...

    Not only would using a non-DESTDATA database key have been sufficient to fix it in the most likely scenario (described above) for the 0.19 release, but using a non-DESTDATA key would have been necessary to fix it for all pre-0.19 releases, because they treat addresses with DESTDATA rows as non-change.

    Really, Luke, DESTDATA is garbage. This PR isolates it to the lowest layer of walletdb, reducing complexity in upper layers and preventing them from misusing it again.

    If you want to add new metadata fields, even new extensible metadata fields, nothing in the PR prevents it or makes it more difficult. I know you're claiming otherwise, but it's not true and you haven't provided plausible examples or specifics to back up your assertions. I promise this PR isn't trying to rain on your metadata parade.

  17. DrahtBot added the label Needs rebase on Apr 27, 2020
  18. ryanofsky force-pushed on Apr 27, 2020
  19. ryanofsky commented at 6:39 PM on April 27, 2020: contributor

    Rebased 5a9bbfe5e7f717426ee573e4a0059f38a834efb4 -> 6847561ce15ec723315abb28279fa69e1f4d8f09 (pr/nodest.2 -> pr/nodest.3, compare) due to conflict with #16528 Rebased 6847561ce15ec723315abb28279fa69e1f4d8f09 -> 4855c6271c731c27cb8a65f8b64e5167e88dc014 (pr/nodest.3 -> pr/nodest.4, compare) due to conflict with #18699 Rebased 4855c6271c731c27cb8a65f8b64e5167e88dc014 -> 3a58f2ae17fbb016cbcda2273be28d7c1a43b0f7 (pr/nodest.4 -> pr/nodest.5, compare) due to conflict with #18918 Rebased 3a58f2ae17fbb016cbcda2273be28d7c1a43b0f7 -> 005f0dfd1628fcd96a1a6828599fb213b8ec25ea (pr/nodest.5 -> pr/nodest.6, compare) due to conflict with #19290 Rebased 005f0dfd1628fcd96a1a6828599fb213b8ec25ea -> e44e5a2e33243fc7e678dc34586999ef18d82435 (pr/nodest.6 -> pr/nodest.7, compare) due to conflict with #19308 Rebased e44e5a2e33243fc7e678dc34586999ef18d82435 -> 36aa5fee72a1a3a192f98215609b33f9ab644ac4 (pr/nodest.7 -> pr/nodest.8, compare) due to conflict with #18918, and after #19422 workaround to avoid TSAN closedir BerkeleyBatch error https://travis-ci.org/github/bitcoin/bitcoin/jobs/703992618

  20. DrahtBot removed the label Needs rebase on Apr 27, 2020
  21. DrahtBot added the label Needs rebase on May 4, 2020
  22. ryanofsky force-pushed on May 4, 2020
  23. DrahtBot removed the label Needs rebase on May 4, 2020
  24. DrahtBot added the label Needs rebase on May 27, 2020
  25. ryanofsky force-pushed on May 27, 2020
  26. DrahtBot removed the label Needs rebase on May 27, 2020
  27. DrahtBot added the label Needs rebase on Jun 17, 2020
  28. ryanofsky force-pushed on Jun 17, 2020
  29. DrahtBot removed the label Needs rebase on Jun 17, 2020
  30. DrahtBot added the label Needs rebase on Jul 1, 2020
  31. ryanofsky force-pushed on Jul 1, 2020
  32. DrahtBot removed the label Needs rebase on Jul 1, 2020
  33. DrahtBot added the label Needs rebase on Jul 5, 2020
  34. ryanofsky force-pushed on Jul 10, 2020
  35. DrahtBot removed the label Needs rebase on Jul 11, 2020
  36. in src/qt/recentrequeststablemodel.cpp:13 in 36aa5fee72 outdated 
       9 | @@ -10,18 +10,20 @@
  10 |  #include <qt/walletmodel.h>
  11 |  
  12 |  #include <clientversion.h>
  13 | +#include <key_io.h>
    promag commented at 10:12 PM on July 12, 2020:

    nit, sort 🙈

    ryanofsky commented at 12:29 PM on July 13, 2020:

    re: #18608 (review)

    nit, sort

    Thanks. Wonder if I'm slowly forgetting the alphabet.

  37. in src/wallet/bdb.cpp:660 in 36aa5fee72 outdated 
     715 | @@ -716,7 +716,7 @@ bool BerkeleyBatch::StartCursor()
 716 |      assert(!m_cursor);
 717 |      if (!pdb)
 718 |          return false;
 719 | -    int ret = pdb->cursor(nullptr, &m_cursor, 0);
 720 | +    int ret = pdb->cursor(activeTxn, &m_cursor, 0);
    promag commented at 10:17 PM on July 12, 2020:

    Is this a bugfix?

    ryanofsky commented at 12:28 PM on July 13, 2020:

    re: #18608 (review)

    Is this a bugfix?

    It's needed to avoid errors if a transaction and a cursor are active at the same time. This didn't used to happen so it wasn't really a bug, but this change is more correct, and necessary for the new erase function.

  38. ryanofsky force-pushed on Jul 13, 2020
  39. ryanofsky commented at 12:51 PM on July 13, 2020: contributor

    Updated 36aa5fee72a1a3a192f98215609b33f9ab644ac4 -> 487684727bb932bf6451e086077b21c88d7fcd70 (pr/nodest.8 -> pr/nodest.9, compare) with suggestion Rebased 487684727bb932bf6451e086077b21c88d7fcd70 -> da12e67616ce47c1a326f45193ad3ffb5492d849 (pr/nodest.9 -> pr/nodest.10, compare) due to conflict with #19325 Rebased da12e67616ce47c1a326f45193ad3ffb5492d849 -> 4ca322b86d8c7091d4055bcbe4a4f47225bdeefe (pr/nodest.10 -> pr/nodest.11, compare) due to conflict with #19289 Rebased 4ca322b86d8c7091d4055bcbe4a4f47225bdeefe -> e04daaeb92f8a2108dbe520e2fecc983172cf7d5 (pr/nodest.11 -> pr/nodest.12, compare) due to conflicts with #19619 and #19738 Rebased e04daaeb92f8a2108dbe520e2fecc983172cf7d5 -> 793f7e8118d1fad69d03e47e2d5760a211bcb4d6 (pr/nodest.12 -> pr/nodest.13, compare) due to conflict with #20480 Rebased 793f7e8118d1fad69d03e47e2d5760a211bcb4d6 -> 7a05b1dee2fa68b32bfb19e273fb55a5b3836a3e (pr/nodest.13 -> pr/nodest.14, compare) on top of #21353, due to various conflicts

  40. DrahtBot added the label Needs rebase on Jul 14, 2020
  41. ryanofsky force-pushed on Jul 14, 2020
  42. DrahtBot removed the label Needs rebase on Jul 14, 2020
  43. DrahtBot added the label Needs rebase on Aug 27, 2020
  44. ryanofsky force-pushed on Aug 28, 2020
  45. DrahtBot removed the label Needs rebase on Aug 28, 2020
  46. DrahtBot added the label Needs rebase on Sep 7, 2020
  47. ryanofsky force-pushed on Sep 13, 2020
  48. DrahtBot removed the label Needs rebase on Sep 13, 2020
  49. DrahtBot added the label Needs rebase on Nov 23, 2020
  50. ryanofsky force-pushed on Jan 21, 2021
  51. DrahtBot removed the label Needs rebase on Jan 21, 2021
  52. RonSherfey approved
  53. test: Add gui test for wallet receive requests
    Make sure wallet receive requests are saved and deleted correctly by GUI
code
    42d5aefa96
  54. interfaces: Stop exposing wallet destdata to gui
    Stop giving GUI access to destdata rows in database. Replace with narrow
API just for saving and reading receive request information.

This simplifies code and should prevent the GUI from interfering with
other destdata like address-used status.

Note: No user-visible behavior is changing in this commit. New
CWallet::SetAddressReceiveRequest() implementation avoids a bug in
CWallet::AddDestData() where a modification would leave the previous
value in memory while writing the new value to disk. But it doesn't
matter because the GUI doesn't currently expose the ability to modify
receive requests, only to add and erase them.
    dd8a9d019a
  55. wallet: Add IsAddressUsed / SetAddressUsed methods
    This simplifies code and adds a less cumbersome interface for accessing
address used information than CWallet AddDestData / EraseDestData /
GetDestData methods.

There is no change in behavior. Lower-level walletdb DestData methods
are also still available and not affected by this change. If there is
interest in consolidating destdata logic more and making it internal to
walletdb, #18608 could be considered as a followup.
    53291c7125
  56. Merge remote-tracking branch 'origin/pull/21353/head' b79a77bf4c
  57. refactor: Remove CAddressBookData::destdata
    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 #18550, #18192, #13756 easier to reason about and less likely to result in unintended behavior and bugs
    7a05b1dee2
  58. ryanofsky force-pushed on Mar 3, 2021
  59. ryanofsky commented at 8:13 PM on March 3, 2021: contributor

    Closing this in favor or more narrow #21353. which doesn't fully consolidate destdata code in walletdb, but does at least remove it from the gui. Can re-evaluate this later if it's still appropriate.

  60. ryanofsky closed this on Mar 3, 2021
  61. ryanofsky referenced this in commit 758cbfa37e on Mar 9, 2021
  62. ryanofsky referenced this in commit f5ba424cd4 on May 19, 2021
  63. rebroad referenced this in commit 51b5c6a754 on Jun 23, 2021
  64. bitcoin locked this on Aug 16, 2022
  65. bitcoin unlocked this on Mar 9, 2023
  66. ryanofsky commented at 4:42 PM on March 9, 2023: contributor

    #27224 is the rebased version of this PR. (I would have reopened this one but ran into github permission issues)

  67. achow101 referenced this in commit 5325a61167 on May 1, 2023
  68. sidhujag referenced this in commit 959e5cd153 on May 1, 2023
  69. bitcoin locked this on Mar 26, 2024
Contributors
ryanofskyluke-jrDrahtBotpromagRonSherfey
Labels
RefactoringWallet
Linked (view graph)
#13756 wallet: "avoid_reuse" wallet flag for improved privacy#18192 Bugfix: Wallet: Safely deal with change in the address book#18550 Store destdata for change in separate key for backward compatibility#21353 interfaces: Stop exposing wallet destdata to gui#27224 refactor: Remove CAddressBookData::destdata#27234 Permission to comment on closed PRs
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: 2026-04-25 00:14 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me