wallet: remove fetch pre-selected-inputs responsibility from SelectCoins

furszy · furszy · commit 84b4ffe85ba0 · 2022-07-29T11:15:19.000-03:00
In the transaction creation process, in order to select which coins the new transaction will spend,
we first obtain all the available coins known by the wallet, which means walking-through the
wallet txes map, gathering the ones that fulfill certain spendability requirements in a vector.

This coins vector is then provided to the Coin Selection process, which first checks if the user
has manually selected any input (which could be internal, aka known by the wallet, or external),
and if it does, it fetches them by searching each of them inside the wallet and/or inside the
Coin Control external tx data.
Then, after finding the pre-selected-inputs and gathering them in a vector, the Coin Selection
process walks-through the entire available coins vector once more just to erase coins that are
in both vectors. So the Coin Selection process doesn’t pick them twice (duplicate inputs inside
the same transaction).

Now, a new method, `FetchCoins` will be responsible for:
1) Lookup the user pre-selected-inputs (which can be internal or external).
2) And, fetch the available coins in the wallet (excluding the already fetched ones).

This new flow allows us to never include the pre-selected-inputs inside the available
coins vector in the first place. So, Coin Selection can perform its main responsibility
without mixing it with having to fetch internal/external coins nor any slow and unneeded
duplicate inputs verification

Improvements:

1) If any pre-selected-input lookup fail, the process will return the error right away.
   (Before, the wallet was fetching all the wallet available coins, walking through the
   entire txes map, and then failing for an invalid pre-selected-input inside SelectCoins)

2) The pre-selected-inputs lookup failure causes are properly described now on the return error.
   (before, we were just returning an "Insufficient Funds" error even if the failure was due
    a not solvable external input)

3) Faster Coin Selection, no longer need to "remove the pre-set inputs from the available coins
   vector so that Coin Selection doesn't pick them" (which meant to loop-over the entire
   available coins vector at coin selection time, erasing coins that were pre-selected).

   Now, the available coins vector, which is built after the pre-selected-inputs fetching,
   doesn't include the already selected inputs in the first place.

4) Faster transaction creation for transactions that only use manually selected inputs.
   We now will return early, as soon as we finish fetching the pre-selected-inputs and
   not perform the resources expensive calculation of walking-through the entire wallet
   txes map to obtain the available coins (coins that we will not use).
diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
@@ -153,6 +153,31 @@ BResult<CoinsResult> FetchSelectedInputs(const CWallet& wallet, const CCoinContr
     return result;
 }
 
