wallet: Turn `destdata` entries into `CAddressBookData` fields #27215

achow101 commented at 9:19 PM on March 6, 2023: member

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.

wallet: Replace "used" destdata with its own member variable

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.

861e7f949b

wallet: Store "receive requests" as its own member

Instead of having "receive requests" for an address stored in the opaque
destdata map, explicitly store it in a member of CAddressBookData.

2e7751def1

wallet: Add explicit RemoveAddressReceiveRequest

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.

af54b02812

wallet: Remove unused CAddressBookData destdata

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.

65b349ff28

DrahtBot commented at 9:19 PM on March 6, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	S3RK, ryanofsky

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27224 (refactor: Remove CAddressBookData::destdata by ryanofsky)
#27217 (wallet: Replace use of purpose strings with an enum by achow101)
#26836 (wallet: finish addressbook encapsulation by furszy)
#25620 (wallet: Introduce AddressBookManager by furszy)

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.

DrahtBot added the label Wallet on Mar 6, 2023

S3RK commented at 8:13 AM on March 7, 2023: contributor

Concept ACK

wallet: Change CAddressBookData to contain only one receive request

Each address can only have one receive request, so just store one at a
time.

4b4c535f5a

in src/wallet/wallet.h:230 in 762c40067e outdated

 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; }

maflcko commented at 9:06 AM on March 7, 2023:

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; }

achow101 commented at 3:55 PM on March 7, 2023:

Made it a reference

achow101 force-pushed on Mar 7, 2023

in src/wallet/wallet.cpp:2452 in 2e7751def1 outdated

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");

ryanofsky commented at 4:36 PM on March 7, 2023:

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.

in src/wallet/wallet.cpp:2448 in 861e7f949b outdated

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

ryanofsky commented at 4:51 PM on March 7, 2023:

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.

ryanofsky commented at 5:21 PM on March 7, 2023: contributor

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:

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) and ParseInt64 calls 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.

achow101 commented at 4:58 PM on March 15, 2023: member

Closing in favor of #27224

achow101 closed this on Mar 15, 2023

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 14, 2024