bench, wallet: Make WalletMigration's setup WalletBatch scoped
What the developer wrote
Authored by Ava Chow
bench, wallet: Make WalletMigration's setup WalletBatch scoped
WalletBatch needs to be in a scope so that it is destroyed before the
database is closed during migration.
What changed, and why it matters
This change fixes a bug in a benchmark test program, not in the main Bitcoin wallet that users run. The benchmark creates a fake wallet to measure how long migration takes, and the fake wallet's database helper object was not being closed before the migration step tried to close the database. That could make the benchmark crash or fail, but it does not affect real wallets or network security.
No security action required. Treat as a normal code-quality/test fix. If backporting, include only for benchmark/test hygiene, not for a security fix.
Security signals we found
Resource lifetime ordering fix in benchmark code only
No change to production wallet migration logic
No cryptographic, consensus, or network handling changes
Evidence from the diff
The commit wraps the WalletBatch used during benchmark setup in a nested scope so it is destroyed before the migration runs. WalletBatch holds a database batch/transaction; if it outlives the database close during migration, the underlying database environment may be closed while the batch still references it, leading to a use-after-close or assertion failure in the benchmark. The fix is purely in src/bench/wallet_migration.cpp and only affects the benchmark harness.
Changed components
src/bench/wallet_migration.cppInspect captured patch +32 / −30
diff --git a/src/bench/wallet_migration.cpp b/src/bench/wallet_migration.cpp
index 910ad1e9..7d7aca11 100644
--- a/src/bench/wallet_migration.cpp
+++ b/src/bench/wallet_migration.cpp
@@ -29,40 +29,42 @@ static void WalletMigration(benchmark::Bench& bench)
// Setup legacy wallet
std::unique_ptr<CWallet> wallet = std::make_unique<CWallet>(test_setup->m_node.chain.get(), "", CreateMockableWalletDatabase());
- LegacyDataSPKM* legacy_spkm = wallet->GetOrCreateLegacyDataSPKM();
- WalletBatch batch{wallet->GetDatabase()};
+ {
+ LegacyDataSPKM* legacy_spkm = wallet->GetOrCreateLegacyDataSPKM();
+ WalletBatch batch{wallet->GetDatabase()};
- // Write a best block record as migration expects one to exist
- CBlockLocator loc;
- batch.WriteBestBlock(loc);
+ // Write a best block record as migration expects one to exist
+ CBlockLocator loc;
+ batch.WriteBestBlock(loc);
- // Add watch-only addresses
- std::vector<CScript> scripts_watch_only;
- for (int w = 0; w < NUM_WATCH_ONLY_ADDR; ++w) {
- CKey key = GenerateRandomKey();
- LOCK(wallet->cs_wallet);
- const PKHash dest{key.GetPubKey()};
- 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());
- }
+ // Add watch-only addresses
+ std::vector<CScript> scripts_watch_only;
+ for (int w = 0; w < NUM_WATCH_ONLY_ADDR; ++w) {
+ CKey key = GenerateRandomKey();
+ LOCK(wallet->cs_wallet);
+ const PKHash dest{key.GetPubKey()};
+ 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
- for (int j = 0; j < 500; ++j) {
- CKey key = GenerateRandomKey();
- CPubKey pubkey = key.GetPubKey();
- // Load key, scripts and create address book record
- Assert(legacy_spkm->LoadKey(key, pubkey));
- CTxDestination dest{PKHash(pubkey)};
- Assert(wallet->SetAddressBook(dest, strprintf("legacy_%d", j), /*purpose=*/std::nullopt));
+ // Generate transactions and local addresses
+ for (int j = 0; j < 500; ++j) {
+ CKey key = GenerateRandomKey();
+ CPubKey pubkey = key.GetPubKey();
+ // Load key, scripts and create address book record
+ Assert(legacy_spkm->LoadKey(key, pubkey));
+ CTxDestination dest{PKHash(pubkey)};
+ Assert(wallet->SetAddressBook(dest, strprintf("legacy_%d", j), /*purpose=*/std::nullopt));
- CMutableTransaction mtx;
- mtx.vout.emplace_back(COIN, GetScriptForDestination(dest));
- 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, /*rescanning_old_block=*/true);
- batch.WriteKey(pubkey, key.GetPrivKey(), CKeyMetadata());
+ CMutableTransaction mtx;
+ mtx.vout.emplace_back(COIN, GetScriptForDestination(dest));
+ 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, /*rescanning_old_block=*/true);
+ batch.WriteKey(pubkey, key.GetPrivKey(), CKeyMetadata());
+ }
}
bench.epochs(/*numEpochs=*/1).epochIterations(/*numIters=*/1) // run the migration exactly once
Why this scored 15/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.