+BResult<CoinsFund> FetchCoins(const CWallet& wallet, const CCoinControl& coin_control,
+                              const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
+{
+    CoinsFund result;
+
+    // Fetch manually select coins
+    if (coin_control.HasSelected()) {
+        result.pre_selected_inputs = FetchSelectedInputs(wallet, coin_control, coin_selection_params);
+        if (!result.pre_selected_inputs) return result.pre_selected_inputs.GetError();
+
+        // If no other inputs are allowed, return early
+        if (!coin_control.m_allow_other_inputs) return result;
+    }
+
+    // Fetch wallet available coins
+    result.available_coins = AvailableCoins(wallet,
+                                            &coin_control,
+                                            coin_selection_params.m_effective_feerate,
+                                            1,            /*nMinimumAmount*/
+                                            MAX_MONEY,    /*nMaximumAmount*/
+                                            MAX_MONEY,    /*nMinimumSumAmount*/
+                                            0);           /*nMaximumCount*/
+    return result;
+}
+
 CoinsResult AvailableCoins(const CWallet& wallet,
                            const CCoinControl* coinControl,
                            std::optional<CFeeRate> feerate,
@@ -240,7 +265,9 @@ CoinsResult AvailableCoins(const CWallet& wallet,
             if (output.nValue < nMinimumAmount || output.nValue > nMaximumAmount)
                 continue;
 
-            if (coinControl && coinControl->HasSelected() && !coinControl->m_allow_other_inputs && !coinControl->IsSelected(outpoint))
+            // Skip manually selected coins (the caller can fetch them directly)
+            // If available and manually selected coins are required, use `FetchCoins`
+            if (coinControl && coinControl->HasSelected() && coinControl->IsSelected(outpoint))
                 continue;
 
             if (wallet.IsLockedCoin(outpoint))
@@ -575,24 +602,17 @@ std::optional<SelectionResult> ChooseSelectionResult(const CWallet& wallet, cons
     return best_result;
 }
 
-std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& available_coins, const CAmount& nTargetValue, const CCoinControl& coin_control, const CoinSelectionParams& coin_selection_params)
+std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsFund& coins, const CAmount& nTargetValue, const CCoinControl& coin_control, const CoinSelectionParams& coin_selection_params)
 {
     CAmount value_to_select = nTargetValue;
 
+    // Add the manually selected coins first
     OutputGroup preset_inputs(coin_selection_params);
-
-    // calculate value from preset inputs and store them
-    std::set<COutPoint> preset_coins;
-
-    const BResult<CoinsResult>& pre_selected_inputs = FetchSelectedInputs(wallet, coin_control, coin_selection_params);
-    if (!pre_selected_inputs) return std::nullopt;
-    else {
-        const auto& inputs = pre_selected_inputs.GetObj();
+    if (coins.pre_selected_inputs) {
+        const auto& inputs = coins.pre_selected_inputs.GetObj();
         for (const auto& coin : inputs.all()) {
-            preset_coins.insert(coin.outpoint);
-            /* Set ancestors and descendants to 0 as they don't matter for preset inputs since 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.
-            */
+            // Set ancestors and descendants to 0 as they don't matter for preset inputs since 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(coin, /*ancestors=*/ 0, /*descendants=*/ 0, /*positive_only=*/ false);
 
             // Decrease the target amount
@@ -613,21 +633,13 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& a
         return result;
     }
 
-    // remove preset inputs from coins so that Coin Selection doesn't pick them.
-    if (coin_control.HasSelected()) {
-        available_coins.legacy.erase(remove_if(available_coins.legacy.begin(), available_coins.legacy.end(), [&](const COutput& c) { return preset_coins.count(c.outpoint); }), available_coins.legacy.end());
-        available_coins.P2SH_segwit.erase(remove_if(available_coins.P2SH_segwit.begin(), available_coins.P2SH_segwit.end(), [&](const COutput& c) { return preset_coins.count(c.outpoint); }), available_coins.P2SH_segwit.end());
-        available_coins.bech32.erase(remove_if(available_coins.bech32.begin(), available_coins.bech32.end(), [&](const COutput& c) { return preset_coins.count(c.outpoint); }), available_coins.bech32.end());
-        available_coins.bech32m.erase(remove_if(available_coins.bech32m.begin(), available_coins.bech32m.end(), [&](const COutput& c) { return preset_coins.count(c.outpoint); }), available_coins.bech32m.end());
-        available_coins.other.erase(remove_if(available_coins.other.begin(), available_coins.other.end(), [&](const COutput& c) { return preset_coins.count(c.outpoint); }), available_coins.other.end());
-    }
-
     unsigned int limit_ancestor_count = 0;
     unsigned int limit_descendant_count = 0;
     wallet.chain().getPackageLimits(limit_ancestor_count, limit_descendant_count);
     const size_t max_ancestors = (size_t)std::max<int64_t>(1, limit_ancestor_count);
     const size_t max_descendants = (size_t)std::max<int64_t>(1, limit_descendant_count);
     const bool fRejectLongChains = gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS);
+    CoinsResult& available_coins = coins.available_coins;
 
     // form groups from remaining coins; note that preset coins will not
     // automatically have their associated (same address) coins included
@@ -908,17 +920,13 @@ static BResult<CreatedTransactionResult> CreateTransactionInternal(
     const CAmount not_input_fees = coin_selection_params.m_effective_feerate.GetFee(coin_selection_params.tx_noinputs_size);
     CAmount selection_target = recipients_sum + not_input_fees;
 
-    // Get available coins
-    auto available_coins = AvailableCoins(wallet,
-                                              &coin_control,
-                                              coin_selection_params.m_effective_feerate,
-                                              1,            /*nMinimumAmount*/
-                                              MAX_MONEY,    /*nMaximumAmount*/
-                                              MAX_MONEY,    /*nMinimumSumAmount*/
-                                              0);           /*nMaximumCount*/
+    // Fetch manually selected inputs and wallet available coins
+    BResult<CoinsFund> res_coins = FetchCoins(wallet, coin_control, coin_selection_params);
+    if (!res_coins) return res_coins.GetError();
+    CoinsFund coins = res_coins.ReleaseObj();
 
     // Choose coins to use
-    std::optional<SelectionResult> result = SelectCoins(wallet, available_coins, /*nTargetValue=*/selection_target, coin_control, coin_selection_params);
+    std::optional<SelectionResult> result = SelectCoins(wallet, coins, /*nTargetValue=*/selection_target, coin_control, coin_selection_params);
     if (!result) {
         return _("Insufficient funds");
     }
diff --git a/src/wallet/spend.h b/src/wallet/spend.h
@@ -125,18 +125,35 @@ std::optional<SelectionResult> AttemptSelection(const CWallet& wallet, const CAm
 std::optional<SelectionResult> ChooseSelectionResult(const CWallet& wallet, const CAmount& nTargetValue, const CoinEligibilityFilter& eligibility_filter, const std::vector<COutput>& available_coins,
                         const CoinSelectionParams& coin_selection_params);
 
+/**
+ * Struct containing the wallet's available coins vector ('AvailableCoins' result)
+ * and the user manually selected inputs whose can be internal (from the wallet)
+ * or external.
+ */
+struct CoinsFund
+{
+    // Coins manually selected by the user
+    // Empty if no coin was selected
+    BResult<CoinsResult> pre_selected_inputs;
+
+    // Available coins in the wallet
+    // (excluding the manually selected coins)
+    CoinsResult available_coins;
+};
+
 /**
  * Select a set of coins such that nTargetValue is met and at least
  * all coins from coin_control are selected; never select unconfirmed coins if they are not ours
  * param@[in]   wallet                 The wallet which provides data necessary to spend the selected coins
- * param@[in]   available_coins        The struct of coins, organized by OutputType, available for selection prior to filtering
+ * param@[in]   coins                  The struct of available coins (organized by OutputType) and the manually selected
+ *                                     inputs who must be part of the result.
  * param@[in]   nTargetValue           The target value
  * param@[in]   coin_selection_params  Parameters for this coin selection such as feerates, whether to avoid partial spends,
  *                                     and whether to subtract the fee from the outputs.
  * returns                             If successful, a SelectionResult containing the selected coins
  *                                     If failed, a nullopt.
  */
-std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& available_coins, const CAmount& nTargetValue, const CCoinControl& coin_control,
+std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsFund& coins, const CAmount& nTargetValue, const CCoinControl& coin_control,
                  const CoinSelectionParams& coin_selection_params) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
 
 struct CreatedTransactionResult
diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp
@@ -330,16 +330,21 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
         wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
         wallet->SetupDescriptorScriptPubKeyMans();
 
-        CoinsResult available_coins;
-
+        CoinsFund coins_fund;
+        CoinsResult& available_coins = coins_fund.available_coins;
         add_coin(available_coins, *wallet, 5 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true);
         add_coin(available_coins, *wallet, 3 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true);
         add_coin(available_coins, *wallet, 2 * CENT, coin_selection_params_bnb.m_effective_feerate, 6 * 24, false, 0, true);
+
+        // Mix manually selected input with available coins
         CCoinControl coin_control;
         coin_control.m_allow_other_inputs = true;
-        coin_control.Select(available_coins.all().at(0).outpoint);
+        CoinsResult pre_selected_inputs;
+        pre_selected_inputs.bech32.emplace_back(available_coins.bech32.at(0));
+        coins_fund.pre_selected_inputs = pre_selected_inputs;
+        available_coins.bech32.erase(available_coins.bech32.begin()); // not include manually selected coin in available vector
         coin_selection_params_bnb.m_effective_feerate = CFeeRate(0);
-        const auto result10 = SelectCoins(*wallet, available_coins, 10 * CENT, coin_control, coin_selection_params_bnb);
+        const auto result10 = SelectCoins(*wallet, coins_fund, 10 * CENT, coin_control, coin_selection_params_bnb);
         BOOST_CHECK(result10);
     }
     {
@@ -349,7 +354,8 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
         wallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
         wallet->SetupDescriptorScriptPubKeyMans();
 
-        CoinsResult available_coins;
+        CoinsFund coins_fund;
+        CoinsResult& available_coins = coins_fund.available_coins;
 
         // single coin should be selected when effective fee > long term fee
         coin_selection_params_bnb.m_effective_feerate = CFeeRate(5000);
@@ -362,7 +368,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
         expected_result.Clear();
         add_coin(10 * CENT, 2, expected_result);
         CCoinControl coin_control;
-        const auto result11 = SelectCoins(*wallet, available_coins, 10 * CENT, coin_control, coin_selection_params_bnb);
+        const auto result11 = SelectCoins(*wallet, coins_fund, 10 * CENT, coin_control, coin_selection_params_bnb);
         BOOST_CHECK(EquivalentResult(expected_result, *result11));
         available_coins.clear();
 
@@ -377,7 +383,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);
-        const auto result12 = SelectCoins(*wallet, available_coins, 10 * CENT, coin_control, coin_selection_params_bnb);
+        const auto result12 = SelectCoins(*wallet, coins_fund, 10 * CENT, coin_control, coin_selection_params_bnb);
         BOOST_CHECK(EquivalentResult(expected_result, *result12));
         available_coins.clear();
 
@@ -393,8 +399,12 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
         add_coin(9 * CENT, 2, expected_result);
         add_coin(1 * CENT, 2, expected_result);
         coin_control.m_allow_other_inputs = true;
-        coin_control.Select(available_coins.all().at(1).outpoint); // pre select 9 coin
-        const auto result13 = SelectCoins(*wallet, available_coins, 10 * CENT, coin_control, coin_selection_params_bnb);
+        CoinsResult pre_selected_inputs;
+        pre_selected_inputs.bech32.emplace_back(available_coins.bech32.at(1)); // pre select 9 coin
+        coins_fund.pre_selected_inputs = pre_selected_inputs;
+        available_coins.bech32.erase(std::next(available_coins.bech32.begin())); // erase pre-selected coin from the available coins vector
+
+        const auto result13 = SelectCoins(*wallet, coins_fund, 10 * CENT, coin_control, coin_selection_params_bnb);
         BOOST_CHECK(EquivalentResult(expected_result, *result13));
     }
 }
@@ -750,14 +760,14 @@ BOOST_AUTO_TEST_CASE(SelectCoins_test)
     // Run this test 100 times
     for (int i = 0; i < 100; ++i)
     {
-        CoinsResult available_coins;
+        CoinsFund coins;
         CAmount balance{0};
 
         // Make a wallet with 1000 exponentially distributed random inputs
         for (int j = 0; j < 1000; ++j)
         {
             CAmount val = distribution(generator)*10000000;
-            add_coin(available_coins, *wallet, val);
+            add_coin(coins.available_coins, *wallet, val);
             balance += val;
         }
 
@@ -780,7 +790,7 @@ BOOST_AUTO_TEST_CASE(SelectCoins_test)
             /*avoid_partial=*/ false,
         };
         CCoinControl cc;
-        const auto result = SelectCoins(*wallet, available_coins, target, cc, cs_params);
+        const auto result = SelectCoins(*wallet, coins, target, cc, cs_params);
         BOOST_CHECK(result);
         BOOST_CHECK_GE(result->GetSelectedValue(), target);
     }
diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py
@@ -405,10 +405,12 @@ def test_two_vin_two_vout(self):
 
     def test_invalid_input(self):
         self.log.info("Test fundrawtxn with an invalid vin")
-        inputs  = [ {'txid' : "1c7f966dab21119bac53213a2bc7532bff1fa844c124fd750a7d0b1332440bd1", 'vout' : 0} ] #invalid vin!
+        txid = "1c7f966dab21119bac53213a2bc7532bff1fa844c124fd750a7d0b1332440bd1"
+        vout = 0
+        inputs  = [ {'txid' : txid, 'vout' : vout} ] #invalid vin!
         outputs = { self.nodes[0].getnewaddress() : 1.0}
         rawtx   = self.nodes[2].createrawtransaction(inputs, outputs)
-        assert_raises_rpc_error(-4, "Insufficient funds", self.nodes[2].fundrawtransaction, rawtx)
+        assert_raises_rpc_error(-4, "Not found pre-selected input COutPoint(%s, %i)" % (txid[0:10], vout), self.nodes[2].fundrawtransaction, rawtx)
 
     def test_fee_p2pkh(self):
         """Compare fee of a standard pubkeyhash transaction."""
@@ -1010,7 +1012,7 @@ def test_external_inputs(self):
 
         # An external input without solving data should result in an error
         raw_tx = wallet.createrawtransaction([ext_utxo], {self.nodes[0].getnewaddress(): ext_utxo["amount"] / 2})
