bench: close wallets after migration

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

The low-priority WalletMigration benchmark existed for some time but was never run automatically in our CI.

Although the failure first surfaced on Windows as a hang during temporary directory cleanup, it could also be reproduced on Linux and macOS when forcing multiple iterations (e.g. via a long --min-time).

Root causes

Leaked open wallets on Windows MigrateLegacyToDescriptor produces two new descriptor wallets (the primary spendable wallet and a companion watch‑only wallet). Without unloading them, their database files remained open in the WalletContext, blocking directory removal and hanging the test harness.

0what():  filesystem error: cannot remove all: The process cannot access the file because it is being used by another process [C:\Users\RUNNER\~1\AppData\Local\Temp\test_common bitcoin\WalletMigration\d8ffd89a7700ce01c31f] [C:\Users\RUNNER~1\AppData\Local\Temp\test_common bitcoin\WalletMigration\d8ffd89a7700ce01c31f\regtest\wallet.dat]

Undefined behavior on repeated runs The benchmark body calls std::move(wallet), invalidating the local wallet pointer. Running more than one iteration causes a use-after-move by the sanitizers.

0error: bench_bitcoin 0x00067927: DW_TAG_member '_M_local_buf' refers to type 0x00000000000b3ba7 which extends beyond the bounds of 0x0006791d
1* thread [#1](/bitcoin-bitcoin/1/), name = 'b-test', stop reason = signal SIGSEGV: address not mapped to object (fault address: 0xc8)
2  * frame [#0](/bitcoin-bitcoin/0/): 0x00005555556a3f33 bench_bitcoin`... basic_string<char>::length(this=<unavailable>) const at basic_string.h:1079:16

Fixes

Automatic wallet teardown Wrap the benchmark in a MakeWalletLoader (owning a WalletContext), so that both migrated wallets are unloaded when the loader goes out of scope, eliminating any lingering open files.
Re-enable benchmarks in CI Drop the temporary filter in GitHub Actions. The -sanity-check run already executes each benchmark once, so WalletMigration now runs automatically without hangs or crashes.
Single iteration Configure the microbenchmark with .epochs(1).epochIterations(1), ensuring the migration code runs exactly once and avoiding use-after-move.

No measurable change in benchmark performance.

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.

Type	Reviewers
ACK	furszy, maflcko, hebasto
Concept ACK	mzumsande, shahsb, achow101

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:

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

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

l0rinc commented at 9:15 pm on April 19, 2025: contributor

Thanks, I have updated the description, let me know if it answered your questions.

Short answer anyway: we have to clean up regardless of the execution count - and the benchmark seems to have been designed for a single execution anyway, otherwise recreating the wallet might have been too costly.

Let me know if you have a better idea.

mzumsande commented at 2:15 am on April 20, 2025: contributor

Thanks, I didn’t realize that there were multiple independent issues - makes sense now, Concept ACK.

furszy commented at 5:13 pm on April 20, 2025: member

On Windows, benchmark runs sporadically hung because the descriptor wallet and its companion watch-only wallet were never unloaded, remaining registered in the global WalletContext. This prevented the test harness from cleaning up its temporary directory.

Hmm, haven’t dug deeper but this does not seem to be completely accurate. The WalletContext is not global. It is created at the beginning of the benchmark, just after creating the test context (which is the one that calls to the fs::removal_all that crashes during destruction).

That said, I think you could just construct a wallet loader instead of manually removing the wallets in the code we want to benchmark. The loader internally creates a WalletContext and unloads all wallets during destruction. E.g.

 0diff --git a/src/bench/wallet_migration.cpp b/src/bench/wallet_migration.cpp
 1--- a/src/bench/wallet_migration.cpp
 2+++ b/src/bench/wallet_migration.cpp
 3@@ -5,6 +5,7 @@
 4 #include <bench/bench.h>
 5 #include <kernel/chain.h>
 6 #include <interfaces/chain.h>
 7+#include <interfaces/wallet.h>
 8 #include <node/context.h>
 9 #include <test/util/mining.h>
10 #include <test/util/setup_common.h>
11@@ -21,9 +22,7 @@
12 {
13     const auto test_setup = MakeNoLogFileContext<TestingSetup>();
14
15-    WalletContext context;
16-    context.args = &test_setup->m_args;
17-    context.chain = test_setup->m_node.chain.get();
18+    std::unique_ptr<interfaces::WalletLoader> loader = MakeWalletLoader(*test_setup->m_node.chain, test_setup->m_args);
19 
20     // Number of imported watch only addresses
21     int NUM_WATCH_ONLY_ADDR = 20;
22@@ -63,6 +62,7 @@
23         batch.WriteKey(pubkey, key.GetPrivKey(), CKeyMetadata());
24     }
25 
26+    WalletContext& context{*loader->context()};
27     bench.epochs(/*numEpochs=*/1).run([&context, &wallet] {
28         util::Result<MigrationResult> res = MigrateLegacyToDescriptor(std::move(wallet), /*passphrase=*/"", context, /*was_loaded=*/false);
29         assert(res);

Also, if you want to double-ensure unload before test context destruction, you could manually delete the loader at the end of the benchmark function (outside the benchmark lambda).

The benchmark harness executed the benchmark lambda multiple times unless explicitly told not to. After the first invocation moved the wallet (std::move(wallet)), subsequent iterations operated on a moved-from object, leading to undefined behavior. While re-creating the wallet in each iteration would have worked, it defeats the purpose of measuring migration performance. Since the benchmark is already slow and likely intended to run only once, the iteration count was capped at one.

When we run the benchmark in the CI, we provide the -sanity-check flag, which already sets bench.epochs(1).epochIterations(1). While it’s good to also set this inside the benchmark for standalone runs, this change shouldn’t be related to the CI issue.

in src/bench/wallet_migration.cpp:68 in e0c95cc123 outdated

62@@ -63,8 +63,9 @@ static void WalletMigration(benchmark::Bench& bench)
63         batch.WriteKey(pubkey, key.GetPrivKey(), CKeyMetadata());
64     }
65 
66-    bench.epochs(/*numEpochs=*/1).run([&context, &wallet] {
67-        util::Result<MigrationResult> res = MigrateLegacyToDescriptor(std::move(wallet), /*passphrase=*/"", context, /*was_loaded=*/false);
68+    const SecureString passphrase{};
69+    bench.epochs(/*numEpochs=*/1).run([&context, &wallet, &passphrase] {
70+        util::Result<MigrationResult> res = MigrateLegacyToDescriptor(std::move(wallet), passphrase, context, /*was_loaded=*/false);

maflcko commented at 11:36 am on April 21, 2025:

q in e0c95cc123351681ab4326adb746debba33129f0: What are the exact steps to reproduce this “dangling-pointer”? Also, what is the explanation where it is destructed early and where it is used after free?

l0rinc commented at 10:52 am on April 22, 2025:

This wasn’t needed in the end, removed in latest push

shahsb commented at 4:24 pm on April 21, 2025: none

Concept ACK

achow101 commented at 8:29 pm on April 21, 2025: member

Is a benchmark that runs only once actually useful?

l0rinc commented at 8:37 pm on April 21, 2025: contributor

Not sure, we can decide to delete it as well - I’ll push tomorrow a fixed version based on @furszy’s recommendation (but have to retest it first after my benchmarking servers free up again). @furszy, is this still relevant or would deleting it be simpler?

achow101 commented at 9:05 pm on April 21, 2025: member

I guess it is useful as a spot check, but the accuracy is not very good.

Concept ACK

Tested on Windows and it worked. Was also able to reproduce failure 3, but don’t know how to reproduce failure 2.

furszy commented at 0:29 am on April 22, 2025: member

Not sure, we can decide to delete it as well - I’ll push tomorrow a fixed version based on @furszy’s recommendation (but have to retest it first after my benchmarking servers free up again). @furszy, is this still relevant or would deleting it be simpler?

I don’t think anyone is planning to touch the legacy wallet code anymore, and the migration procedure no longer takes an hour either (which was the reason behind this benchmark creation). So I’d say it’s no longer relevant as a benchmark.

Also, agree with achow. Since the introduction of the read-only BDB class, this benchmark no longer covers db reads either. So it’s not realistically accurate.

I believe we’ve kept it merely as a sanity check because it’s the only test that will survive the annihilation of the legacy wallet code.

bench: clean up migrated descriptor wallets via loader teardown

`MigrateLegacyToDescriptor` returns both a spendable descriptor wallet and a watch‑only wallet.
If these remain attached, their files stay open and on Windows this can hang CI when removing the test directory.

By constructing them via `MakeWalletLoader` (which owns the `WalletContext`), both wallets are automatically unloaded when the loader is destroyed at the end.
This ensures no lingering handles or resource leaks when running the benchmark on CI with `-sanity-check`.

Co-authored-by: furszy <matiasfurszyfer@protonmail.com>

1da11dbc44

ci: re-enable all benchmark runs

Drop the temporary `-filter` that excluded the `WalletMigration` benchmark.

c1f458aaa0

bench: ensure wallet migration benchmark runs exactly once

The migration benchmark crashes if run more than once, because of `std::move(wallet)` and leaves subsequent iterations in an undefined state - avoiding `UndefinedBehaviorSanitizer` null‑dereference error.

cad39f86fb

l0rinc force-pushed on Apr 22, 2025

l0rinc commented at 10:55 am on April 22, 2025: contributor

Thanks for the suggestions @furszy, @achow101 and @maflcko. Updated the commits to use MakeWalletLoader instead of manually closing, and moved the epochIterations(1) after the CI change to obviate that the CI already does that.

TheCharlatan commented at 11:02 am on April 22, 2025: contributor

I believe we’ve kept it merely as a sanity check because it’s the only test that will survive the annihilation of the legacy wallet code.

Sounds like this should be an actual test then instead of a benchmark, no?

l0rinc commented at 11:17 am on April 22, 2025: contributor

Sounds like this should be an actual test then instead of a benchmark, no?

I also asked the same. I suggest we fix and reenable full CI-validation first, and we can create a detailed test with proper asserts separately - since I’m not a wallet expert (yet?)

furszy commented at 2:31 pm on April 22, 2025: member

utACK cad39f86fb5a81

Sounds like this should be an actual test then instead of a benchmark, no?

I’d keep it as is for now and drive all our focus towards #31250. It would be nice to cut down the tree first - we can pick up any leftover later.

DrahtBot requested review from achow101 on Apr 22, 2025

DrahtBot requested review from shahsb on Apr 22, 2025

DrahtBot requested review from mzumsande on Apr 22, 2025

in src/bench/wallet_migration.cpp:70 in 1da11dbc44 outdated

60@@ -63,12 +61,13 @@ static void WalletMigration(benchmark::Bench& bench)
61         batch.WriteKey(pubkey, key.GetPrivKey(), CKeyMetadata());
62     }
63 
64-    bench.epochs(/*numEpochs=*/1).run([&context, &wallet] {
65-        util::Result<MigrationResult> res = MigrateLegacyToDescriptor(std::move(wallet), /*passphrase=*/"", context, /*was_loaded=*/false);
66-        assert(res);
67-        assert(res->wallet);
68-        assert(res->watchonly_wallet);

maflcko commented at 2:52 pm on April 22, 2025:

style nit in 1da11dbc441790773502ffd5f60dc05191514a83: It would be good to use clang-format. This would reduce the whitespace churn of this commit.

l0rinc commented at 3:10 pm on April 22, 2025:

you mean like this, i.e. one less space for the block?

0bench.epochs(/*numEpochs=*/1).epochIterations(/*numIters=*/1) // run the migration exactly once
1    .run([&] {

What do you mean exactly by “would reduce the whitespace churn of this commit”, we still need to indent it (and the methods are aligned this way, I like that more). You can convince me otherwise, but if it’s a nit, I’d prefer the current one.

Or if you meant

0bench.epochs(/*numEpochs=*/1).epochIterations(/*numIters=*/1).run([&] { // run the migration exactly once

, in which case there wouldn’t be any new indentation, but the current comment prevent clang-format from doing that automatically.

Let me know if you have strong preference here.

maflcko commented at 3:26 pm on April 22, 2025:

It is just a nit. I am just saying that for new (or touched) code, it would be better to use clang-format, according to https://github.com/bitcoin/bitcoin/blob/96a5cd8000d2b500585834277cc5f5e23181a6f3/doc/developer-notes.md?plain=1#L66. If there is an issue with clang-format, it would be good to fix it. Using clang-format consistently makes it easier when touching code in the future again.

l0rinc commented at 3:51 pm on April 22, 2025:

I’m all for consistency - but is this not what you’re getting when formatting the latest commit (except for that 1 space difference I mentioned above)? If I need to touch it again, I’ll put the run to the previous line to minimize the diff (though I consider this to be slightly more readable).

maflcko commented at 2:52 pm on April 22, 2025: member

review ACK cad39f86fb5a81f0e3b5116e8e989bab8af89718 🍥

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK cad39f86fb5a81f0e3b5116e8e989bab8af89718 🍥
3N0Yshc7sd+vM1UmJaQayP38FATXXYL4CaoysjnfZ5MKyq2UwVYCv3eW9US68YcgDTa8SUUMNXk3uHTj2PuXLAQ==

hebasto approved

hebasto commented at 4:41 pm on April 22, 2025: member

ACK cad39f86fb5a81f0e3b5116e8e989bab8af89718, tested on Ubuntu 25.04.

hebasto merged this on Apr 22, 2025

hebasto closed this on Apr 22, 2025

TheCharlatan referenced this in commit a9c46ce3c3 on Apr 24, 2025

furszy commented at 11:41 pm on April 24, 2025: member

I believe we’ve kept it merely as a sanity check because it’s the only test that will survive the annihilation of the legacy wallet code.

Best bench-test ever. Just caught a bug in #31423. It seems we have no other migration test exercising a default named wallet with spendable and watch-only scripts.

l0rinc deleted the branch on Apr 25, 2025

bench: close wallets after migration #32309

Root causes

Fixes

Code Coverage & Benchmarks

Reviews

Conflicts