bench: Fix WalletMigration benchmark

pablomartin4btc commented at 10:22 pm on April 15, 2025: member

The keys and scripts created for the Legacy Wallet needed to be persisted in order for the migration to work properly.

Fixes #32277.

DrahtBot commented at 10:22 pm on April 15, 2025: 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/32281.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101, davidgumberg, furszy

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28710 (Remove the legacy wallet and BDB dependency by achow101)

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.

DrahtBot added the label Tests on Apr 15, 2025

in src/wallet/walletdb.cpp:572 in 43410fedb5 outdated

568@@ -569,6 +569,22 @@ bool HasLegacyRecords(CWallet& wallet, DatabaseBatch& batch)
569     return false;
570 }
571 
572+bool HasAnyRecordWithKeyPrefix(CWallet& wallet, const std::string& dbkey_prefix)

achow101 commented at 11:03 pm on April 15, 2025:

I don’t think this is really necessary, especially since it’s test only.

pablomartin4btc commented at 11:12 pm on April 15, 2025:

Ok, I can move it to the bench file.

furszy commented at 11:49 pm on April 15, 2025:

Could just remove it.

in src/bench/wallet_migration.cpp:46 in 43410fedb5 outdated

41@@ -42,6 +42,11 @@ static void WalletMigration(benchmark::Bench& bench)
42         const CScript& script = scripts_watch_only.emplace_back(GetScriptForDestination(dest));
43         assert(legacy_spkm->LoadWatchOnly(script));
44         assert(wallet->SetAddressBook(dest, strprintf("watch_%d", w), /*purpose=*/std::nullopt));
45+        {
46+            WalletBatch batch(wallet->GetDatabase());

furszy commented at 11:51 pm on April 15, 2025:

Can create a single batch instance outside the loop and use it everywhere. The “database” on this test is a plain map in memory. No thread synchronization is needed, writes are done sequential.

in src/bench/wallet_migration.cpp:68 in 43410fedb5 outdated

62@@ -58,6 +63,11 @@ static void WalletMigration(benchmark::Bench& bench)
63         mtx.vout.emplace_back(COIN, scripts_watch_only.at(j % NUM_WATCH_ONLY_ADDR));
64         mtx.vin.resize(2);
65         wallet->AddToWallet(MakeTransactionRef(mtx), TxStateInactive{}, /*update_wtx=*/nullptr, /*fFlushOnClose=*/false, /*rescanning_old_block=*/true);
66+        {
67+            WalletBatch batch(wallet->GetDatabase());
68+            batch.WriteKey(pubkey, key.GetPrivKey(), CKeyMetadata());

furszy commented at 11:56 pm on April 15, 2025:

The empty metadata field smells bad. Double check migration.

pablomartin4btc commented at 3:32 pm on April 16, 2025:

I’ve checked that keys (500 txs on local addresses) and scripts (20 wo imported addresses) are being migrated correctly (the first at ApplyMigrationData in for (auto& desc_spkm : data.desc_spkms) { and the second at DoMigration in for (const auto& [desc_str, creation_time] : data->watch_descs) {).

achow101 commented at 6:02 pm on April 16, 2025:

I think empty metadata is fine, there isn’t really much metadata to be had for imports and randomly generated keys. It’s basically just the creation time.

bench: Fix WalletMigration benchmark

The keys and scripts created for the Legacy Wallet
needed to be persisted in order for the migration to work
properly.

7912cd4125

pablomartin4btc force-pushed on Apr 16, 2025

pablomartin4btc commented at 3:42 pm on April 16, 2025: member

Updates:

Addressed feedbacks from @achow101 and @furszy.

furszy commented at 5:28 pm on April 16, 2025: member

Can you comment on #32277 (comment)?

achow101 commented at 6:11 pm on April 16, 2025: member

ACK 7912cd41258d59e41f3c961a3be35a8921194670

in src/bench/wallet_migration.cpp:45 in 7912cd4125

42@@ -42,6 +43,7 @@ static void WalletMigration(benchmark::Bench& bench)
43         const CScript& script = scripts_watch_only.emplace_back(GetScriptForDestination(dest));
44         assert(legacy_spkm->LoadWatchOnly(script));
45         assert(wallet->SetAddressBook(dest, strprintf("watch_%d", w), /*purpose=*/std::nullopt));

davidgumberg commented at 0:25 am on April 17, 2025:

For context on my comment in #32277 (comment), SetAddressBook() includes a write to the database, and so does AddToWallet(tx)

This is the reason why some records were persisted, but none that were strictly “legacy” records as mentioned in #32277 (comment).

(@furszy)

davidgumberg commented at 0:50 am on April 17, 2025: contributor

Tested ACK 7912cd41258d59e4

Either of the two DB writes added here seems to fix the issue in my testing, but it’s better to be consistent and write both.

I am no longer confident the change to success introduced in #32149 makes sense:

https://github.com/bitcoin/bitcoin/blob/e66e30c9e5383a467789574e61114b57536193b9/src/wallet/wallet.cpp#L4538-L4544

Now, any wallet which fails the HasLegacyRecords() check gets flagged as success = true, before it was only wallets with the blank flag set which got success = true for free. On second thought, should #32149 have explicitly handled the empty watchonly wallet case, rather than having to reason about the correctness/completeness of HasLegacyRecords()

This bug was an instance of a more general class of crash during migration: #32111. This would still be a bug if #32111 was fixed, but might not have resulted in a crash.

furszy commented at 2:40 am on April 17, 2025: member

utACK 7912cd41258d5

fanquake merged this on Apr 17, 2025

fanquake closed this on Apr 17, 2025

bitcoin deleted a comment on Apr 17, 2025

bench: Fix WalletMigration benchmark #32281

Code Coverage & Benchmarks

Reviews

Conflicts