refactor: Remove CAddressBookData::destdata #18608
Conversation
|
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 |
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.)
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.
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. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
|
This approach makes it easy to remove circular dependency recentrequeststablemodel -> walletmodel -> recentrequeststablemodel, see 452ecd7 (https://github.com/promag/bitcoin/tree/2020-04-review-18608). |
There was a problem hiding this comment.
Updated 02c9d2c -> 5a9bbfe (pr/nodest.1 -> pr/nodest.2, compare) fixing memory leak from misuse of SafeDbt and adding missing #include
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 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... |
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.
There are various extension points in the wallet for metadata, and this PR doesn't remove them. This PR removes the
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
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. |
|
Rebased 5a9bbfe -> 6847561 ( |
4ca322b to
e04daae
Compare
Make sure wallet receive requests are saved and deleted correctly by GUI code
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.
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, bitcoin#18608 could be considered as a followup.
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 bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs
|
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. |
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, bitcoin#18608 could be considered as a followup.
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, bitcoin#18608 could be considered as a followup.
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, bitcoin#18608 could be considered as a followup.
|
#27224 is the rebased version of this PR. (I would have reopened this one but ran into github permission issues) |
a5986e8 refactor: Remove CAddressBookData::destdata (Ryan Ofsky) 5938ad0 wallet: Add DatabaseBatch::ErasePrefix method (Ryan Ofsky) Pull request description: This is cleanup that doesn't change external behavior. Benefits of the cleanup are: - Removes awkward `StringMap` intermediate representation for wallet address metadata. - Simplifies `CWallet`, deals with used address and received request serialization in `walletdb.cpp` instead of higher level wallet code - Adds test coverage and documentation This PR doesn't change externally observable behavior. Internally, the only change in behavior is that `EraseDestData` deletes rows directly from the database because they are no longer stored in memory. This is more direct and efficient because it uses a single 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 --- This PR is a rebased copy of #18608. For some reason that PR is locked and couldn't be reopened or commented on. This PR is an alternative to #27215 with differences described in #27215 (review) ACKs for top commit: achow101: ACK a5986e8 furszy: Code ACK a5986e8 Tree-SHA512: 6bd3e402f1f60263fbd433760bdc29d04175ddaf8307207c4a67d59f6cffa732e176ba57886e02926f7a1615dce0ed9cda36c8cbc6430aa8e5b56934c23f3fe7
a5986e8 refactor: Remove CAddressBookData::destdata (Ryan Ofsky) 5938ad0 wallet: Add DatabaseBatch::ErasePrefix method (Ryan Ofsky) Pull request description: This is cleanup that doesn't change external behavior. Benefits of the cleanup are: - Removes awkward `StringMap` intermediate representation for wallet address metadata. - Simplifies `CWallet`, deals with used address and received request serialization in `walletdb.cpp` instead of higher level wallet code - Adds test coverage and documentation This PR doesn't change externally observable behavior. Internally, the only change in behavior is that `EraseDestData` deletes rows directly from the database because they are no longer stored in memory. This is more direct and efficient because it uses a single lookup and scan instead of multiple lookups. Motivation for this cleanup is making changes like bitcoin#18550, bitcoin#18192, bitcoin#13756 easier to reason about and less likely to result in unintended behavior and bugs --- This PR is a rebased copy of bitcoin#18608. For some reason that PR is locked and couldn't be reopened or commented on. This PR is an alternative to bitcoin#27215 with differences described in bitcoin#27215 (review) ACKs for top commit: achow101: ACK a5986e8 furszy: Code ACK a5986e8 Tree-SHA512: 6bd3e402f1f60263fbd433760bdc29d04175ddaf8307207c4a67d59f6cffa732e176ba57886e02926f7a1615dce0ed9cda36c8cbc6430aa8e5b56934c23f3fe7
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:
7a05b1dee2frefactor: Remove CAddressBookData::destdataThis is cleanup that doesn't change external behavior.
StringMapintermediate representationThis PR doesn't change externally observable behavior. Internally, only change in behavior is that EraseDestData deletes directly from database because the
StringMapis gone. This is more direct and efficient because it uses a single btree lookup and scan instead of multiple lookupsMotivation for this cleanup is making changes like #18550, #18192, #13756 easier to reason about and less likely to result in unintended behavior and bugs