bench: enable wallet creation benchmarks on all platforms

[furszy]

Simple fix for #29816.

Since the wallet is appended to the global WalletContext during
creation, merely calling reset() on the benchmark shared_pointer
is insufficient to destruct the wallet. This no destruction of the
wallet object results in keeping the db connection open, which
was causes the fs::remove_all() failure on Windows.

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	maflcko, hebasto, kevkevinpal

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

[fanquake]

The native Windows CI (https://github.com/bitcoin/bitcoin/actions/runs/9112695910/job/25052498545?pr=30122#step:24:64) is failing with:

Run src\bench_bitcoin.exe -sanity-check
Running with -sanity-check option, output is being suppressed as benchmark results will be useless.
Error: Process completed with exit code 1.

[fanquake]

It'd be good if the commit message could explain why this is being done. It seems like the current approach is just to try and refactor away the Windows bug, even though it might not have been properly identified (prior to #29816 (comment)). However now that the bug might have been found, it'd be good to at least document what it is, and if possible, fix/workaround it in a more targetted way, rather than refactor code that is currently perfectly fine for all other platforms.

[furszy]

It'd be good if the commit message could explain why this is being done. It seems like the current approach is just to try and refactor away the Windows bug, even though it might not have been properly identified (prior to #29816 (comment)). However now that the bug might have been found, it'd be good to at least document what it is, and if possible, fix/workaround it in a more targetted way, rather than refactor code that is currently perfectly fine for all other platforms.

Sure with a small nuance. This code is working fine for all other platforms but that does not mean the code is perfect.
When I wrote this benchmark, I was probably more concerned about the number of files the benchmark could open in parallel than the delays/inaccuracies introduced by the, OS dependent, directory removal line. Essentially, we shouldn't be removing a directory within the benchmarked code scope.
But yeah, np on splitting this in two well described commits.

[hebasto]

Concept ACK.

[hebasto]

ACK 7323bf6.

Maybe deduplicate code:

--- a/src/bench/wallet_create.cpp
+++ b/src/bench/wallet_create.cpp
@@ -35,8 +35,8 @@ static void WalletCreate(benchmark::Bench& bench, bool encrypted)
     std::vector<bilingual_str> warnings;
 
     int round = 0;
-    fs::path wallet_path = test_setup->m_path_root / strprintf("test_wallet%d_%d", round, random.rand32()).c_str();
     bench.run([&] {
+        auto wallet_path = test_setup->m_path_root / strprintf("test_wallet%d_%d", round++, random.rand32()).c_str();
         auto wallet = CreateWallet(context, wallet_path.utf8string(), /*load_on_start=*/std::nullopt, options, status, error_string, warnings);
         assert(status == DatabaseStatus::SUCCESS);
         assert(wallet != nullptr);
@@ -44,7 +44,6 @@ static void WalletCreate(benchmark::Bench& bench, bool encrypted)
         // Unload wallet and update wallet directory for the next round
         RemoveWallet(context, wallet, /*load_on_start=*/ std::nullopt);
         UnloadWallet(std::move(wallet));
-        wallet_path = test_setup->m_path_root / strprintf("test_wallet%d_%d", ++round, random.rand32()).c_str();
     });
 }
 

?

[furszy]

Applied, thanks @hebasto 👍🏼.

[hebasto]

re-ACK b6b2d82.

[maflcko]

utACK 7e30fda

[maflcko]

utACK 7c8abf3

[hebasto]

re-ACK 7c8abf3, I agree with changes since my recent review.

[kevkevinpal]

utACK 7c8abf3