destdata entries into CAddressBookData fields
#27215
destdata is a generic strings map in CAddressBookEntry. It is opaque and thus hard to know what is actually being stored in a CAddressBookEntry. 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 of CAddressBookEntry to make it easier to understand what kind of data is being stored.
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.
See 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.
Reviewers, 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.
Each address can only have one receive request, so just store one at a
time.
228 }
229+
230+ bool GetInputUsed() const { return m_input_used; }
231+ void SetInputUsed(bool value) { m_input_used = value; }
232+
233+ const std::optional<std::pair<int64_t, std::string>> GetReceiveRequest() const { return m_receive_request; }
nit: Returning a const value is not possible. You can either return a (mutable) value or a const reference. See also:
�[1m/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/src/./wallet/wallet.h:230:5: �[0m�[0;1;31merror: �[0m�[1mreturn type 'const std::optional<std::pair<int64_t, std::string>>' (aka 'const optional<pair<long, basic_string<char>>>') is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type,-warnings-as-errors]�[0m
const std::optional<std::pair<int64_t, std::string>> GetReceiveRequest() const { return m_receive_request; }
2447@@ -2448,6 +2448,9 @@ bool CWallet::DelAddressBook(const CTxDestination& address)
2448 // Explicitly erase "used" record since we no longer store it in destdata
2449 batch.EraseDestData(strAddress, "used");
2450
2451+ // Explicitly erase "rr" record since we no longer store it in destdata
2452+ batch.EraseDestData(strAddress, "rr");
In commit “wallet: Store “receive requests” as its own member” (2e7751def15fe430daf6c8bea7fca31ff49f71d9)
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 7a05b1dee2fa68b32bfb19e273fb55a5b3836a3e 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.
2443@@ -2444,6 +2444,10 @@ bool CWallet::DelAddressBook(const CTxDestination& address)
2444 {
2445 batch.EraseDestData(strAddress, item.first);
2446 }
2447+
2448+ // Explicitly erase "used" record since we no longer store it in destdata
In commit “wallet: Replace “used” destdata with its own member variable” (861e7f949bb0d94f0ff59c7b74a1b10c85b4a08e)
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.
Concept ACK. This PR is basically doing the same thing as commit 7a05b1dee2fa68b32bfb19e273fb55a5b3836a3e 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:
key.compare(0, rr_prefix.size(), rr_prefix) and ParseInt64 calls where #18608 put this parsing and serialization code more properly (imo) in the walletdb layer.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.