-        assert_raises_rpc_error(-4, "Insufficient funds", wallet.fundrawtransaction, raw_tx)
+        assert_raises_rpc_error(-4, "Not solvable pre-selected input COutPoint(%s, %s)" % (ext_utxo["txid"][0:10], ext_utxo["vout"]), wallet.fundrawtransaction, raw_tx)
 
         # Error conditions
         assert_raises_rpc_error(-5, "'not a pubkey' is not hex", wallet.fundrawtransaction, raw_tx, {"solving_data": {"pubkeys":["not a pubkey"]}})
diff --git a/test/functional/rpc_psbt.py b/test/functional/rpc_psbt.py
@@ -650,7 +650,7 @@ def test_psbt_input_keys(psbt_input, keys):
         ext_utxo = self.nodes[0].listunspent(addresses=[addr])[0]
 
         # An external input without solving data should result in an error
-        assert_raises_rpc_error(-4, "Insufficient funds", wallet.walletcreatefundedpsbt, [ext_utxo], {self.nodes[0].getnewaddress(): 15})
+        assert_raises_rpc_error(-4, "Not solvable pre-selected input COutPoint(%s, %s)" % (ext_utxo["txid"][0:10], ext_utxo["vout"]), wallet.walletcreatefundedpsbt, [ext_utxo], {self.nodes[0].getnewaddress(): 15})
 
         # But funding should work when the solving data is provided
         psbt = wallet.walletcreatefundedpsbt([ext_utxo], {self.nodes[0].getnewaddress(): 15}, 0, {"add_inputs": True, "solving_data": {"pubkeys": [addr_info['pubkey']], "scripts": [addr_info["embedded"]["scriptPubKey"], addr_info["embedded"]["embedded"]["scriptPubKey"]]}})
diff --git a/test/functional/wallet_send.py b/test/functional/wallet_send.py
@@ -508,7 +508,7 @@ def run_test(self):
         ext_utxo = ext_fund.listunspent(addresses=[addr])[0]
 
         # An external input without solving data should result in an error
-        self.test_send(from_wallet=ext_wallet, to_wallet=self.nodes[0], amount=15, inputs=[ext_utxo], add_inputs=True, psbt=True, include_watching=True, expect_error=(-4, "Insufficient funds"))
+        self.test_send(from_wallet=ext_wallet, to_wallet=self.nodes[0], amount=15, inputs=[ext_utxo], add_inputs=True, psbt=True, include_watching=True, expect_error=(-4, "Not solvable pre-selected input COutPoint(%s, %s)" % (ext_utxo["txid"][0:10], ext_utxo["vout"])))
 
         # But funding should work when the solving data is provided
         res = self.test_send(from_wallet=ext_wallet, to_wallet=self.nodes[0], amount=15, inputs=[ext_utxo], add_inputs=True, psbt=True, include_watching=True, solving_data={"pubkeys": [addr_info['pubkey']], "scripts": [addr_info["embedded"]["scriptPubKey"], addr_info["embedded"]["embedded"]["scriptPubKey"]]})
