bench: close wallets after migration

l0rinc commented at 10:15 am on April 19, 2025: contributor

Fixes the failure after https://github.com/bitcoin/bitcoin/commit/27f11217ca63e0f8f78f14db139150052dcd9962 The performance of the benchmark is the same as before the change

DrahtBot commented at 10:15 am on April 19, 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/32309.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29694 (fuzz: wallet: add target for spkm migration by brunoerg)

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 19, 2025

in src/bench/wallet_migration.cpp:73 in 9035f05142 outdated

67@@ -68,6 +68,9 @@ static void WalletMigration(benchmark::Bench& bench)
68         assert(res);
69         assert(res->wallet);
70         assert(res->watchonly_wallet);
71+
72+        res->wallet->Close();
73+        res->watchonly_wallet->Close();

hebasto commented at 10:22 am on April 19, 2025:

Should the fact that the migrated wallet databases remain open be documented in MigrateLegacyToDescriptor?

l0rinc commented at 10:27 am on April 19, 2025:

I needed to close both for the build to pass (res->wallet->Close() alone wasn’t enough). Maybe it passes with only res->watchonly_wallet->Close() - I can check, if you think it makes sense.

hebasto commented at 10:46 am on April 19, 2025:

~~I wonder why res->watchonly_wallet.use_count() == 3? Is this the expected behavior of the MigrateLegacyToDescriptor() function?~~

UPD. I’ve figured it out. Seems fine to me.

l0rinc commented at 10:55 am on April 19, 2025:

Maybe @furszy knows more - is that not what you would expect?

hebasto commented at 11:05 am on April 19, 2025:

Unfortunately, this patch does not fix the test case posted here.

l0rinc commented at 11:28 am on April 19, 2025:

Maybe it passes with only res->watchonly_wallet->Close() - I can check, if you think it makes sense

No, we do need to close both: https://github.com/l0rinc/bitcoin/actions/runs/14548535133/job/40817048030?pr=9

l0rinc commented at 11:51 am on April 19, 2025:

Unfortunately, this patch does not fix the test case posted #32291 (comment).

I can reproduce the failure with

0cmake --preset=libfuzzer -DBUILD_FOR_FUZZING=OFF -DBUILD_BENCH=ON && cmake --build build_fuzz -j$(nproc) && build_fuzz/bin/bench_bitcoin -filter='WalletMigration' --min-time=10000

I pushed a new version based on https://github.com/bitcoin/bitcoin/blob/master/src/bench/wallet_create.cpp#L54 which calls RemoveWallet instead - @hebasto, can you please check if this fixes it for you as well?

l0rinc commented at 7:56 pm on April 19, 2025:

Pushed again, please recheck

hebasto added the label Wallet on Apr 19, 2025

l0rinc marked this as ready for review on Apr 19, 2025

l0rinc force-pushed on Apr 19, 2025

hebasto commented at 11:52 am on April 19, 2025: member

In commit 86646448770888f66c446eacdd47d1a62d5552eb, “… we’re only running 1 epochs anyway” – where does this assumption come from?

l0rinc commented at 11:53 am on April 19, 2025: contributor

where does this assumption come from

0bench.epochs(/*numEpochs=*/1)

maflcko commented at 12:00 pm on April 19, 2025: member

needs to revert d91a746815e4428c738f1a096a950292cbdb5469 here in this pull

hebasto commented at 12:00 pm on April 19, 2025: member

Tested 86646448770888f66c446eacdd47d1a62d5552eb on Ubuntu 24.04:

0$ ./build/bin/bench_bitcoin -filter=WalletMigration -min-time=100000
1Warning, results might be unstable:
2* CPU frequency scaling enabled: CPU 0 between 800.0 and 4,800.0 MHz
3* CPU governor is 'powersave' but should be 'performance'
4* Turbo is enabled, CPU frequency will fluctuate
5
6Recommendations
7* Use 'pyperf system tune' before benchmarking. See https://github.com/psf/pyperf
8Segmentation fault (core dumped)

l0rinc commented at 12:03 pm on April 19, 2025: contributor

Segmentation fault (core dumped)

Can you show me the full build command please, cmake --preset=libfuzzer -DBUILD_FOR_FUZZING=OFF -DBUILD_BENCH=ON && cmake --build build_fuzz -j$(nproc) && build_fuzz/bin/bench_bitcoin -filter='WalletMigration' --min-time=10000 passes for me on Ubuntu.

hebasto commented at 12:06 pm on April 19, 2025: member

Segmentation fault (core dumped)

Can you show me the full build command please, cmake --preset=libfuzzer -DBUILD_FOR_FUZZING=OFF -DBUILD_BENCH=ON && cmake --build build_fuzz -j$(nproc) && build_fuzz/bin/bench_bitcoin -filter='WalletMigration' --min-time=10000 passes for me on Ubuntu.

 0$ rm -rf build && cmake -B build -DBUILD_BENCH=ON
 1$ cmake --build build -t bench_bitcoin
 2$ ./build/bin/bench_bitcoin -filter=WalletMigration -min-time=100000
 3Warning, results might be unstable:
 4* CPU frequency scaling enabled: CPU 0 between 800.0 and 4,800.0 MHz
 5* CPU governor is 'powersave' but should be 'performance'
 6* Turbo is enabled, CPU frequency will fluctuate
 7
 8Recommendations
 9* Use 'pyperf system tune' before benchmarking. See https://github.com/psf/pyperf
10Segmentation fault (core dumped)

bench: use a stable `SecureString` for wallet migration

Introduce a long‑lived `SecureString` passphrase instead of passing a string literal into `MigrateLegacyToDescriptor`.
This avoids constructing a temporary `SecureString` per invocation and prevents dangling‑pointer issues inside the migration logic.

e0c95cc123

bench: ensure wallet migration benchmark runs exactly once

Limit the wallet migration microbenchmark to a single execution by configuring both epochs(1) and epochIterations(1). This guarantees the body of run() is invoked exactly one time.

367d217d28

bench: clean up both migrated descriptor wallets to avoid leaks

After `MigrateLegacyToDescriptor` we end up with two new descriptor wallets – the primary (spendable) wallet and a watch‑only wallet.
Unload both to prevent lingering state in our benchmark harness.

1ef015a099

ci: re‑enable all benchmark runs

Drop the temporary filter that excluded only the `WalletMigration` benchmark as part of the test suite.

2fa7dd26e3

l0rinc force-pushed on Apr 19, 2025

mzumsande commented at 8:38 pm on April 19, 2025: contributor

Would be good to explain 1.) Why it failed before and 2.) How this is fixed Looking at the commits, it seems that there are two contradictory things going on: Making sure that the benchmarks could have multiple invocations (commit 1 and 3), and making sure that it actually only runs once (commit 2).

bench: close wallets after migration #32309

Code Coverage & Benchmarks

Reviews

Conflicts