wallet: Turn destdata entries into CAddressBookData fields
#27215
Conversation
Instead of having "used" be an opaque value in the destdata map of a CAddressBookData, use an explicit member variable that can also document what the value represents.
Instead of having "receive requests" for an address stored in the opaque destdata map, explicitly store it in a member of CAddressBookData.
Instead of having a receive request value of the empty string to indicate the deletion of that request, have a function that explicitly does that so that SetAddressReceiveRequest only adds.
The values that were stored in destdata have been moved to their own member variables. Since destdata is unused, remove it. We don't want to keep it around since it is rather opaque and we should not use it in the future.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK |
Each address can only have one receive request, so just store one at a time.
There was a problem hiding this comment.
Concept ACK. This PR is basically doing the same thing as commit 7a05b1d from #18608, which luke-jr objected to because it would make it more difficult to add new metadata to addresses in the wallet. I think the differences between this PR and #18608 are that:
- This PR keeps the LoadDestData WriteDestData EraseDestData functions intact, while #18608 gets rid of them. I think that might make Luke happier, because it means wallet code can easily add new destdata fields in the future. But to me it seems worse because now CWallet has new code making
key.compare(0, rr_prefix.size(), rr_prefix)andParseInt64calls where #18608 put this parsing and serialization code more properly (imo) in the walletdb layer. - #18608 added more test coverage
- #18608 kept wallet behavior deleting address metadata same as before, while this PR changes behavior if there are new metadata fields. Instead of deleting all metadata associated with an address, this erases only the known metadata.
- #18608 kept wallet behavior with receive requests exactly the same as before, while this PR seems to be doing something different hardcoding receive requests numbers (which may be fine, but I'd want to look into it)
I might try to rebase #18608 and see what that looks like for comparison
EDIT: I rebased #18608 and reopened it as #27224, because PR #18608 was locked. I think #27224 is a better alternative to this PR because it keeps behavior the same and doesn't try to do as many things. It doesn't add new warnings about extra destdata fields and leave destdata fields behind when addresses are deleted. It doesn't change Qt code at all, and it keeps receive request semantics exactly the same. It's possible some of the other changes in this PR might make sense on their own, but right now they aren't independently justified and not actually necessary just to get rid of the destdata stringmap. I also think #27224 leaves code in a better final state by simplifying CWallet and putting wallet disk format code in the walletdb layer.
| @@ -2448,6 +2448,9 @@ bool CWallet::DelAddressBook(const CTxDestination& address) | |||
| // Explicitly erase "used" record since we no longer store it in destdata | |||
| batch.EraseDestData(strAddress, "used"); | |||
|
|
|||
| // Explicitly erase "rr" record since we no longer store it in destdata | |||
| batch.EraseDestData(strAddress, "rr"); | |||
There was a problem hiding this comment.
In commit "wallet: Store "receive requests" as its own member" (2e7751d)
Does this work? It seems possibly broken because it will only erase keys that are literally the string "rr", not receive requests that begin with "rr" and are followed by a request id
In 7a05b1d from #18608 I had to deal with this problem by creating an ErasePrefix function, and I also added unit tests to verify the code worked.
| @@ -2444,6 +2444,10 @@ bool CWallet::DelAddressBook(const CTxDestination& address) | |||
| { | |||
| batch.EraseDestData(strAddress, item.first); | |||
| } | |||
|
|
|||
| // Explicitly erase "used" record since we no longer store it in destdata | |||
There was a problem hiding this comment.
In commit "wallet: Replace "used" destdata with its own member variable" (861e7f9)
This comment seems a little crazy because the code is literally reading and writing the "used" record with LoadDestData WriteDestData EraseDestData functions. So saying that the field is not stored in destdata would seem to describe the exact opposite of what the code is doing.
I guess the comment is trying to say that the record is no longer added to the CAddressBookData::destdata struct field, but that field is deleted later in the PR, so that would make the comment immediately obsolete after this PR.
Would suggest dropping "since we no longer store in in destdata" part or dropping the whole comment.
|
Closing in favor of #27224 |
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 bitcoin/bitcoin#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
destdatais a generic strings map inCAddressBookEntry. It is opaque and thus hard to know what is actually being stored in aCAddressBookEntry. It is a map with fixed fields, but these are not documented nor are they immediately obvious when looking at the class definition. This PR changes those to be member variables inside ofCAddressBookEntryto make it easier to understand what kind of data is being stored.