From 97b075392305becfbad4d497614478cff2d9237f Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 9 Jul 2022 21:30:57 -0300 Subject: [PATCH 1/6] wallet: clean redundancies in DelAddressBook 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 --- src/wallet/wallet.cpp | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e03f5532fcf..28c696d0dd0 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2385,6 +2385,7 @@ bool CWallet::SetAddressBook(const CTxDestination& address, const std::string& s bool CWallet::DelAddressBook(const CTxDestination& address) { + const std::string& dest = EncodeDestination(address); WalletBatch batch(GetDatabase()); { LOCK(cs_wallet); @@ -2396,14 +2397,30 @@ bool CWallet::DelAddressBook(const CTxDestination& address) return false; } // Delete data rows associated with this address - batch.EraseAddressData(address); + if (!batch.EraseAddressData(address)) { + WalletLogPrintf("Error: cannot erase address book entry data\n"); + return false; + } + + // Delete purpose entry + if (!batch.ErasePurpose(dest)) { + WalletLogPrintf("Error: cannot erase address book entry purpose\n"); + return false; + } + + // Delete name entry + if (!batch.EraseName(dest)) { + WalletLogPrintf("Error: cannot erase address book entry name\n"); + return false; + } + + // finally, remove it from the map m_address_book.erase(address); } + // All good, signal changes NotifyAddressBookChanged(address, "", /*is_mine=*/false, AddressPurpose::SEND, CT_DELETED); - - batch.ErasePurpose(EncodeDestination(address)); - return batch.EraseName(EncodeDestination(address)); + return true; } size_t CWallet::KeypoolCountExternalKeys() const From bba4f8dcb55de3ca4963711dc17882b43cb0bc4a Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 9 Jul 2022 21:48:37 -0300 Subject: [PATCH 2/6] refactor: SetAddrBookWithDB, signal only if write succeeded --- src/wallet/wallet.cpp | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 28c696d0dd0..55f8f1b9b5a 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2368,13 +2368,22 @@ bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& add purpose = m_address_book[address].purpose; } } + + const std::string& encoded_dest = EncodeDestination(address); + if (new_purpose && !batch.WritePurpose(encoded_dest, PurposeToString(*new_purpose))) { + WalletLogPrintf("Error: fail to write address book 'purpose' entry\n"); + return false; + } + if (!batch.WriteName(encoded_dest, strName)) { + WalletLogPrintf("Error: fail to write address book 'name' entry\n"); + return false; + } + // In very old wallets, address purpose may not be recorded so we derive it from IsMine NotifyAddressBookChanged(address, strName, is_mine, purpose.value_or(is_mine ? AddressPurpose::RECEIVE : AddressPurpose::SEND), (fUpdated ? CT_UPDATED : CT_NEW)); - if (new_purpose && !batch.WritePurpose(EncodeDestination(address), PurposeToString(*new_purpose))) - return false; - return batch.WriteName(EncodeDestination(address), strName); + return true; } bool CWallet::SetAddressBook(const CTxDestination& address, const std::string& strName, const std::optional& purpose) From d0943315b1d00905fe7f4513b2f3f47b88a99e8f Mon Sep 17 00:00:00 2001 From: furszy Date: Sat, 25 Feb 2023 16:39:11 -0300 Subject: [PATCH 3/6] refactor: SetAddressBookWithDB, minimize number of map lookups --- src/wallet/wallet.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 55f8f1b9b5a..e392f7f8aac 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2359,14 +2359,15 @@ bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& add { LOCK(cs_wallet); std::map::iterator mi = m_address_book.find(address); - fUpdated = (mi != m_address_book.end() && !mi->second.IsChange()); - m_address_book[address].SetLabel(strName); + fUpdated = mi != m_address_book.end() && !mi->second.IsChange(); + + CAddressBookData& record = mi != m_address_book.end() ? mi->second : m_address_book[address]; + record.SetLabel(strName); is_mine = IsMine(address) != ISMINE_NO; if (new_purpose) { /* update purpose only if requested */ - purpose = m_address_book[address].purpose = new_purpose; - } else { - purpose = m_address_book[address].purpose; + record.purpose = new_purpose; } + purpose = record.purpose; } const std::string& encoded_dest = EncodeDestination(address); From 595bbe6e81885d35179aba6137dc63d0e652cc1f Mon Sep 17 00:00:00 2001 From: furszy Date: Sun, 26 Feb 2023 20:30:02 -0300 Subject: [PATCH 4/6] refactor: wallet, simplify addressbook migration Same process written in a cleaner manner. Removing code duplication. --- src/wallet/wallet.cpp | 73 ++++++++++++++++++------------------------- 1 file changed, 31 insertions(+), 42 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index e392f7f8aac..cebb7b39126 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4009,52 +4009,41 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) // Check the address book data in the same way we did for transactions std::vector dests_to_delete; - for (const auto& addr_pair : m_address_book) { - // Labels applied to receiving addresses should go based on IsMine - if (addr_pair.second.purpose == AddressPurpose::RECEIVE) { - if (!IsMine(addr_pair.first)) { - // Check the address book data is the watchonly wallet's - if (data.watchonly_wallet) { - LOCK(data.watchonly_wallet->cs_wallet); - if (data.watchonly_wallet->IsMine(addr_pair.first)) { - // Add to the watchonly. Copy the entire address book entry - data.watchonly_wallet->m_address_book[addr_pair.first] = addr_pair.second; - dests_to_delete.push_back(addr_pair.first); - continue; - } - } - if (data.solvable_wallet) { - LOCK(data.solvable_wallet->cs_wallet); - if (data.solvable_wallet->IsMine(addr_pair.first)) { - // Add to the solvable. Copy the entire address book entry - data.solvable_wallet->m_address_book[addr_pair.first] = addr_pair.second; - dests_to_delete.push_back(addr_pair.first); - continue; - } - } + for (const auto& [dest, record] : m_address_book) { + // Ensure "receive" entries that are no longer part of the original wallet are transferred to another wallet + // Entries for everything else ("send") will be cloned to all wallets. + bool require_transfer = record.purpose == AddressPurpose::RECEIVE && !IsMine(dest); + bool copied = false; + for (auto& wallet : {data.watchonly_wallet, data.solvable_wallet}) { + if (!wallet) continue; - // Skip invalid/non-watched scripts that will not be migrated - if (not_migrated_dests.count(addr_pair.first) > 0) { - dests_to_delete.push_back(addr_pair.first); - continue; - } + LOCK(wallet->cs_wallet); + if (require_transfer && !wallet->IsMine(dest)) continue; - // Not ours, not in watchonly wallet, and not in solvable - error = _("Error: Address book data in wallet cannot be identified to belong to migrated wallets"); - return false; + // Copy the entire address book entry + wallet->m_address_book[dest] = record; + + copied = true; + // Only delete 'receive' records that are no longer part of the original wallet + if (require_transfer) { + dests_to_delete.push_back(dest); + break; } - } else { - // Labels for everything else ("send") should be cloned to all - if (data.watchonly_wallet) { - LOCK(data.watchonly_wallet->cs_wallet); - // Add to the watchonly. Copy the entire address book entry - data.watchonly_wallet->m_address_book[addr_pair.first] = addr_pair.second; - } - if (data.solvable_wallet) { - LOCK(data.solvable_wallet->cs_wallet); - // Add to the solvable. Copy the entire address book entry - data.solvable_wallet->m_address_book[addr_pair.first] = addr_pair.second; + } + + // Fail immediately if we ever found an entry that was ours and cannot be transferred + // to any of the created wallets (watch-only, solvable). + // Means that no inferred descriptor maps to the stored entry. Which mustn't happen. + if (require_transfer && !copied) { + + // Skip invalid/non-watched scripts that will not be migrated + if (not_migrated_dests.count(dest) > 0) { + dests_to_delete.push_back(dest); + continue; } + + error = _("Error: Address book data in wallet cannot be identified to belong to migrated wallets"); + return false; } } From 342c45f80e32b0320829ce380b5854844cd74bc8 Mon Sep 17 00:00:00 2001 From: furszy Date: Sun, 21 Jan 2024 13:01:56 -0300 Subject: [PATCH 5/6] wallet: addressbook migration, batch db writes Optimizing the process performance and consistency. --- src/wallet/wallet.cpp | 52 +++++++++++++++++++++++++++---------------- 1 file changed, 33 insertions(+), 19 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index cebb7b39126..aae876a819e 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4007,6 +4007,30 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) } } + // Pair external wallets with their corresponding db handler + std::vector, std::unique_ptr>> wallets_vec; + for (const auto& ext_wallet : {data.watchonly_wallet, data.solvable_wallet}) { + if (!ext_wallet) continue; + + std::unique_ptr batch = std::make_unique(ext_wallet->GetDatabase()); + if (!batch->TxnBegin()) { + error = strprintf(_("Error: database transaction cannot be executed for wallet %s"), ext_wallet->GetName()); + return false; + } + wallets_vec.emplace_back(ext_wallet, std::move(batch)); + } + + // Write address book entry to disk + auto func_store_addr = [](WalletBatch& batch, const CTxDestination& dest, const CAddressBookData& entry) { + auto address{EncodeDestination(dest)}; + if (entry.purpose) batch.WritePurpose(address, PurposeToString(*entry.purpose)); + if (entry.label) batch.WriteName(address, *entry.label); + for (const auto& [id, request] : entry.receive_requests) { + batch.WriteAddressReceiveRequest(dest, id, request); + } + if (entry.previously_spent) batch.WriteAddressPreviouslySpent(dest, true); + }; + // Check the address book data in the same way we did for transactions std::vector dests_to_delete; for (const auto& [dest, record] : m_address_book) { @@ -4014,14 +4038,13 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) // Entries for everything else ("send") will be cloned to all wallets. bool require_transfer = record.purpose == AddressPurpose::RECEIVE && !IsMine(dest); bool copied = false; - for (auto& wallet : {data.watchonly_wallet, data.solvable_wallet}) { - if (!wallet) continue; - + for (auto& [wallet, batch] : wallets_vec) { LOCK(wallet->cs_wallet); if (require_transfer && !wallet->IsMine(dest)) continue; // Copy the entire address book entry wallet->m_address_book[dest] = record; + func_store_addr(*batch, dest, record); copied = true; // Only delete 'receive' records that are no longer part of the original wallet @@ -4047,24 +4070,15 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) } } - // Persist added address book entries (labels, purpose) for watchonly and solvable wallets - auto persist_address_book = [](const CWallet& wallet) { - LOCK(wallet.cs_wallet); - WalletBatch batch{wallet.GetDatabase()}; - for (const auto& [destination, addr_book_data] : wallet.m_address_book) { - auto address{EncodeDestination(destination)}; - if (addr_book_data.purpose) batch.WritePurpose(address, PurposeToString(*addr_book_data.purpose)); - if (addr_book_data.label) batch.WriteName(address, *addr_book_data.label); - for (const auto& [id, request] : addr_book_data.receive_requests) { - batch.WriteAddressReceiveRequest(destination, id, request); - } - if (addr_book_data.previously_spent) batch.WriteAddressPreviouslySpent(destination, true); + // Persist external wallets address book entries + for (auto& [wallet, batch] : wallets_vec) { + if (!batch->TxnCommit()) { + error = strprintf(_("Error: address book copy failed for wallet %s"), wallet->GetName()); + return false; } - }; - if (data.watchonly_wallet) persist_address_book(*data.watchonly_wallet); - if (data.solvable_wallet) persist_address_book(*data.solvable_wallet); + } - // Remove the things to delete + // Remove the things to delete in this wallet if (dests_to_delete.size() > 0) { for (const auto& dest : dests_to_delete) { if (!DelAddressBook(dest)) { From 86960cdb7f75eaa2ae150914c54240d1d5ef96d1 Mon Sep 17 00:00:00 2001 From: furszy Date: Fri, 29 Sep 2023 15:42:15 -0300 Subject: [PATCH 6/6] wallet: migration, batch addressbook records removal 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. --- src/wallet/wallet.cpp | 12 ++++++++++-- src/wallet/wallet.h | 1 + 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index aae876a819e..5c19a44fb4d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -2395,8 +2395,13 @@ bool CWallet::SetAddressBook(const CTxDestination& address, const std::string& s bool CWallet::DelAddressBook(const CTxDestination& address) { - const std::string& dest = EncodeDestination(address); WalletBatch batch(GetDatabase()); + return DelAddressBookWithDB(batch, address); +} + +bool CWallet::DelAddressBookWithDB(WalletBatch& batch, const CTxDestination& address) +{ + const std::string& dest = EncodeDestination(address); { LOCK(cs_wallet); // If we want to delete receiving addresses, we should avoid calling EraseAddressData because it will delete the previously_spent value. Could instead just erase the label so it becomes a change address, and keep the data. @@ -4079,14 +4084,17 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error) } // Remove the things to delete in this wallet + WalletBatch local_wallet_batch(GetDatabase()); + local_wallet_batch.TxnBegin(); if (dests_to_delete.size() > 0) { for (const auto& dest : dests_to_delete) { - if (!DelAddressBook(dest)) { + if (!DelAddressBookWithDB(local_wallet_batch, dest)) { error = _("Error: Unable to remove watchonly address book data"); return false; } } } + local_wallet_batch.TxnCommit(); // Connect the SPKM signals ConnectScriptPubKeyManNotifiers(); diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 487921443fd..9e09df4d821 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -795,6 +795,7 @@ public: bool SetAddressBook(const CTxDestination& address, const std::string& strName, const std::optional& purpose); bool DelAddressBook(const CTxDestination& address); + bool DelAddressBookWithDB(WalletBatch& batch, const CTxDestination& address); bool IsAddressPreviouslySpent(const CTxDestination& dest) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet); bool SetAddressPreviouslySpent(WalletBatch& batch, const CTxDestination& dest, bool used) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);