Repository: bitcoin/bitcoin (language: cpp)

Issue:
bench: WalletMigration benchmark broken
master @ cdc32994feadf3f15df3cfac5baae36b4b011462
```bash
export CC=clang
export CXX=clang++
cmake -B build -DBUILD_BENCH=ON
cmake --build build
./build/bin/bench_bitcoin -filter=WalletMigration
bench_bitcoin: wallet/wallet.cpp:1772: void wallet::CWallet::InitWalletFlags(uint64_t): Assertion `m_wallet_flags == 0' failed.
Aborted (core dumped)
```


Discussion / hints:
I'll take a look...
I think I got it, will create a PR soon.
Looks like this is a "low" benchmark, but it would be good to run those at least once in the CI. Maybe in the asan task? Otherwise, issues like this could be missed.

I guess this is also the reason why corecheck is down?

Ref https://corecheck.dev/bitcoin/bitcoin/pulls/32273

cc @m3dwards 
I think they should just be run in any task that runs the benchmarks (i.e drop the usage of -priority-level). Otherwise, if benchmarks are so slow that they can't be run "normally" (or under a single iteration, which was introduced so that benchmarks could be run in the CI iirc), they should probably just be deleted; especially if nobody is even going to run them, when changing related code.
Incomplete observations from debugging:

One part of the problem is that [`HasLegacyRecords()`](https://github.com/bitcoin/bitcoin/blob/cdc32994feadf3f15df3cfac5baae36b4b011462/src/wallet/walletdb.cpp#L549-L570) is returning false for some reason in the migration benchmark case, that's why [`success` is true when it shouldn't be](https://github.com/bitcoin/bitcoin/blob/cdc32994feadf3f15df3cfac5baae36b4b011462/src/wallet/wallet.cpp#L4538-L4543).

The wallet is not blank, migration has not been performed, but `success == true`, so it does seem right to crash here.

-------

The wallet migration benchmark creates a "legacy" wallet using SQLite since #31241, maybe this has something to do with why `HasLegacyRecords()` isn't working as intended.
> One part of the problem is that [`HasLegacyRecords()`](https://github.com/bitcoin/bitcoin/blob/cdc32994feadf3f15df3cfac5baae36b4b011462/src/wallet/walletdb.cpp#L549-L570) is returning false for some reason in the migration benchmark case, that's why [`success` is true when it shouldn't be](https://github.com/bitcoin/bitcoin/blob/cdc32994feadf3f15df3cfac5baae36b4b011462/src/wallet/wallet.cpp#L4538-L4543).

The problem is that the scripts and keys created are not being saved/ written into the DB (mocked/ in memory), so if those objs are persisted the test and `HasLegacyRecords` work both fine. I'll create the PR asap.


Fix patch:
diff --git a/src/bench/wallet_migration.cpp b/src/bench/wallet_migration.cpp
index 5305833131a05..ca28ab0448789 100644
--- a/src/bench/wallet_migration.cpp
+++ b/src/bench/wallet_migration.cpp
@@ -32,6 +32,7 @@ static void WalletMigration(benchmark::Bench& bench)
     std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(test_setup->m_node.chain.get(), "", CreateMockableWalletDatabase());
     wallet->chainStateFlushed(ChainstateRole::NORMAL, CBlockLocator{});
     LegacyDataSPKM* legacy_spkm = wallet->GetOrCreateLegacyDataSPKM();
+    WalletBatch batch{wallet->GetDatabase()};
 
     // Add watch-only addresses
     std::vector<CScript> scripts_watch_only;
@@ -42,6 +43,7 @@ static void WalletMigration(benchmark::Bench& bench)
         const CScript& script = scripts_watch_only.emplace_back(GetScriptForDestination(dest));
         assert(legacy_spkm->LoadWatchOnly(script));
         assert(wallet->SetAddressBook(dest, strprintf("watch_%d", w), /*purpose=*/std::nullopt));
+        batch.WriteWatchOnly(script, CKeyMetadata());
     }
 
     // Generate transactions and local addresses
@@ -58,6 +60,7 @@ static void WalletMigration(benchmark::Bench& bench)
         mtx.vout.emplace_back(COIN, scripts_watch_only.at(j % NUM_WATCH_ONLY_ADDR));
         mtx.vin.resize(2);
         wallet->AddToWallet(MakeTransactionRef(mtx), TxStateInactive{}, /*update_wtx=*/nullptr, /*fFlushOnClose=*/false, /*rescanning_old_block=*/true);
+        batch.WriteKey(pubkey, key.GetPrivKey(), CKeyMetadata());
     }
 
     bench.epochs(/*numEpochs=*/1).run([&context, &wallet] {


Test patch: