Commits decoupled from #28574, focused on the address book cloning process
Includes:
CWallet::DelAddressBook and Wallet::SetAddrBookWithDB methods.These changes will let us consolidate all individual write operations that take place during the wallet migration process into a single db txn in the future.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | achow101, josibake |
| Concept ACK | rserranon, hernanmarino |
| Approach ACK | w0xlt |
| Stale ACK | pablomartin4btc |
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.
479@@ -480,6 +480,12 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
480
481 //! Adds a destination data tuple to the store, without saving it to disk
482 void LoadDestData(const CTxDestination& dest, const std::string& key, const std::string& value) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
483+ //! Adds destination label to the store, without saving it to disk
484+ void LoadAddrbookEntryLabel(const CTxDestination& dest, const std::string& label) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
LoadAddrBookEntryLablel and LoadAddrBookEntryPurpose as in GetAddrBookSize and ListAddrBookAddreses ?
2393@@ -2394,31 +2394,42 @@ bool CWallet::SetAddressBook(const CTxDestination& address, const std::string& s
2394
2395 bool CWallet::DelAddressBook(const CTxDestination& address)
2396 {
2397- bool is_mine;
2398+ isminetype is_mine;
2399+ const std::string& dest = EncodeDestination(address);
Concept ACK,
I understand this may become a separate commit, but why not encapsulate the other calls to wallet->m_address_book[addr_pair.first].purpose = purpose ?
0src/wallet/wallet.cpp:3980: data.watchonly_wallet->m_address_book[addr_pair.first].purpose = purpose;
1src/wallet/wallet.cpp:3996: data.solvable_wallet->m_address_book[addr_pair.first].purpose = purpose;
2src/wallet/wallet.cpp:4017: data.watchonly_wallet->m_address_book[addr_pair.first].purpose = purpose;
3src/wallet/wallet.cpp:4030: data.solvable_wallet->m_address_book[addr_pair.first].purpose = purpose;
If m_address_book has been encapsulated, why not make it private or protected?
Thanks @rserranon.
why not encapsulate the other calls to wallet->m_address_book[addr_pair.first].purpose = purpose ?
Good catch. I did this changes prior the wallet migration PR merge. Will add those too and make the addressbook member private so we don’t keep adding more in the future.
Updated per feedback. Now we no longer access the wallet’s addressbook member externally.
Plus, improvements to the address book migration process:
tAck
4058- data.solvable_wallet->m_address_book[addr_pair.first].SetLabel(label);
4059+ for (const auto& [dest, record] : m_address_book) {
4060+ // Append "receive" data to other wallets only if not mine.
4061+ // Labels for everything else ("send") should be cloned to all.
4062+ bool require_is_mine = record.purpose == "receive" && !IsMine(dest);
4063+ bool copied = false;
nit:
0 bool require_is_mine{record.purpose == "receive" && !IsMine(dest)};
1 bool copied{false};
2409@@ -2394,31 +2410,42 @@ bool CWallet::SetAddressBook(const CTxDestination& address, const std::string& s
2410
2411 bool CWallet::DelAddressBook(const CTxDestination& address)
2412 {
2413- bool is_mine;
2414+ isminetype is_mine;
2415+ const std::string& dest = EncodeDestination(address);
0 const std::string& dest{EncodeDestination(address)};
bitcoin-qt.
This PR ends up doing a lot of different things that are only tangentially related. I think it could be split up into a few smaller PRs, e.g. one for the migration fix, one for the migration refactoring, and one for the other refactors.
sure, will work on it.
2384@@ -2385,6 +2385,7 @@ bool CWallet::SetAddressBook(const CTxDestination& address, const std::string& s
2385
in “wallet: clean redundancies in DelAddressBook” (https://github.com/bitcoin/bitcoin/pull/26836/commits/da3b8f927c2da5b7761e36fe548eb8ade29171d7):
nit: your commit message mentions “Call IsMine only once (instead of two)” but I don’t see any IsMine related changes in the commit?
IsMine calls, and I forgot to update the commit description after the rebase. Will clean it, thanks.
2367@@ -2368,13 +2368,21 @@ bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& add
2368 purpose = m_address_book[address].purpose;
2369 }
2370 }
2371+
2372+ if (new_purpose && !batch.WritePurpose(EncodeDestination(address), PurposeToString(*new_purpose))) {
2373+ WalletLogPrintf("%s error writing purpose\n", __func__);
2374+ return false;
2375+ }
2376+ if (!batch.WriteName(EncodeDestination(address), strName)) {
in “refactor: SetAddrBookWithDB, signal only if write succeeded” (https://github.com/bitcoin/bitcoin/pull/26836/commits/0b83c8691b277d407e61a8ffca807e65866f4ded):
Maybe do encoded_dest = EncodeDestination(address) and then use that to avoid calling EncodeDestination twice?
2358@@ -2359,14 +2359,15 @@ bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& add
2359 {
2360 LOCK(cs_wallet);
2361 std::map<CTxDestination, CAddressBookData>::iterator mi = m_address_book.find(address);
2362- fUpdated = (mi != m_address_book.end() && !mi->second.IsChange());
2363- m_address_book[address].SetLabel(strName);
2364+ fUpdated = mi != m_address_book.end() && !mi->second.IsChange();
in “refactor: SetAddressBookWithDB, minimize number of map lookups” (https://github.com/bitcoin/bitcoin/pull/26836/commits/10f9639fd6332cdd440f540c2202082044aab98c):
nit: unnecessary formatting change
4038+
4039+ copied = true;
4040+ // Only delete 'receive' records that are no longer part of the original wallet
4041+ if (require_transfer) {
4042+ dests_to_delete.push_back(dest);
4043+ break;
in “refactor: wallet, simplify addressbook migration” (https://github.com/bitcoin/bitcoin/pull/26836/commits/8b331613bf79ab614850e71a603dd444d6bb3a48):
This is a pretty thorny block of code with all of the booleans, let me summarize just to make sure I’m following:
dests_to_delete. Do not check the next wallet. I’m assuming the assumption here is it can only belong to one of the wallets, and if it does end up happening to belong to both wallets this is not an issue?Might be worth adding a comment explaining why its okay to break early? That’s the part that kinda threw me.
If the destination requires_transfer but isn’t ours, go to the next wallet. If this happens for both wallets, copy will never be set and we will fail
If the destination requires transfer (a “receive” addr that is no longer part of the main wallet), the process checks to which wallet the entry belongs to. And, when the entry does not belong to any of the created wallets, it means that the key/script-to-descriptor migration process failed. This is because “receive” entries must always be tracked by the wallet.
If it requires transfer and does belong to the current wallet in the loop, continue and set copy = true, and add the destination to dests_to_delete. Do not check the next wallet. I’m assuming the assumption here is it can only belong to one of the wallets, and if it does end up happening to belong to both wallets this is not an issue?
Yes, thats the existing assumption. If you check the previous code, will see that I did not change this part of the flow on purpose. This is what we had before:
0if (purpose == "RECEIVE" && not_mine_anymore) {
1
2 // check if it is part of the watch-only wallet
3 if (watch_only_wallet.IsMine(dest)) {
4 store_address(watch_only_wallet, dest);
5 add_to_pending_to_delete(dest);
6 continue; // --> this skips the next wallet.
7 }
8
9 // check if it is part of the solvables wallet
10 if (solvables_wallet.IsMine(dest)) {
11 store_address(solvables_wallet, dest);
12 add_to_pending_to_delete(dest);
13 continue; // --> skips any following-up check
14 }
15
16 // We only reach here if the entry wasn't added to any wallet
17 error out...
18}
The only diff between the new code and the previous one is that we had a continue instead of a break statement.
Might be worth adding a comment explaining why it’s okay to break early? That’s the part that kinda threw me.
We should double-check if we could ever have the same no-longer ours “receive” entry in both wallets (the watch-only and the solvables ones). If we find a way to achieve this, then the bug predates this PR.
2393@@ -2394,8 +2394,13 @@ bool CWallet::SetAddressBook(const CTxDestination& address, const std::string& s
2394
2395 bool CWallet::DelAddressBook(const CTxDestination& address)
2396 {
2397- const std::string& dest = EncodeDestination(address);
2398 WalletBatch batch(GetDatabase());
2399+ return DelAddressBookWithDb(batch, address);
2400+}
2401+
2402+bool CWallet::DelAddressBookWithDb(WalletBatch& batch, const CTxDestination& address)
in “wallet: migration, batch addressbook records removal” (https://github.com/bitcoin/bitcoin/pull/26836/commits/3aff6d5b4f3fd32ad1285b259db0ef1580518e39):
nit: most other places in the codebase we use DB in functions names (as opposed to Db)
4068- if (data.solvable_wallet) {
4069- LOCK(data.solvable_wallet->cs_wallet);
4070- // Add to the solvable. Copy the entire address book entry
4071- data.solvable_wallet->m_address_book[addr_pair.first] = addr_pair.second;
4072+ // Skip invalid/non-watched scripts that will not be migrated
4073+ if (not_migrated_dests.count(dest) > 0) {
in “refactor: wallet, simplify addressbook migration” (https://github.com/bitcoin/bitcoin/pull/26836/commits/8b331613bf79ab614850e71a603dd444d6bb3a48):
nit: C++20 has .contains(arg) for sets (yay!). No performance difference, but semantically I think it makes a lot more sense to use when testing for existence.
crACK https://github.com/bitcoin/bitcoin/pull/26836/commits/3aff6d5b4f3fd32ad1285b259db0ef1580518e39
Looks good! Code reviewed, didn’t verify the bench numbers. Left some nits, feel free to ignore
reACK https://github.com/bitcoin/bitcoin/pull/26836/commits/238993ea40c1c384e9fbcf83bfcf45efc73ad1bb
verified the diff, thanks for taking the suggestions!
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
2367@@ -2368,13 +2368,22 @@ bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& add
2368 purpose = m_address_book[address].purpose;
2369 }
2370 }
2371+
2372+ const std::string& encoded_dest = EncodeDestination(address);
2373+ if (new_purpose && !batch.WritePurpose(encoded_dest, PurposeToString(*new_purpose))) {
2374+ WalletLogPrintf("%s error writing entry purpose\n", __func__);
In c154e0940b6518487e29de96eb24ecf1839bd3fe “refactor: SetAddrBookWithDB, signal only if write succeeded”
nit: __func__ is not necessary
__func__ removed.
2373+ if (new_purpose && !batch.WritePurpose(encoded_dest, PurposeToString(*new_purpose))) {
2374+ WalletLogPrintf("%s error writing entry purpose\n", __func__);
2375+ return false;
2376+ }
2377+ if (!batch.WriteName(encoded_dest, strName)) {
2378+ WalletLogPrintf("%s error writing entry name\n", __func__);
In c154e0940b6518487e29de96eb24ecf1839bd3fe “refactor: SetAddrBookWithDB, signal only if write succeeded”
nit: __func__ is not necessary
__func__ removed.
1) Encode destination only once (instead of three).
2) Fail if the entry's linked data cannot be removed.
3) Don't remove entry from memory if db write fail.
4) Notify GUI only if removal succeeded
Same process written in a cleaner manner.
Removing code duplication.
Optimizing the process performance and consistency.
Instead of doing one db transaction per removed record,
we now batch all removals in a single db transaction.
Speeding up the process and preventing the wallet from entering
an inconsistent state when any of the intermediate writes fail.
Updated per feedback. Thanks achow!. Small diff.
Two changes:
reACK https://github.com/bitcoin/bitcoin/commit/86960cdb7f75eaa2ae150914c54240d1d5ef96d1
Verified the diff, looks good.
2358@@ -2359,14 +2359,15 @@ bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& add
2359 {
2360 LOCK(cs_wallet);
2361 std::map<CTxDestination, CAddressBookData>::iterator mi = m_address_book.find(address);
2362- fUpdated = (mi != m_address_book.end() && !mi->second.IsChange());
2363- m_address_book[address].SetLabel(strName);
2364+ fUpdated = mi != m_address_book.end() && !mi->second.IsChange();
2365+
2366+ CAddressBookData& record = mi != m_address_book.end() ? mi->second : m_address_book[address];
2367+ record.SetLabel(strName);
In commit “refactor: SetAddressBookWithDB, minimize number of map lookups” (d0943315b1d00905fe7f4513b2f3f47b88a99e8f)
This isn’t actually minimizing the number of lookups since it is doing two lookups in the insert case. Would replace:
0std::map<CTxDestination, CAddressBookData>::iterator mi = m_address_book.find(address);
1fUpdated = mi != m_address_book.end() && !mi->second.IsChange();
2CAddressBookData& record = mi != m_address_book.end() ? mi->second : m_address_book[address];
with:
0auto [it, inserted] = m_address_book.emplace(std::piecewise_construct, std::forward_as_tuple(address), std::tuple{});
1fUpdated = !inserted && !it->second.IsChange();
2CAddressBookData& record = it->second;
4033- }
4034- }
4035+ for (const auto& [dest, record] : m_address_book) {
4036+ // Ensure "receive" entries that are no longer part of the original wallet are transferred to another wallet
4037+ // Entries for everything else ("send") will be cloned to all wallets.
4038+ bool require_transfer = record.purpose == AddressPurpose::RECEIVE && !IsMine(dest);
In commit “refactor: wallet, simplify addressbook migration” (595bbe6e81885d35179aba6137dc63d0e652cc1f)
IMO, this would be less confusing if require_transfer bool was replaced by an copy_to_all bool with the opposite meaning, because the thing require_transfer is used for in the loop below is to determining whether the address book entry is copied to all wallets or is moved from the *this wallet to one of the other two.
The name require_transfer is also misleading because the transfer is not required if dest is in not_migrated_dests.
4019+ }
4020+ wallets_vec.emplace_back(ext_wallet, std::move(batch));
4021+ }
4022+
4023+ // Write address book entry to disk
4024+ auto func_store_addr = [](WalletBatch& batch, const CTxDestination& dest, const CAddressBookData& entry) {
In commit “wallet: addressbook migration, batch db writes” (342c45f80e32b0320829ce380b5854844cd74bc8)
write_address_book would be a more descriptive name than func_store_addr and watchonly_wallets would be more descriptive than wallet_vec. The _vec suffix and and func_ prefix do not do anything to help a reader of the code, IMO.