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 cross-referenced this on Apr 12, 2020 from issue Store destdata for change in separate key for backward compatibility by luke-jr
  10. 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.

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

  12. DrahtBot cross-referenced this on Apr 12, 2020 from issue util: Make our stringstream usage locale independent by practicalswift
  13. 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

  14. 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).

  15. ryanofsky force-pushed on Apr 13, 2020
  16. 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

  17. DrahtBot cross-referenced this on Apr 13, 2020 from issue Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101
  18. 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...

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

  20. promag cross-referenced this on Apr 13, 2020 from issue gui: Drop RecentRequestsTableModel dependency to WalletModel by promag
  21. DrahtBot cross-referenced this on Apr 13, 2020 from issue rpc: gui: Don't change behavior based on private keys disabled, instead add new buttons/rpcs/menu items by achow101
  22. DrahtBot cross-referenced this on Apr 15, 2020 from issue gui: Add bumpFeePSBT action instead of changing normal bumpfee behavior by achow101
  23. DrahtBot cross-referenced this on Apr 18, 2020 from issue wallet: Avoid translating RPC errors by maflcko
  24. DrahtBot cross-referenced this on Apr 23, 2020 from issue External signer support - Wallet Box edition by Sjors
  25. DrahtBot cross-referenced this on Apr 23, 2020 from issue UI external signer support (e.g. hardware wallet) by Sjors
  26. DrahtBot added the label Needs rebase on Apr 27, 2020
  27. ryanofsky force-pushed on Apr 27, 2020
  28. 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

  29. DrahtBot removed the label Needs rebase on Apr 27, 2020
  30. DrahtBot cross-referenced this on Apr 28, 2020 from issue Use shared pointers only in validation interface by bvbfan
  31. DrahtBot added the label Needs rebase on May 4, 2020
  32. ryanofsky force-pushed on May 4, 2020
  33. DrahtBot removed the label Needs rebase on May 4, 2020
  34. ryanofsky cross-referenced this on May 8, 2020 from issue Sqlite wallet storage by Sjors
  35. DrahtBot cross-referenced this on May 9, 2020 from issue wallet: Move salvagewallet into wallettool by achow101
  36. DrahtBot cross-referenced this on May 14, 2020 from issue wallet: Refactor the classes in wallet/db.{cpp/h} by achow101
  37. DrahtBot added the label Needs rebase on May 27, 2020
  38. ryanofsky force-pushed on May 27, 2020
  39. DrahtBot removed the label Needs rebase on May 27, 2020
  40. DrahtBot cross-referenced this on May 27, 2020 from issue wallet: Add sqlite as an alternative wallet database and use it for new descriptor wallets by achow101
  41. DrahtBot cross-referenced this on May 29, 2020 from issue wallet: Introduce and use DummyDatabase instead of dummy BerkeleyDatabase by achow101
  42. DrahtBot cross-referenced this on May 29, 2020 from issue refactor: remove ::vpwallets and related global variables by ryanofsky
  43. DrahtBot cross-referenced this on Jun 2, 2020 from issue wallettool: Add dump and createfromdump commands by achow101
  44. DrahtBot cross-referenced this on Jun 6, 2020 from issue [WIP DONOTMERGE] Replace boost with C++17 (std::shared_mutex) by maflcko
  45. DrahtBot cross-referenced this on Jun 16, 2020 from issue wallet: Refactor BerkeleyBatch Read, Write, Erase, and Exists functions into non-template functions by achow101
  46. DrahtBot cross-referenced this on Jun 16, 2020 from issue wallet: move BDB specific classes to bdb.{cpp/h} by achow101
  47. DrahtBot cross-referenced this on Jun 16, 2020 from issue wallet: GetWalletTx and IsMine require cs_wallet lock by promag
  48. DrahtBot cross-referenced this on Jun 16, 2020 from issue Replace boost::filesystem with std::filesystem by kiminuo
  49. DrahtBot added the label Needs rebase on Jun 17, 2020
  50. ryanofsky force-pushed on Jun 17, 2020
  51. DrahtBot removed the label Needs rebase on Jun 17, 2020
  52. DrahtBot cross-referenced this on Jun 17, 2020 from issue wallet: BerkeleyBatch Handle cursor internally by achow101
  53. DrahtBot cross-referenced this on Jun 19, 2020 from issue wallet: Refactor BerkeleyDatabase to introduce DatabaseBatch abstract class by achow101
  54. DrahtBot cross-referenced this on Jun 19, 2020 from issue wallet: Move BerkeleyBatch static functions to BerkeleyDatabase by achow101
  55. DrahtBot cross-referenced this on Jun 20, 2020 from issue wallet: Cleanup and separate BerkeleyDatabase and BerkeleyBatch by achow101
  56. DrahtBot cross-referenced this on Jun 20, 2020 from issue wallet: Introduce WalletDatabase abstract class by achow101
  57. DrahtBot added the label Needs rebase on Jul 1, 2020
  58. ryanofsky force-pushed on Jul 1, 2020
  59. DrahtBot removed the label Needs rebase on Jul 1, 2020
  60. ryanofsky cross-referenced this on Jul 2, 2020 from issue Intermittent CI failure: ThreadSanitizer failure (data race) when running wallet_multiwallet.py --usecli by practicalswift
  61. DrahtBot added the label Needs rebase on Jul 5, 2020
  62. ryanofsky force-pushed on Jul 10, 2020
  63. DrahtBot removed the label Needs rebase on Jul 11, 2020
  64. 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.

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

  66. ryanofsky force-pushed on Jul 13, 2020
  67. 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

  68. DrahtBot added the label Needs rebase on Jul 14, 2020
  69. ryanofsky force-pushed on Jul 14, 2020
  70. DrahtBot removed the label Needs rebase on Jul 14, 2020
  71. DrahtBot cross-referenced this on Jul 29, 2020 from issue Remove wallet.dat path handling from wallet.cpp, rpcwallet.cpp by ryanofsky
  72. DrahtBot cross-referenced this on Aug 20, 2020 from issue wallet: Avoid multiple BerkeleyBatch in DelAddressBook by promag
  73. DrahtBot cross-referenced this on Aug 21, 2020 from issue wallet: Avoid recursive lock in IsTrusted by promag
  74. DrahtBot added the label Needs rebase on Aug 27, 2020
  75. ryanofsky force-pushed on Aug 28, 2020
  76. DrahtBot removed the label Needs rebase on Aug 28, 2020
  77. DrahtBot added the label Needs rebase on Sep 7, 2020
  78. ryanofsky force-pushed on Sep 13, 2020
  79. DrahtBot removed the label Needs rebase on Sep 13, 2020
  80. DrahtBot cross-referenced this on Oct 21, 2020 from issue wallet: Make BDB support optional by achow101
  81. DrahtBot cross-referenced this on Oct 30, 2020 from issue Disable and fix tests for when BDB is not compiled by achow101
  82. DrahtBot added the label Needs rebase on Nov 23, 2020
  83. ryanofsky force-pushed on Jan 21, 2021
  84. DrahtBot removed the label Needs rebase on Jan 21, 2021
  85. RonSherfey approved
  86. test: Add gui test for wallet receive requests
    Make sure wallet receive requests are saved and deleted correctly by GUI
    code
    42d5aefa96
  87. 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
  88. 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
  89. Merge remote-tracking branch 'origin/pull/21353/head' b79a77bf4c
  90. 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
  91. ryanofsky force-pushed on Mar 3, 2021
  92. 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.

  93. ryanofsky closed this on Mar 3, 2021

  94. ryanofsky referenced this in commit 758cbfa37e on Mar 9, 2021
  95. ryanofsky referenced this in commit f5ba424cd4 on May 19, 2021
  96. rebroad referenced this in commit 51b5c6a754 on Jun 23, 2021
  97. bitcoin locked this on Aug 16, 2022
  98. ryanofsky cross-referenced this on Sep 30, 2022 from issue wallet: Load database records in a particular order by achow101
  99. ryanofsky cross-referenced this on Mar 7, 2023 from issue wallet: Turn `destdata` entries into `CAddressBookData` fields by achow101
  100. ryanofsky cross-referenced this on Mar 7, 2023 from issue refactor: Remove CAddressBookData::destdata by ryanofsky
  101. ryanofsky cross-referenced this on Mar 9, 2023 from issue Permission to comment on closed PRs by ryanofsky
  102. bitcoin unlocked this on Mar 9, 2023
  103. 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)

  104. achow101 referenced this in commit 5325a61167 on May 1, 2023
  105. sidhujag referenced this in commit 959e5cd153 on May 1, 2023
  106. bitcoin locked this on Mar 26, 2024

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-05-19 06:54 UTC