Skip to content

Commit 835948d

Browse files
committed
Merge bitcoin/bitcoin#26836: wallet: batch and simplify addressbook migration process
86960cd wallet: migration, batch addressbook records removal (furszy) 342c45f wallet: addressbook migration, batch db writes (furszy) 595bbe6 refactor: wallet, simplify addressbook migration (furszy) d094331 refactor: SetAddressBookWithDB, minimize number of map lookups (furszy) bba4f8d refactor: SetAddrBookWithDB, signal only if write succeeded (furszy) 97b0753 wallet: clean redundancies in DelAddressBook (furszy) Pull request description: Commits decoupled from #28574, focused on the address book cloning process Includes: 1) DB batch operations and flow simplification for the address book migration process. 2) Code improvements to `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. ACKs for top commit: achow101: ACK 86960cd josibake: reACK bitcoin/bitcoin@86960cd Tree-SHA512: 10c941df3cd84fd8662b9c9ca6a1ed2c7402d38c677d2fc66b8b6c9edc6d73e827a5821487bbcacb5569d502934fa548fd10699e2ec45185f869e43174d8b2a1
2 parents 801ef07 + 86960cd commit 835948d

File tree

2 files changed

+111
-72
lines changed

2 files changed

+111
-72
lines changed

src/wallet/wallet.cpp

Lines changed: 110 additions & 72 deletions
Original file line numberDiff line numberDiff line change
@@ -2359,22 +2359,32 @@ bool CWallet::SetAddressBookWithDB(WalletBatch& batch, const CTxDestination& add
23592359
{
23602360
LOCK(cs_wallet);
23612361
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);
2362+
fUpdated = mi != m_address_book.end() && !mi->second.IsChange();
2363+
2364+
CAddressBookData& record = mi != m_address_book.end() ? mi->second : m_address_book[address];
2365+
record.SetLabel(strName);
23642366
is_mine = IsMine(address) != ISMINE_NO;
23652367
if (new_purpose) { /* update purpose only if requested */
2366-
purpose = m_address_book[address].purpose = new_purpose;
2367-
} else {
2368-
purpose = m_address_book[address].purpose;
2368+
record.purpose = new_purpose;
23692369
}
2370+
purpose = record.purpose;
2371+
}
2372+
2373+
const std::string& encoded_dest = EncodeDestination(address);
2374+
if (new_purpose && !batch.WritePurpose(encoded_dest, PurposeToString(*new_purpose))) {
2375+
WalletLogPrintf("Error: fail to write address book 'purpose' entry\n");
2376+
return false;
2377+
}
2378+
if (!batch.WriteName(encoded_dest, strName)) {
2379+
WalletLogPrintf("Error: fail to write address book 'name' entry\n");
2380+
return false;
23702381
}
2382+
23712383
// In very old wallets, address purpose may not be recorded so we derive it from IsMine
23722384
NotifyAddressBookChanged(address, strName, is_mine,
23732385
purpose.value_or(is_mine ? AddressPurpose::RECEIVE : AddressPurpose::SEND),
23742386
(fUpdated ? CT_UPDATED : CT_NEW));
2375-
if (new_purpose && !batch.WritePurpose(EncodeDestination(address), PurposeToString(*new_purpose)))
2376-
return false;
2377-
return batch.WriteName(EncodeDestination(address), strName);
2387+
return true;
23782388
}
23792389

23802390
bool CWallet::SetAddressBook(const CTxDestination& address, const std::string& strName, const std::optional<AddressPurpose>& purpose)
@@ -2386,6 +2396,12 @@ bool CWallet::SetAddressBook(const CTxDestination& address, const std::string& s
23862396
bool CWallet::DelAddressBook(const CTxDestination& address)
23872397
{
23882398
WalletBatch batch(GetDatabase());
2399+
return DelAddressBookWithDB(batch, address);
2400+
}
2401+
2402+
bool CWallet::DelAddressBookWithDB(WalletBatch& batch, const CTxDestination& address)
2403+
{
2404+
const std::string& dest = EncodeDestination(address);
23892405
{
23902406
LOCK(cs_wallet);
23912407
// 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.
@@ -2396,14 +2412,30 @@ bool CWallet::DelAddressBook(const CTxDestination& address)
23962412
return false;
23972413
}
23982414
// Delete data rows associated with this address
2399-
batch.EraseAddressData(address);
2415+
if (!batch.EraseAddressData(address)) {
2416+
WalletLogPrintf("Error: cannot erase address book entry data\n");
2417+
return false;
2418+
}
2419+
2420+
// Delete purpose entry
2421+
if (!batch.ErasePurpose(dest)) {
2422+
WalletLogPrintf("Error: cannot erase address book entry purpose\n");
2423+
return false;
2424+
}
2425+
2426+
// Delete name entry
2427+
if (!batch.EraseName(dest)) {
2428+
WalletLogPrintf("Error: cannot erase address book entry name\n");
2429+
return false;
2430+
}
2431+
2432+
// finally, remove it from the map
24002433
m_address_book.erase(address);
24012434
}
24022435

2436+
// All good, signal changes
24032437
NotifyAddressBookChanged(address, "", /*is_mine=*/false, AddressPurpose::SEND, CT_DELETED);
2404-
2405-
batch.ErasePurpose(EncodeDestination(address));
2406-
return batch.EraseName(EncodeDestination(address));
2438+
return true;
24072439
}
24082440

