Repository: bitcoin/bitcoin (language: cpp)

Issue:
Benchmark WalletCreateTxUsePresetInputsAndCoinSelection crashes due to #25273
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

The WalletCreateTxUsePresetInputsAndCoinSelection benchmark crashes on master due to the commit 758501b71391136c33b525b1a0109b990d4f463e introduced in #25273 (I used git bisect to verify).

```
valgrind ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection -min-time=10000
==34277== Memcheck, a memory error detector
==34277== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==34277== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==34277== Command: ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection -min-time=10000
==34277==
bench_bitcoin: bench/wallet_create_tx.cpp:135: WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>)::<lambda()>: Assertion `tx_res' failed.
==34277==
==34277== Process terminating with default action of signal 6 (SIGABRT)
==34277==    at 0x4DD79FC: __pthread_kill_implementation (pthread_kill.c:44)
==34277==    by 0x4DD79FC: __pthread_kill_internal (pthread_kill.c:78)
==34277==    by 0x4DD79FC: pthread_kill@@GLIBC_2.34 (pthread_kill.c:89)
==34277==    by 0x4D83475: raise (raise.c:26)
==34277==    by 0x4D697F2: abort (abort.c:79)
==34277==    by 0x4D6971A: __assert_fail_base.cold (assert.c:92)
==34277==    by 0x4D7AE95: __assert_fail (assert.c:101)
==34277==    by 0x29607B: operator() (wallet_create_tx.cpp:135)
==34277==    by 0x29607B: ankerl::nanobench::Bench& ankerl::nanobench::Bench::run<WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>)::{lambda()#3}>(WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>)::{lambda()#3}&&) [clone .isra.0] (nanobench.h:1221)
==34277==    by 0x297F81: WalletCreateTx(ankerl::nanobench::Bench&, OutputType, bool, std::optional<PreSelectInputs>) [clone .constprop.0] [clone .isra.0] (wallet_create_tx.cpp:132)
==34277==    by 0x21B74A: operator() (std_function.h:590)
==34277==    by 0x21B74A: benchmark::BenchRunner::RunAll(benchmark::Args const&) (bench.cpp:119)
==34277==    by 0x1F8B96: main (bench_bitcoin.cpp:132)
==34277==
==34277== HEAP SUMMARY:
==34277==     in use at exit: 48,159,834 bytes in 72,552 blocks
==34277==   total heap usage: 1,668,158 allocs, 1,595,606 frees, 552,252,252 bytes allocated
==34277==
==34277== LEAK SUMMARY:
==34277==    definitely lost: 0 bytes in 0 blocks
==34277==    indirectly lost: 0 bytes in 0 blocks
==34277==      possibly lost: 912 bytes in 3 blocks
==34277==    still reachable: 48,158,922 bytes in 72,549 blocks
==34277==         suppressed: 0 bytes in 0 blocks
==34277== Rerun with --leak-check=full to see details of leaked memory
==34277==
==34277== For lists of detected and suppressed errors, rerun with: -s
==34277== ERROR SUMMARY: 0 errors from 0 contexts (suppressed: 0 from 0)
Aborted
```

### Expected behaviour

No crash

### Steps to reproduce

`git checkout master && make -j$(nproc)`
`valgrind ./src/bench/bench_bitcoin -filter=WalletCreateTxUsePresetInputsAndCoinSelection -min-time=10000`

### Relevant log output

_No response_

### How did you obtain Bitcoin Core

Compiled from source

### What version of Bitcoin Core are you using?

master@758501b71391136c33b525b1a0109b990d4f463e

### Operating system and version

Ubuntu 22.04.3 LTS

### Machine specifications

_No response_


Discussion / hints:
Was this fixed in #29055?
Why was this not caught by CI?
> Why was this not caught by CI?

Looks like that is a "low priority" benchmark, meaning it's not run in the CI, which only run high?
> Was this fixed in https://github.com/bitcoin/bitcoin/pull/29055?

Unfortunately no, it still crashes on master as of a7484be65f7617d77aff92ecf6f5fb26015d27a8 

Fix patch:
diff --git a/src/bench/wallet_create_tx.cpp b/src/bench/wallet_create_tx.cpp
index c0ca8f983da66..84e02d2a26a0e 100644
--- a/src/bench/wallet_create_tx.cpp
+++ b/src/bench/wallet_create_tx.cpp
@@ -129,7 +129,7 @@ static void WalletCreateTx(benchmark::Bench& bench, const OutputType output_type
 
     bench.epochIterations(5).run([&] {
         LOCK(wallet.cs_wallet);
-        const auto& tx_res = CreateTransaction(wallet, recipients, -1, coin_control);
+        const auto& tx_res = CreateTransaction(wallet, recipients, /*change_pos=*/std::nullopt, coin_control);
         assert(tx_res);
     });
 }
diff --git a/src/wallet/feebumper.cpp b/src/wallet/feebumper.cpp
index c6ed0fe11cf2e..6a8453965be71 100644
--- a/src/wallet/feebumper.cpp
+++ b/src/wallet/feebumper.cpp
@@ -316,7 +316,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
     // We cannot source new unconfirmed inputs(bip125 rule 2)
     new_coin_control.m_min_depth = 1;
 
-    auto res = CreateTransaction(wallet, recipients, std::nullopt, new_coin_control, false);
+    auto res = CreateTransaction(wallet, recipients, /*change_pos=*/std::nullopt, new_coin_control, false);
     if (!res) {
         errors.push_back(Untranslated("Unable to create transaction.") + Untranslated(" ") + util::ErrorString(res));
         return Result::WALLET_ERROR;
diff --git a/src/wallet/rpc/spend.cpp b/src/wallet/rpc/spend.cpp
index b121c8e1a7085..5a13b5ac8ecfa 100644
--- a/src/wallet/rpc/spend.cpp
+++ b/src/wallet/rpc/spend.cpp
@@ -155,7 +155,7 @@ UniValue SendMoney(CWallet& wallet, const CCoinControl &coin_control, std::vecto
     std::shuffle(recipients.begin(), recipients.end(), FastRandomContext());
 
     // Send
-    auto res = CreateTransaction(wallet, recipients, std::nullopt, coin_control, true);
+    auto res = CreateTransaction(wallet, recipients, /*change_pos=*/std::nullopt, coin_control, true);
     if (!res) {
         throw JSONRPCError(RPC_WALLET_INSUFFICIENT_FUNDS, util::ErrorString(res).original);
     }
diff --git a/src/wallet/spend.h b/src/wallet/spend.h
index 504c078b803f8..3bd37cfd0eca4 100644
--- a/src/wallet/spend.h
+++ b/src/wallet/spend.h
@@ -216,7 +216,7 @@ struct CreatedTransactionResult
 /**
  * Create a new transaction paying the recipients with a set of coins
  * selected by SelectCoins(); Also create the change output, when needed
- * @note passing change_pos as -1 will result in setting a random position
+ * @note passing change_pos as std::nullopt will result in setting a random position
  */
 util::Result<CreatedTransactionResult> CreateTransaction(CWallet& wallet, const std::vector<CRecipient>& vecSend, std::optional<unsigned int> change_pos, const CCoinControl& coin_control, bool sign = true);
 


Test patch:
diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp
index b2d252b3f959d..3509bc116f7aa 100644
--- a/src/wallet/test/spend_tests.cpp
+++ b/src/wallet/test/spend_tests.cpp
@@ -33,7 +33,7 @@ BOOST_FIXTURE_TEST_CASE(SubtractFee, TestChain100Setup)
         coin_control.fOverrideFeeRate = true;
         // We need to use a change type with high cost of change so that the leftover amount will be dropped to fee instead of added as a change output
         coin_control.m_change_type = OutputType::LEGACY;
-        auto res = CreateTransaction(*wallet, {recipient}, std::nullopt, coin_control);
+        auto res = CreateTransaction(*wallet, {recipient}, /*change_pos=*/std::nullopt, coin_control);
         BOOST_CHECK(res);
         const auto& txr = *res;
         BOOST_CHECK_EQUAL(txr.tx->vout.size(), 1);
@@ -97,12 +97,12 @@ BOOST_FIXTURE_TEST_CASE(wallet_duplicated_preset_inputs_test, TestChain100Setup)
     // so that the recipient's amount is no longer equal to the user's selected target of 299 BTC.
 
     // First case, use 'subtract_fee_from_outputs=true'
-    util::Result<CreatedTransactionResult> res_tx = CreateTransaction(*wallet, recipients, /*change_pos*/-1, coin_control);
+    util::Result<CreatedTransactionResult> res_tx = CreateTransaction(*wallet, recipients, /*change_pos=*/std::nullopt, coin_control);
     BOOST_CHECK(!res_tx.has_value());
 
     // Second case, don't use 'subtract_fee_from_outputs'.
     recipients[0].fSubtractFeeFromAmount = false;
-    res_tx = CreateTransaction(*wallet, recipients, /*change_pos*/-1, coin_control);
+    res_tx = CreateTransaction(*wallet, recipients, /*change_pos=*/std::nullopt, coin_control);
     BOOST_CHECK(!res_tx.has_value());
 }
 
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 3d1cbe36a87d1..1c5ca1483f034 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -558,7 +558,7 @@ class ListCoinsTestingSetup : public TestChain100Setup
         CTransactionRef tx;
         CCoinControl dummy;
         {
-            auto res = CreateTransaction(*wallet, {recipient}, std::nullopt, dummy);
+            auto res = CreateTransaction(*wallet, {recipient}, /*change_pos=*/std::nullopt, dummy);
             BOOST_CHECK(res);
             tx = res->tx;
         }