Merge bitcoin/bitcoin#25118: wallet: unify “allow/block other inputs“ concept

d338712 scripted-diff: rename fAllowOtherInputs -> m_allow_other_inputs (furszy)
8dea74a refactor: use GetWalletTx in SelectCoins instead of access mapWallet (furszy)
b4e2d4d wallet: move "use-only coinControl inputs" below the selected inputs lookup (furszy)
25749f1 wallet: unify “allow/block other inputs“ concept (furszy)

Pull request description:

  Seeking to make the `CoinControl` options less confusing/redundant.
  It should have no functional changes.

  The too long to read technical description; remove `m_add_inputs`, we can use the already existent `fAllowOtherInputs` flag.

  In #16377 the `CoinControl` flag ‘m_add_inputs’ was added to tell the coin filtering and selection process two things:
  - Coin Filtering: Only use the provided inputs. Skip the Rest.
  - Coin Selection: Search the wtxs-outputs and append all the `CoinControl` internal and external selected outpoints to the selection result (skipping all the available output checks). Nothing else.

  Meanwhile, in `CoinControl` we already have a flag ‘fAllowOtherInputs’ which is already saying:
  - Coin Filtering: Only use the provided inputs. Skip the Rest.
  - Coin Selection: If false, no selection process -> append all the `CoinControl` selected outpoints to the selection result (while they passed all the `AvailableCoins` checks and are available in the 'vCoins' vector).

  ### Changes

  As can notice, the first point in the coin filtering process is duplicated in the two option flags. And the second one, is slightly different merely because it takes into account whether the coin is on the `AvailableCoins` vector or not.
  So it makes sense to merge ‘m_add_inputs’ and ‘fAllowOtherInputs’ into a single field for the coin filtering process while introduce other changes to add the missing/skipped internal and external coins into 'vCoins' vector if they were manually selected by the user.

  ——————————————————————————————————

  Just as an extra note:
  On top of this, I’m working on unifying/untangling further the coin filtering and selection processes so we have less duplicate functionality in both processes.

ACKs for top commit:
  laanwj:
    Code review ACK d338712

Tree-SHA512: 98920b80dd787cfe737dacd4c59575dfa8393c799b55f2aaef9aed2b15c61470715a88663557b49c7400938220f99af7690be01980a8684f4f71947407f21750

Author: laanwj

src/wallet/coincontrol.h

@@ -33,12 +33,11 @@ class CCoinControl
     CTxDestination destChange = CNoDestination();
     //! Override the default change type if set, ignored if destChange is set
     std::optional<OutputType> m_change_type;
-    //! If false, only selected inputs are used
-    bool m_add_inputs = true;
     //! If false, only safe inputs will be used
     bool m_include_unsafe_inputs = false;
-    //! If false, allows unselected inputs, but requires all selected inputs be used
-    bool fAllowOtherInputs = false;
+    //! If true, the selection process can add extra unselected inputs from the wallet
+    //! while requires all selected inputs be used
+    bool m_allow_other_inputs = false;
     //! Includes watch only addresses which are solvable
     bool fAllowWatchOnly = false;
     //! Override automatic min/max checks on fee, m_feerate must be set if true

src/wallet/feebumper.cpp

@@ -213,7 +213,7 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
     for (const auto& inputs : wtx.tx->vin) {
         new_coin_control.Select(COutPoint(inputs.prevout));
     }
-    new_coin_control.fAllowOtherInputs = true;
+    new_coin_control.m_allow_other_inputs = true;
 
     // We cannot source new unconfirmed inputs(bip125 rule 2)
     new_coin_control.m_min_depth = 1;

src/wallet/rpc/spend.cpp

@@ -535,8 +535,8 @@ void FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& fee_out,
             },
             true, true);
 