24092441
size_t CWallet::KeypoolCountExternalKeys() const
@@ -4008,83 +4040,89 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
40084040
}
40094041
}
40104042

4043+
// Pair external wallets with their corresponding db handler
4044+
std::vector<std::pair<std::shared_ptr<CWallet>, std::unique_ptr<WalletBatch>>> wallets_vec;
4045+
for (const auto& ext_wallet : {data.watchonly_wallet, data.solvable_wallet}) {
4046+
if (!ext_wallet) continue;
4047+
4048+
std::unique_ptr<WalletBatch> batch = std::make_unique<WalletBatch>(ext_wallet->GetDatabase());
4049+
if (!batch->TxnBegin()) {
4050+
error = strprintf(_("Error: database transaction cannot be executed for wallet %s"), ext_wallet->GetName());
4051+
return false;
4052+
}
4053+
wallets_vec.emplace_back(ext_wallet, std::move(batch));
4054+
}
4055+
4056+
// Write address book entry to disk
4057+
auto func_store_addr = [](WalletBatch& batch, const CTxDestination& dest, const CAddressBookData& entry) {
4058+
auto address{EncodeDestination(dest)};
4059+
if (entry.purpose) batch.WritePurpose(address, PurposeToString(*entry.purpose));
4060+
if (entry.label) batch.WriteName(address, *entry.label);
4061+
for (const auto& [id, request] : entry.receive_requests) {
4062+
batch.WriteAddressReceiveRequest(dest, id, request);
4063+
}
4064+
if (entry.previously_spent) batch.WriteAddressPreviouslySpent(dest, true);
4065+
};
4066+
40114067
// Check the address book data in the same way we did for transactions
40124068
std::vector<CTxDestination> dests_to_delete;
4013-
for (const auto& addr_pair : m_address_book) {
4014-
// Labels applied to receiving addresses should go based on IsMine
4015-
if (addr_pair.second.purpose == AddressPurpose::RECEIVE) {
4016-
if (!IsMine(addr_pair.first)) {
4017-
// Check the address book data is the watchonly wallet's
4018-
if (data.watchonly_wallet) {
4019-
LOCK(data.watchonly_wallet->cs_wallet);
4020-
if (data.watchonly_wallet->IsMine(addr_pair.first)) {
4021-
// Add to the watchonly. Copy the entire address book entry
4022-
data.watchonly_wallet->m_address_book[addr_pair.first] = addr_pair.second;
4023-
dests_to_delete.push_back(addr_pair.first);
4024-
continue;
4025-
}
4026-
}
4027-
if (data.solvable_wallet) {
4028-
LOCK(data.solvable_wallet->cs_wallet);
4029-
if (data.solvable_wallet->IsMine(addr_pair.first)) {
4030-
// Add to the solvable. Copy the entire address book entry
4031-
data.solvable_wallet->m_address_book[addr_pair.first] = addr_pair.second;
4032-
dests_to_delete.push_back(addr_pair.first);
4033-
continue;
4034-
}
4035-
}
4069+
for (const auto& [dest, record] : m_address_book) {
4070+
// Ensure "receive" entries that are no longer part of the original wallet are transferred to another wallet
4071+
// Entries for everything else ("send") will be cloned to all wallets.
4072+
bool require_transfer = record.purpose == AddressPurpose::RECEIVE && !IsMine(dest);
4073+
bool copied = false;
4074+
for (auto& [wallet, batch] : wallets_vec) {
4075+
LOCK(wallet->cs_wallet);
4076+
if (require_transfer && !wallet->IsMine(dest)) continue;
4077+
4078+
// Copy the entire address book entry
4079+
wallet->m_address_book[dest] = record;
4080+
func_store_addr(*batch, dest, record);
4081+
4082+
copied = true;
4083+
// Only delete 'receive' records that are no longer part of the original wallet
4084+
if (require_transfer) {
4085+
dests_to_delete.push_back(dest);
4086+
break;
4087+
}
4088+
}
40364089

4037-
// Skip invalid/non-watched scripts that will not be migrated
4038-
if (not_migrated_dests.count(addr_pair.first) > 0) {
4039-
dests_to_delete.push_back(addr_pair.first);
4040-
continue;
4041-
}
4090+
// Fail immediately if we ever found an entry that was ours and cannot be transferred
4091+
// to any of the created wallets (watch-only, solvable).
4092+
// Means that no inferred descriptor maps to the stored entry. Which mustn't happen.
4093+
if (require_transfer && !copied) {
40424094

4043-
// Not ours, not in watchonly wallet, and not in solvable
4044-
error = _("Error: Address book data in wallet cannot be identified to belong to migrated wallets");
4045-
return false;
4046-
}
4047-
} else {
4048-
// Labels for everything else ("send") should be cloned to all
4049-
if (data.watchonly_wallet) {
4050-
LOCK(data.watchonly_wallet->cs_wallet);
4051-
// Add to the watchonly. Copy the entire address book entry
4052-
data.watchonly_wallet->m_address_book[addr_pair.first] = addr_pair.second;
4053-
}
4054-
if (data.solvable_wallet) {
4055-
LOCK(data.solvable_wallet->cs_wallet);
4056-
// Add to the solvable. Copy the entire address book entry
4057-
data.solvable_wallet->m_address_book[addr_pair.first] = addr_pair.second;
4095+
// Skip invalid/non-watched scripts that will not be migrated
4096+
if (not_migrated_dests.count(dest) > 0) {
4097+
dests_to_delete.push_back(dest);
4098+
continue;
40584099
}
4100+
4101+
error = _("Error: Address book data in wallet cannot be identified to belong to migrated wallets");
4102+
return false;
40594103
}
40604104
}
40614105

4062-
// Persist added address book entries (labels, purpose) for watchonly and solvable wallets
4063-
auto persist_address_book = [](const CWallet& wallet) {
4064-
LOCK(wallet.cs_wallet);
4065-
WalletBatch batch{wallet.GetDatabase()};
4066-
for (const auto& [destination, addr_book_data] : wallet.m_address_book) {
4067-
auto address{EncodeDestination(destination)};
4068-
if (addr_book_data.purpose) batch.WritePurpose(address, PurposeToString(*addr_book_data.purpose));
4069-
if (addr_book_data.label) batch.WriteName(address, *addr_book_data.label);
4070-
for (const auto& [id, request] : addr_book_data.receive_requests) {
4071-
batch.WriteAddressReceiveRequest(destination, id, request);
4072-
}
4073-
if (addr_book_data.previously_spent) batch.WriteAddressPreviouslySpent(destination, true);
4106+
// Persist external wallets address book entries
4107+
for (auto& [wallet, batch] : wallets_vec) {
4108+
if (!batch->TxnCommit()) {
4109+
error = strprintf(_("Error: address book copy failed for wallet %s"), wallet->GetName());
4110+
return false;
40744111
}
4075-
};
4076-
if (data.watchonly_wallet) persist_address_book(*data.watchonly_wallet);
4077-
if (data.solvable_wallet) persist_address_book(*data.solvable_wallet);
4112+
}
40784113

4079-
// Remove the things to delete
4114+
// Remove the things to delete in this wallet
4115+
WalletBatch local_wallet_batch(GetDatabase());
4116+
local_wallet_batch.TxnBegin();
40804117
if (dests_to_delete.size() > 0) {
40814118
for (const auto& dest : dests_to_delete) {
4082-
if (!DelAddressBook(dest)) {
4119+
if (!DelAddressBookWithDB(local_wallet_batch, dest)) {
40834120
error = _("Error: Unable to remove watchonly address book data");
40844121
return false;
40854122
}
40864123
}
40874124
}
4125+
local_wallet_batch.TxnCommit();
40884126

40894127
// Connect the SPKM signals
40904128
ConnectScriptPubKeyManNotifiers();

src/wallet/wallet.h

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -795,6 +795,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
795795
bool SetAddressBook(const CTxDestination& address, const std::string& strName, const std::optional<AddressPurpose>& purpose);
796796

797797
bool DelAddressBook(const CTxDestination& address);
798+
bool DelAddressBookWithDB(WalletBatch& batch, const CTxDestination& address);
798799

799800
bool IsAddressPreviouslySpent(const CTxDestination& dest) const EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);
800801
bool SetAddressPreviouslySpent(WalletBatch& batch, const CTxDestination& dest, bool used) EXCLUSIVE_LOCKS_REQUIRED(cs_wallet);

0 commit comments

Comments
 (0)