Merge bitcoin/bitcoin#29065: bench: wallet, fix change position out of range error

37c75c5820 test: wallet, fix change position out of range error (furszy)

Pull request description:

  Fixes #29061. Only the benchmark is affected.

  Since #25273, the behavior of 'inserting change at a random position'
  is instructed by passing ´std::nullopt´ instead of -1.

  Also, added missing documentation about the meaning of
  'change_pos=std::nullopt' inside 'CWallet::CreateTransaction()'

ACKs for top commit:
  achow101:
    ACK 37c75c5820
  kevkevinpal:
    ACK [37c75c5](37c75c5820)
  BrandonOdiwuor:
    ACK 37c75c5820

Tree-SHA512: d9a8d8533540455716a5090fcf407573cad9f0d0018a05f903f89e51620302f9b256318db6f7338b85c047f7fab372d724e916b1721d7ed302dbf3d845b08734

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);
     });
 }

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;

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);
     }

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

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());
 }
 

src/wallet/test/wallet_tests.cpp

@@ -558,7 +558,7 @@ public:
         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;
         }