Merge bitcoin/bitcoin#32309: bench: close wallets after migration
Some checks are pending
CI / test each commit (push) Waiting to run
CI / macOS 14 native, arm64, no depends, sqlite only, gui (push) Waiting to run
CI / macOS 14 native, arm64, fuzz (push) Waiting to run
CI / Windows native, VS 2022 (push) Waiting to run
CI / Windows native, fuzz, VS 2022 (push) Waiting to run
CI / Linux->Windows cross, no tests (push) Waiting to run
CI / Windows, test cross-built (push) Blocked by required conditions
CI / ASan + LSan + UBSan + integer, no depends, USDT (push) Waiting to run

cad39f86fb bench: ensure wallet migration benchmark runs exactly once (Lőrinc)
c1f458aaa0 ci: re-enable all benchmark runs (Lőrinc)
1da11dbc44 bench: clean up migrated descriptor wallets via loader teardown (Lőrinc)

Pull request description:

  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

  1. **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.

  <details><summary>Details</summary>

  ```bash
  what():  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]
  ```

  </details>

  2. **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.

  <details><summary>Details</summary>

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

  </details>

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

ACKs for top commit:
  maflcko:
    review ACK cad39f86fb 🍥
  furszy:
    utACK cad39f86fb
  hebasto:
    ACK cad39f86fb, tested on Ubuntu 25.04.

Tree-SHA512: 10343ce7ab9b63ba4f51a7673018215577ea7ec188e41d535a66d69d73b85bca6ba301c33f6920c02f8f7d686c75c65c4a4e9bdafb04b60be85d66aa743cfa20
This commit is contained in:
Hennadii Stepanov 2025-04-22 17:42:12 +01:00
commit e5a00b2497
No known key found for this signature in database
GPG key ID: 410108112E7EA81F
2 changed files with 13 additions and 15 deletions

View file

@ -358,8 +358,7 @@ jobs:
./src/univalue/unitester.exe
- name: Run benchmarks
# TODO: Fix the `WalletMigration` benchmark and re-enable it.
run: ./bin/bench_bitcoin.exe -sanity-check -filter='^(?!WalletMigration$).+$'
run: ./bin/bench_bitcoin.exe -sanity-check
- name: Adjust paths in test/config.ini
shell: pwsh

View file

@ -3,14 +3,15 @@
// file COPYING or https://www.opensource.org/licenses/mit-license.php.
#include <bench/bench.h>
#include <kernel/chain.h>
#include <interfaces/chain.h>
#include <interfaces/wallet.h>
#include <kernel/chain.h>
#include <node/context.h>
#include <test/util/mining.h>
#include <test/util/setup_common.h>
#include <wallet/test/util.h>
#include <wallet/context.h>
#include <wallet/receive.h>
#include <wallet/test/util.h>
#include <wallet/wallet.h>
#include <optional>
@ -19,11 +20,8 @@ namespace wallet{
static void WalletMigration(benchmark::Bench& bench)
{
const auto test_setup = MakeNoLogFileContext<TestingSetup>();
WalletContext context;
context.args = &test_setup->m_args;
context.chain = test_setup->m_node.chain.get();
const auto test_setup{MakeNoLogFileContext<TestingSetup>()};
const auto loader{MakeWalletLoader(*test_setup->m_node.chain, test_setup->m_args)};
// Number of imported watch only addresses
int NUM_WATCH_ONLY_ADDR = 20;
@ -63,8 +61,9 @@ static void WalletMigration(benchmark::Bench& bench)
batch.WriteKey(pubkey, key.GetPrivKey(), CKeyMetadata());
}
bench.epochs(/*numEpochs=*/1).run([&context, &wallet] {
util::Result<MigrationResult> res = MigrateLegacyToDescriptor(std::move(wallet), /*passphrase=*/"", context, /*was_loaded=*/false);
bench.epochs(/*numEpochs=*/1).epochIterations(/*numIters=*/1) // run the migration exactly once
.run([&] {
auto res{MigrateLegacyToDescriptor(std::move(wallet), /*passphrase=*/"", *loader->context(), /*was_loaded=*/false)};
assert(res);
assert(res->wallet);
assert(res->watchonly_wallet);