refactor: Remove CAddressBookData::destdata

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:

7a05b1dee2f 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

DrahtBot added the label GUI on Apr 12, 2020

DrahtBot added the label Refactoring on Apr 12, 2020

DrahtBot added the label Tests on Apr 12, 2020

DrahtBot added the label Wallet on Apr 12, 2020

fanquake removed the label GUI on Apr 12, 2020

fanquake removed the label Tests on Apr 12, 2020

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

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.

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

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

Conflicts

No conflicts as of last run.

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 0: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

promag commented at 0: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).

ryanofsky force-pushed on Apr 13, 2020

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

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…

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.

DrahtBot added the label Needs rebase on Apr 27, 2020

ryanofsky force-pushed on Apr 27, 2020

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

DrahtBot removed the label Needs rebase on Apr 27, 2020

DrahtBot added the label Needs rebase on May 4, 2020

ryanofsky force-pushed on May 4, 2020

DrahtBot removed the label Needs rebase on May 4, 2020

DrahtBot added the label Needs rebase on May 27, 2020

ryanofsky force-pushed on May 27, 2020

DrahtBot removed the label Needs rebase on May 27, 2020

DrahtBot added the label Needs rebase on Jun 17, 2020

ryanofsky force-pushed on Jun 17, 2020

DrahtBot removed the label Needs rebase on Jun 17, 2020

DrahtBot added the label Needs rebase on Jul 1, 2020

ryanofsky force-pushed on Jul 1, 2020

DrahtBot removed the label Needs rebase on Jul 1, 2020

DrahtBot added the label Needs rebase on Jul 5, 2020

ryanofsky force-pushed on Jul 10, 2020

DrahtBot removed the label Needs rebase on Jul 11, 2020

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.

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.

ryanofsky force-pushed on Jul 13, 2020

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

DrahtBot added the label Needs rebase on Jul 14, 2020

ryanofsky force-pushed on Jul 14, 2020

DrahtBot removed the label Needs rebase on Jul 14, 2020

DrahtBot added the label Needs rebase on Aug 27, 2020

ryanofsky force-pushed on Aug 28, 2020

DrahtBot removed the label Needs rebase on Aug 28, 2020

DrahtBot added the label Needs rebase on Sep 7, 2020

ryanofsky force-pushed on Sep 13, 2020

DrahtBot removed the label Needs rebase on Sep 13, 2020

DrahtBot added the label Needs rebase on Nov 23, 2020

ryanofsky force-pushed on Jan 21, 2021

DrahtBot removed the label Needs rebase on Jan 21, 2021

RonSherfey approved

test: Add gui test for wallet receive requests

Make sure wallet receive requests are saved and deleted correctly by GUI
code

42d5aefa96

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

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

Merge remote-tracking branch 'origin/pull/21353/head' b79a77bf4c

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

ryanofsky force-pushed on Mar 3, 2021

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.

ryanofsky closed this on Mar 3, 2021

ryanofsky referenced this in commit 758cbfa37e on Mar 9, 2021

ryanofsky referenced this in commit f5ba424cd4 on May 19, 2021

rebroad referenced this in commit 51b5c6a754 on Jun 23, 2021

bitcoin locked this on Aug 16, 2022

bitcoin unlocked this on Mar 9, 2023

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)

achow101 referenced this in commit 5325a61167 on May 1, 2023

sidhujag referenced this in commit 959e5cd153 on May 1, 2023

bitcoin locked this on Mar 26, 2024

refactor: Remove CAddressBookData::destdata #18608

Conflicts