-        if (options.exists("add_inputs") ) {
-            coinControl.m_add_inputs = options["add_inputs"].get_bool();
+        if (options.exists("add_inputs")) {
+            coinControl.m_allow_other_inputs = options["add_inputs"].get_bool();
         }
 
         if (options.exists("changeAddress") || options.exists("change_address")) {
@@ -823,7 +823,7 @@ RPCHelpMan fundrawtransaction()
     int change_position;
     CCoinControl coin_control;
     // Automatically select (additional) coins. Can be overridden by options.add_inputs.
-    coin_control.m_add_inputs = true;
+    coin_control.m_allow_other_inputs = true;
     FundTransaction(*pwallet, tx, fee, change_position, request.params[1], coin_control, /*override_min_fee=*/true);
 
     UniValue result(UniValue::VOBJ);
@@ -1225,7 +1225,7 @@ RPCHelpMan send()
             CCoinControl coin_control;
             // Automatically select coins, unless at least one is manually selected. Can
             // be overridden by options.add_inputs.
-            coin_control.m_add_inputs = rawTx.vin.size() == 0;
+            coin_control.m_allow_other_inputs = rawTx.vin.size() == 0;
             SetOptionsInputWeights(options["inputs"], options);
             FundTransaction(*pwallet, rawTx, fee, change_position, options, coin_control, /*override_min_fee=*/false);
 
@@ -1649,7 +1649,7 @@ RPCHelpMan walletcreatefundedpsbt()
     CCoinControl coin_control;
     // Automatically select coins, unless at least one is manually selected. Can
     // be overridden by options.add_inputs.
-    coin_control.m_add_inputs = rawTx.vin.size() == 0;
+    coin_control.m_allow_other_inputs = rawTx.vin.size() == 0;
     SetOptionsInputWeights(request.params[0], options);
     FundTransaction(wallet, rawTx, fee, change_position, options, coin_control, /*override_min_fee=*/true);
 

src/wallet/spend.cpp

@@ -168,15 +168,10 @@ CoinsResult AvailableCoins(const CWallet& wallet,
             const CTxOut& output = wtx.tx->vout[i];
             const COutPoint outpoint(wtxid, i);
 
-            // Only consider selected coins if add_inputs is false
-            if (coinControl && !coinControl->m_add_inputs && !coinControl->IsSelected(outpoint)) {
-                continue;
-            }
-
             if (output.nValue < nMinimumAmount || output.nValue > nMaximumAmount)
                 continue;
 
-            if (coinControl && coinControl->HasSelected() && !coinControl->fAllowOtherInputs && !coinControl->IsSelected(outpoint))
+            if (coinControl && coinControl->HasSelected() && !coinControl->m_allow_other_inputs && !coinControl->IsSelected(outpoint))
                 continue;
 
             if (wallet.IsLockedCoin(outpoint))
@@ -438,23 +433,6 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, const std::vec
 
     OutputGroup preset_inputs(coin_selection_params);
 
-    // coin control -> return all selected outputs (we want all selected to go into the transaction for sure)
-    if (coin_control.HasSelected() && !coin_control.fAllowOtherInputs)
-    {
-        for (const COutput& out : vCoins) {
-            if (!out.spendable) continue;
-            /* Set ancestors and descendants to 0 as these don't matter for preset inputs as no actual selection is being done.
-             * positive_only is set to false because we want to include all preset inputs, even if they are dust.
-             */
-            preset_inputs.Insert(out, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ false);
-        }
-        SelectionResult result(nTargetValue, SelectionAlgorithm::MANUAL);
-        result.AddInput(preset_inputs);
-        if (result.GetSelectedValue() < nTargetValue) return std::nullopt;
-        result.ComputeAndSetWaste(coin_selection_params.m_cost_of_change);
-        return result;
-    }
-
     // calculate value from preset inputs and store them
     std::set<COutPoint> preset_coins;
 
@@ -463,15 +441,14 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, const std::vec
     for (const COutPoint& outpoint : vPresetInputs) {
         int input_bytes = -1;
         CTxOut txout;
-        std::map<uint256, CWalletTx>::const_iterator it = wallet.mapWallet.find(outpoint.hash);
-        if (it != wallet.mapWallet.end()) {
-            const CWalletTx& wtx = it->second;
+        auto ptr_wtx = wallet.GetWalletTx(outpoint.hash);
+        if (ptr_wtx) {
             // Clearly invalid input, fail
-            if (wtx.tx->vout.size() <= outpoint.n) {
+            if (ptr_wtx->tx->vout.size() <= outpoint.n) {
                 return std::nullopt;
             }
-            input_bytes = GetTxSpendSize(wallet, wtx, outpoint.n, false);
-            txout = wtx.tx->vout.at(outpoint.n);
+            input_bytes = GetTxSpendSize(wallet, *ptr_wtx, outpoint.n, false);
+            txout = ptr_wtx->tx->vout.at(outpoint.n);
         } else {
             // The input is external. We did not find the tx in mapWallet.
             if (!coin_control.GetExternalOutput(outpoint, txout)) {
@@ -502,6 +479,15 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, const std::vec
         preset_inputs.Insert(output, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ false);
     }
 
+    // coin control -> return all selected outputs (we want all selected to go into the transaction for sure)
+    if (coin_control.HasSelected() && !coin_control.m_allow_other_inputs) {
+        SelectionResult result(nTargetValue, SelectionAlgorithm::MANUAL);
+        result.AddInput(preset_inputs);
+        if (result.GetSelectedValue() < nTargetValue) return std::nullopt;
+        result.ComputeAndSetWaste(coin_selection_params.m_cost_of_change);
+        return result;
+    }
+
     // remove preset inputs from vCoins so that Coin Selection doesn't pick them.
     for (std::vector<COutput>::iterator it = vCoins.begin(); it != vCoins.end() && coin_control.HasSelected();)
     {
@@ -1033,8 +1019,6 @@ bool FundTransaction(CWallet& wallet, CMutableTransaction& tx, CAmount& nFeeRet,
         vecSend.push_back(recipient);
     }
 
-    coinControl.fAllowOtherInputs = true;
-
     // Acquire the locks to prevent races to the new locked unspents between the
     // CreateTransaction call and LockCoin calls (when lockUnspents is true).
     LOCK(wallet.cs_wallet);

src/wallet/test/coinselector_tests.cpp

@@ -336,7 +336,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
         add_coin(coins, *wallet, 3 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true);
         add_coin(coins, *wallet, 2 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true);
         CCoinControl coin_control;
-        coin_control.fAllowOtherInputs = true;
+        coin_control.m_allow_other_inputs = true;
         coin_control.Select(coins.at(0).outpoint);
         coin_selection_params_bnb.m_effective_feerate = CFeeRate(0);
         const auto result10 = SelectCoins(*wallet, coins, 10 * CENT, coin_control, coin_selection_params_bnb);
@@ -392,7 +392,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
         expected_result.Clear();
         add_coin(9 * CENT, 2, expected_result);
         add_coin(1 * CENT, 2, expected_result);
-        coin_control.fAllowOtherInputs = true;
+        coin_control.m_allow_other_inputs = true;
         coin_control.Select(coins.at(1).outpoint); // pre select 9 coin
         const auto result13 = SelectCoins(*wallet, coins, 10 * CENT, coin_control, coin_selection_params_bnb);
         BOOST_CHECK(EquivalentResult(expected_result, *result13));