We currently fail migration if the wallet does not contain the best block locator. This is a problem for wallets created before #152, which are not storing such record.
Missing this record is not an error. it simply means the wallet will scan the chain prior to finish migration.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34198.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
ACK 63a079e7761e8ef94e706eb2103ece4e2a117e50
Code review ACK. The fix makes sense - ancient pre-#152 wallets shouldn't fail migration just because they lack the bestblock record.
Couldn't run the test locally (requires previous releases for BDB wallet support), but the logic is straightforward.
3931 | @@ -3932,10 +3932,9 @@ util::Result<void> CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch, 3932 | } 3933 | 3934 | // Get best block locator so that we can copy it to the watchonly and solvables 3935 | + // Note: The best block locator was introduced in #152 so ancient wallets do not have it 3936 | CBlockLocator best_block_locator; 3937 | - if (!local_wallet_batch.ReadBestBlock(best_block_locator)) { 3938 | - return util::Error{_("Error: Unable to read wallet's best block locator record")}; 3939 | - } 3940 | + (void)local_wallet_batch.ReadBestBlock(best_block_locator);
This could result in a corrupt best_block_locator, later getting written in non-corrupt form. Is that the behaviour we want?
Yes. The locator per se is not important. If it is missing or corrupted, the wallet will rescan the chain before completing migration and store the correct one. Its sole purpose is to track how far the wallet has scanned the chain.
1638 | + data = bytearray(wallet_dat_path.read_bytes()) 1639 | + idx = data.find(key) 1640 | + assert idx != -1, f"{key!r} not found in wallet.dat" 1641 | + 1642 | + # Zero the byte immediately after the key (CompactSize vector length) 1643 | + data[idx + len(key)] = 0x00
Wouldn't it be better to change the key, so it's actually missing?
Have you tried it? I think I tried it back then and it didn't work but.. maybe I'm mixing it with some other bug fix.
Seems to work:
diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
index 404da94b8e0..aab3ac52c79 100755
--- a/test/functional/wallet_migration.py
+++ b/test/functional/wallet_migration.py
@@ -1640,7 +1640,8 @@ class WalletMigrationTest(BitcoinTestFramework):
assert idx != -1, f"{key!r} not found in wallet.dat"
# Zero the byte immediately after the key (CompactSize vector length)
- data[idx + len(key)] = 0x00
+ for i in range(idx, idx + len(key)):
+ data[i] = 0
wallet_dat_path.write_bytes(data)
Done as suggested. Thx.
1652 | + 1653 | + # Erase block locator record like if this would be a pre-#152 wallet 1654 | + self.erase_bdb_record(self.old_node.wallets_path / wallet_name / "wallet.dat", b"bestblock") 1655 | + 1656 | + shutil.copytree(self.old_node.wallets_path / wallet_name, self.master_node.wallets_path / wallet_name, dirs_exist_ok=True) 1657 | + self.master_node.migratewallet(wallet_name)
Could check the debug.log that the rescan is actually happening
Done as suggested.
1676 | + wallet_name = "nobestblock" 1677 | + wallet = self.create_legacy_wallet(wallet_name) 1678 | + wallet.unloadwallet() 1679 | + 1680 | + # Erase block locator record like if this would be a pre-#152 wallet 1681 | + self.erase_bdb_record(self.old_node.wallets_path / wallet_name / "wallet.dat", b"bestblock")
In 7740cd59966a90a388ae93a4ba7a040f6f08a250 "test: add coverage for migrating ancient wallets"
erase_bdb_record erases the first instance of bestblock here. However, there are actually 2 bestblock records, bestblock and bestblock_nomerkle. In most cases, this is probably erasing (the first part of) bestblock_nomerkle and leaving bestblock intact. So strictly speaking, this test is not testing pre #152 wallets, but rather a bit of a corruption case where bestblock is empty, but bestblock_nomerkle doesn't exist.
This should probably erase bestblock_nomerkle first.
Sure. Done as suggested.
ACK 0c8deee2ed45716a105485c6b02e19bc4dbebde0
1663 | + def erase_bdb_record(wallet_dat_path, key): 1664 | + data = bytearray(wallet_dat_path.read_bytes()) 1665 | + idx = data.find(key) 1666 | + assert idx != -1, f"{key!r} not found in wallet.dat" 1667 | + 1668 | + # Zero the byte immediately after the key (CompactSize vector length)
In f8b74f57636d8435fa4d4e67c55bc041b8fbb8fd "test: add coverage for migrating ancient wallets"
The comment is no longer accurate. The entire key except for the compact size is being zero'd.
Removed.
Re-ACK 673b5bc7f74d0e3a1d6cbbde451c85b1bc5db1fe
nit: outdated comment.
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 422a3661a8..7f2c88ac74 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3969,7 +3969,7 @@ util::Result<void> CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch,
LOCK(data.watchonly_wallet->cs_wallet);
data.watchonly_wallet->nOrderPosNext = nOrderPosNext;
watchonly_batch->WriteOrderPosNext(data.watchonly_wallet->nOrderPosNext);
- // Write the best block locator to avoid rescanning on reload
+ // Write the locator record. An empty locator is valid and triggers rescan on load.
if (!watchonly_batch->WriteBestBlock(best_block_locator)) {
return util::Error{_("Error: Unable to write watchonly wallet best block locator record")};
}
@@ -3978,7 +3978,7 @@ util::Result<void> CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch,
if (data.solvable_wallet) {
solvables_batch = std::make_unique<WalletBatch>(data.solvable_wallet->GetDatabase());
if (!solvables_batch->TxnBegin()) return util::Error{strprintf(_("Error: database transaction cannot be executed for wallet %s"), data.solvable_wallet->GetName())};
- // Write the best block locator to avoid rescanning on reload
+ // Write the locator record. An empty locator is valid and triggers rescan on load.
if (!solvables_batch->WriteBestBlock(best_block_locator)) {
return util::Error{_("Error: Unable to write solvable wallet best block locator record")};
}
Great, applied. Thanks.
ACK 673b5bc7f74d0e3a1d6cbbde451c85b1bc5db1fe
The best block locator was introduced in #152, previously created
wallets do not have these record.
Pre-#152 wallets have no best block stored. Test we
can migrate them.
reACK b86c1c443d89a50547efd422635e378b5e8f5e87
Re-ACK b86c1c443d89a50547efd422635e378b5e8f5e87
ACK b86c1c443d89a50547efd422635e378b5e8f5e87