mirror of
https://github.com/bitcoin/bitcoin.git
synced 2024-05-17 23:56:39 +00:00
Merge bitcoin/bitcoin#28038: wallet: address book migration bug fixes
7ecc29a0b7a23d8f5d3c1e6a0dad29b3ad839eb9 test: wallet, add coverage for addressbook migration (furszy)
a277f8357ad8b0eb26f33fc36f919d868c06847b wallet: migration bugfix, persist empty labels (furszy)
1b64f6498c394a143df196172a14204fe3b8a744 wallet: migration bugfix, clone 'send' record label to all wallets (furszy)
Pull request description:
Addressing two specific bugs encountered during the wallet migration process, related to the address book, and improves the test coverage for it.
Bug 1: Non-Cloning of External 'Send' Records
The external 'send' records were not being correctly cloned to all wallets.
Bug 2: Persistence of Empty Labels
As address book entries without associated db label records can be treated as change (the `label` field inside the `CAddressBookData` class is optional, `nullopt` labels make `CAddressBookData ::IsChange()` return true), we must persist empty labels during the migration process.
The user might have called `setlabel` with an "" string for an external address and that must be retained during migration.
ACKs for top commit:
achow101:
ACK 7ecc29a0b7a23d8f5d3c1e6a0dad29b3ad839eb9
Tree-SHA512: b8a8483a4178a37c49af11eb7ba8a82ca95e54a6cd799e155e33f9fbe7f37b259e28372c77d6944d46b6765f9eaca6b8ca8d1cdd9d223120a3653e4e41d0b6b7
This commit is contained in:
fanquake
commit
87e19b047c
@ -4031,7 +4031,7 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
|
||||
return false;
|
||||
}
|
||||
} else {
|
||||
// Labels for everything else (send) should be cloned to all
|
||||
// Labels for everything else ("send") should be cloned to all
|
||||
if (data.watchonly_wallet) {
|
||||
LOCK(data.watchonly_wallet->cs_wallet);
|
||||
// Add to the watchonly. Preserve the labels, purpose, and change-ness
|
||||
@ -4040,7 +4040,6 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
|
||||
if (!addr_pair.second.IsChange()) {
|
||||
data.watchonly_wallet->m_address_book[addr_pair.first].SetLabel(label);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
if (data.solvable_wallet) {
|
||||
LOCK(data.solvable_wallet->cs_wallet);
|
||||
@ -4050,7 +4049,6 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
|
||||
if (!addr_pair.second.IsChange()) {
|
||||
data.solvable_wallet->m_address_book[addr_pair.first].SetLabel(label);
|
||||
}
|
||||
continue;
|
||||
}
|
||||
}
|
||||
}
|
||||
@ -4061,10 +4059,10 @@ bool CWallet::ApplyMigrationData(MigrationData& data, bilingual_str& error)
|
||||
WalletBatch batch{wallet.GetDatabase()};
|
||||
for (const auto& [destination, addr_book_data] : wallet.m_address_book) {
|
||||
auto address{EncodeDestination(destination)};
|
||||
auto label{addr_book_data.GetLabel()};
|
||||
// don't bother writing default values (unknown purpose, empty label)
|
||||
std::optional<std::string> label = addr_book_data.IsChange() ? std::nullopt : std::make_optional(addr_book_data.GetLabel());
|
||||
// don't bother writing default values (unknown purpose)
|
||||
if (addr_book_data.purpose) batch.WritePurpose(address, PurposeToString(*addr_book_data.purpose));
|
||||
if (!label.empty()) batch.WriteName(address, label);
|
||||
if (label) batch.WriteName(address, *label);
|
||||
}
|
||||
};
|
||||
if (data.watchonly_wallet) persist_address_book(*data.watchonly_wallet);
|
||||
|
||||
@ -67,6 +67,15 @@ class WalletMigrationTest(BitcoinTestFramework):
|
||||
del d["parent_descs"]
|
||||
assert_equal(received_list_txs, expected_list_txs)
|
||||
|
||||
def check_address(self, wallet, addr, is_mine, is_change, label):
|
||||
addr_info = wallet.getaddressinfo(addr)
|
||||
assert_equal(addr_info['ismine'], is_mine)
|
||||
assert_equal(addr_info['ischange'], is_change)
|
||||
if label is not None:
|
||||
assert_equal(addr_info['labels'], [label]),
|
||||
else:
|
||||
assert_equal(addr_info['labels'], []),
|
||||
|
||||
def test_basic(self):
|
||||
default = self.nodes[0].get_wallet_rpc(self.default_wallet_name)
|
||||
|
||||
@ -504,6 +513,132 @@ class WalletMigrationTest(BitcoinTestFramework):
|
||||
assert wallet_path.is_dir()
|
||||
assert wallet_dat_path.is_file()
|
||||
|
||||
def test_addressbook(self):
|
||||
df_wallet = self.nodes[0].get_wallet_rpc(self.default_wallet_name)
|
||||
|
||||
self.log.info("Test migration of address book data")
|
||||
wallet = self.create_legacy_wallet("legacy_addrbook")
|
||||
df_wallet.sendtoaddress(wallet.getnewaddress(), 3)
|
||||
|
||||
# Import watch-only script to create a watch-only wallet after migration
|
||||
watch_addr = df_wallet.getnewaddress()
|
||||
wallet.importaddress(watch_addr)
|
||||
df_wallet.sendtoaddress(watch_addr, 2)
|
||||
|
||||
# Import solvable script
|
||||
multi_addr1 = wallet.getnewaddress()
|
||||
multi_addr2 = wallet.getnewaddress()
|
||||
multi_addr3 = df_wallet.getnewaddress()
|
||||
wallet.importpubkey(df_wallet.getaddressinfo(multi_addr3)["pubkey"])
|
||||
ms_addr_info = wallet.addmultisigaddress(2, [multi_addr1, multi_addr2, multi_addr3])
|
||||
|
||||
self.generate(self.nodes[0], 1)
|
||||
|
||||
# Test vectors
|
||||
addr_external = {
|
||||
"addr": df_wallet.getnewaddress(),
|
||||
"is_mine": False,
|
||||
"is_change": False,
|
||||
"label": ""
|
||||
}
|
||||
addr_external_with_label = {
|
||||
"addr": df_wallet.getnewaddress(),
|
||||
"is_mine": False,
|
||||
"is_change": False,
|
||||
"label": "external"
|
||||
}
|
||||
addr_internal = {
|
||||
"addr": wallet.getnewaddress(),
|
||||
"is_mine": True,
|
||||
"is_change": False,
|
||||
"label": ""
|
||||
}
|
||||
addr_internal_with_label = {
|
||||
"addr": wallet.getnewaddress(),
|
||||
"is_mine": True,
|
||||
"is_change": False,
|
||||
"label": "internal"
|
||||
}
|
||||
change_address = {
|
||||
"addr": wallet.getrawchangeaddress(),
|
||||
"is_mine": True,
|
||||
"is_change": True,
|
||||
"label": None
|
||||
}
|
||||
watch_only_addr = {
|
||||
"addr": watch_addr,
|
||||
"is_mine": False,
|
||||
"is_change": False,
|
||||
"label": "imported"
|
||||
}
|
||||
ms_addr = {
|
||||
"addr": ms_addr_info['address'],
|
||||
"is_mine": False,
|
||||
"is_change": False,
|
||||
"label": "multisig"
|
||||
}
|
||||
|
||||
# To store the change address in the addressbook need to send coins to it
|
||||
wallet.send(outputs=[{wallet.getnewaddress(): 2}], options={"change_address": change_address['addr']})
|
||||
self.generate(self.nodes[0], 1)
|
||||
|
||||
# Util wrapper func for 'addr_info'
|
||||
def check(info, node):
|
||||
self.check_address(node, info['addr'], info['is_mine'], info['is_change'], info["label"])
|
||||
|
||||
# Pre-migration: set label and perform initial checks
|
||||
for addr_info in [addr_external, addr_external_with_label, addr_internal, addr_internal_with_label, change_address, watch_only_addr, ms_addr]:
|
||||
if not addr_info['is_change']:
|
||||
wallet.setlabel(addr_info['addr'], addr_info["label"])
|
||||
check(addr_info, wallet)
|
||||
|
||||
# Migrate wallet
|
||||
info_migration = wallet.migratewallet()
|
||||
wallet_wo = self.nodes[0].get_wallet_rpc(info_migration["watchonly_name"])
|
||||
wallet_solvables = self.nodes[0].get_wallet_rpc(info_migration["solvables_name"])
|
||||
|
||||
#########################
|
||||
# Post migration checks #
|
||||
#########################
|
||||
|
||||
# First check the main wallet
|
||||
for addr_info in [addr_external, addr_external_with_label, addr_internal, addr_internal_with_label, change_address, ms_addr]:
|
||||
check(addr_info, wallet)
|
||||
|
||||
# Watch-only wallet will contain the watch-only entry (with 'is_mine=True') and all external addresses ('send')
|
||||
self.check_address(wallet_wo, watch_only_addr['addr'], is_mine=True, is_change=watch_only_addr['is_change'], label=watch_only_addr["label"])
|
||||
for addr_info in [addr_external, addr_external_with_label, ms_addr]:
|
||||
check(addr_info, wallet_wo)
|
||||
|
||||
# Solvables wallet will contain the multisig entry (with 'is_mine=True') and all external addresses ('send')
|
||||
self.check_address(wallet_solvables, ms_addr['addr'], is_mine=True, is_change=ms_addr['is_change'], label=ms_addr["label"])
|
||||
for addr_info in [addr_external, addr_external_with_label]:
|
||||
check(addr_info, wallet_solvables)
|
||||
|
||||
########################################################################################
|
||||
# Now restart migrated wallets and verify that the addressbook entries are still there #
|
||||
########################################################################################
|
||||
|
||||
# First the main wallet
|
||||
self.nodes[0].unloadwallet("legacy_addrbook")
|
||||
self.nodes[0].loadwallet("legacy_addrbook")
|
||||
for addr_info in [addr_external, addr_external_with_label, addr_internal, addr_internal_with_label, change_address, ms_addr]:
|
||||
check(addr_info, wallet)
|
||||
|
||||
# Watch-only wallet
|
||||
self.nodes[0].unloadwallet(info_migration["watchonly_name"])
|
||||
self.nodes[0].loadwallet(info_migration["watchonly_name"])
|
||||
self.check_address(wallet_wo, watch_only_addr['addr'], is_mine=True, is_change=watch_only_addr['is_change'], label=watch_only_addr["label"])
|
||||
for addr_info in [addr_external, addr_external_with_label, ms_addr]:
|
||||
check(addr_info, wallet_wo)
|
||||
|
||||
# Solvables wallet
|
||||
self.nodes[0].unloadwallet(info_migration["solvables_name"])
|
||||
self.nodes[0].loadwallet(info_migration["solvables_name"])
|
||||
self.check_address(wallet_solvables, ms_addr['addr'], is_mine=True, is_change=ms_addr['is_change'], label=ms_addr["label"])
|
||||
for addr_info in [addr_external, addr_external_with_label]:
|
||||
check(addr_info, wallet_solvables)
|
||||
|
||||
def run_test(self):
|
||||
self.generate(self.nodes[0], 101)
|
||||
|
||||
@ -518,6 +653,7 @@ class WalletMigrationTest(BitcoinTestFramework):
|
||||
self.test_unloaded_by_path()
|
||||
self.test_default_wallet()
|
||||
self.test_direct_file()
|
||||
self.test_addressbook()
|
||||
|
||||
if __name__ == '__main__':
|
||||
WalletMigrationTest().main()
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user