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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34198.
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
No conflicts as of last run.
Possible places where comparison-specific test macros should replace generic comparisons:
assert idx != -1, f"{key!r} not found in wallet.dat" -> recommendation: use assert_not_equal(idx, -1, f"{key!r} not found in wallet.dat") instead of a bare assert.2026-01-26 22:55:23
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);
best_block_locator, later getting written in non-corrupt form. Is that the behaviour we want?
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
Seems to work:
0diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
1index 404da94b8e0..aab3ac52c79 100755
2--- a/test/functional/wallet_migration.py
3+++ b/test/functional/wallet_migration.py
4@@ -1640,7 +1640,8 @@ class WalletMigrationTest(BitcoinTestFramework):
5 assert idx != -1, f"{key!r} not found in wallet.dat"
6
7 # Zero the byte immediately after the key (CompactSize vector length)
8- data[idx + len(key)] = 0x00
9+ for i in range(idx, idx + len(key)):
10+ data[i] = 0
11
12 wallet_dat_path.write_bytes(data)
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)
The best block locator was introduced in #152, previously created
wallets do not have these record.
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.
Pre-#152 wallets have no best block stored. Test we
can migrate them.
