wallet: fix ancient wallets migration

furszy commented at 8:03 pm on January 3, 2026: member

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.

Note: Originally I wrote the test using a BDB library (bsddb3) but I wasn’t happy with the new dependency, and ended up zeroing the record length manually. I’m not sure this will work on every platform. If it doesn’t, we can just just drop the test as this can be tested manually on any ancient wallet too.

DrahtBot added the label Wallet on Jan 3, 2026

DrahtBot commented at 8:03 pm on January 3, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34198.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	bensig

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

LLM Linter (✨ experimental)

Possible places where comparison-specific test macros should replace generic comparisons:

test/functional/wallet_migration.py: 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

bensig commented at 0:45 am on January 5, 2026: contributor

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.

DrahtBot added the label Needs rebase on Jan 7, 2026

furszy force-pushed on Jan 7, 2026

DrahtBot removed the label Needs rebase on Jan 7, 2026

DrahtBot added the label CI failed on Jan 7, 2026

DrahtBot removed the label CI failed on Jan 19, 2026

in src/wallet/wallet.cpp:3955 in 785eadf03f outdated

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);

luke-jr commented at 2:13 pm on January 22, 2026:

This could result in a corrupt best_block_locator, later getting written in non-corrupt form. Is that the behaviour we want?

furszy commented at 3:08 pm on January 22, 2026:

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.

in test/functional/wallet_migration.py:1643 in 785eadf03f

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

luke-jr commented at 2:13 pm on January 22, 2026:

Wouldn’t it be better to change the key, so it’s actually missing?

furszy commented at 3:11 pm on January 22, 2026:

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.

achow101 commented at 9:15 pm on January 22, 2026:

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)

furszy commented at 9:46 pm on January 22, 2026:

Done as suggested. Thx.

in test/functional/wallet_migration.py:1657 in 785eadf03f

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)

achow101 commented at 9:16 pm on January 22, 2026:

Could check the debug.log that the rescan is actually happening

furszy commented at 9:45 pm on January 22, 2026:

Done as suggested.

wallet: fix ancient wallets migration

The best block locator was introduced in #152, previously created
wallets do not have these record.

c0edd9ff66

furszy force-pushed on Jan 22, 2026

in test/functional/wallet_migration.py:1682 in 7740cd5996 outdated

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")

achow101 commented at 9:49 pm on January 26, 2026:

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.

furszy commented at 10:55 pm on January 26, 2026:

Sure. Done as suggested.

test: add coverage for migrating ancient wallets

Pre-#152 wallets have no best block stored. Test we
can migrate them.

0c8deee2ed

furszy force-pushed on Jan 26, 2026

DrahtBot added the label CI failed on Jan 27, 2026

DrahtBot removed the label CI failed on Jan 27, 2026

wallet: fix ancient wallets migration #34198

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)