Merge f0c4807a6a into merged_master

src/wallet/coinselection.cpp

@@ -740,12 +740,16 @@ void SelectionResult::AddInput(const SelectionResult& result) {
 
 void SelectionResult::Merge(const SelectionResult& other)
 {
+    // Obtain the expected selected inputs count after the merge (for now, duplicates are not allowed)
+    const size_t expected_count = m_selected_inputs.size() + other.m_selected_inputs.size();
+
     m_target += other.m_target;
     m_use_effective |= other.m_use_effective;
     if (m_algo == SelectionAlgorithm::MANUAL) {
         m_algo = other.m_algo;
     }
     util::insert(m_selected_inputs, other.m_selected_inputs);
+    assert(m_selected_inputs.size() == expected_count);
 }
 
 const std::set<COutput>& SelectionResult::GetInputSet() const

src/wallet/coinselection.h

@@ -114,6 +114,8 @@ struct COutput {
         assert(effective_value.has_value());
         return effective_value.value();
     }
+
+    bool HasEffectiveValue() const { return effective_value.has_value(); }
 };
 
 /** Parameters for one iteration of Coin Selection. */
@@ -320,6 +322,12 @@ struct SelectionResult
     void ComputeAndSetWaste(const CAmount min_viable_change, const CAmount change_cost, const CAmount change_fee);
     [[nodiscard]] CAmount GetWaste() const;
 
+    /**
+     * Combines the @param[in] other selection result into 'this' selection result.
+     *
+     * Important note:
+     * There must be no shared 'COutput' among the two selection results being combined.
+     */
     void Merge(const SelectionResult& other);
 
     /** Get m_selected_inputs */

src/wallet/spend.cpp

@@ -199,15 +199,19 @@ void CoinsResult::Clear() {
     coins.clear();
 }
 
-void CoinsResult::Erase(std::set<COutPoint>& preset_coins)
+void CoinsResult::Erase(const std::unordered_set<COutPoint, SaltedOutpointHasher>& coins_to_remove)
 {
-    for (auto& it : coins) {
-        auto& vec = it.second;
-        auto i = std::find_if(vec.begin(), vec.end(), [&](const COutput &c) { return preset_coins.count(c.outpoint);});
-        if (i != vec.end()) {
-            vec.erase(i);
-            break;
-        }
+    for (auto& [type, vec] : coins) {
+        auto remove_it = std::remove_if(vec.begin(), vec.end(), [&](const COutput& coin) {
+            // remove it if it's on the set
+            if (coins_to_remove.count(coin.outpoint) == 0) return false;
+
+            // update cached amounts
+            total_amount[coin.asset] -= coin.value;
+            if (coin.HasEffectiveValue()) total_effective_amount[coin.asset] -= coin.GetEffectiveValue();
+            return true;
+        });
+        vec.erase(remove_it, vec.end());
     }
 }
 
@@ -221,6 +225,10 @@ void CoinsResult::Shuffle(FastRandomContext& rng_fast)
 void CoinsResult::Add(OutputType type, const COutput& out)
 {
     coins[type].emplace_back(out);
+    total_amount[out.asset] += out.value;
+    if (out.HasEffectiveValue()) {
+    	   total_effective_amount[out.asset] += out.GetEffectiveValue();
+    }
 }
 
 static OutputType GetOutputType(TxoutType type, bool is_from_p2sh)
@@ -440,11 +448,9 @@ CoinsResult AvailableCoins(const CWallet& wallet,
             result.Add(GetOutputType(type, is_from_p2sh),
                        COutput(wallet, wtx, outpoint, output, nDepth, input_bytes, spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me, feerate));
 
-            // Cache total amount as we go
-            result.total_amount[asset] += outValue;
             // Checks the sum amount of all UTXO's.
             if (params.min_sum_amount != MAX_MONEY) {
-                if (result.total_amount[::policyAsset] >= params.min_sum_amount) { // ELEMENTS: only use mininum sum for policy asset
+                if (result.GetTotalAmount()[::policyAsset] >= params.min_sum_amount) { // ELEMENTS: only use mininum sum for policy asset
                     return result;
                 }
             }
@@ -468,7 +474,7 @@ CoinsResult AvailableCoinsListUnspent(const CWallet& wallet, const CCoinControl*
 CAmountMap GetAvailableBalance(const CWallet& wallet, const CCoinControl* coinControl)
 {
     LOCK(wallet.cs_wallet);
-    return AvailableCoins(wallet, coinControl).total_amount;
+    return AvailableCoins(wallet, coinControl).GetTotalAmount();
 }
 
 const CTxOut& FindNonChangeParentOutput(const CWallet& wallet, const CTransaction& tx, int output) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)
@@ -756,6 +762,11 @@ std::optional<SelectionResult> SelectCoins(const CWallet& wallet, CoinsResult& a
         return result;
     }
 
+    CAmountMap available_coins_total_amount = coin_selection_params.m_subtract_fee_outputs ? available_coins.GetTotalAmount() : available_coins.GetEffectiveTotalAmount();
+    if (selection_target > available_coins_total_amount) {
+        return std::nullopt; // Insufficient funds
+    }
+
     // Start wallet Coin Selection procedure
     auto op_selection_result = AutomaticCoinSelection(wallet, available_coins, selection_target, coin_control, coin_selection_params);
     if (!op_selection_result) return op_selection_result;
@@ -782,7 +793,7 @@ std::optional<SelectionResult> AutomaticCoinSelection(const CWallet& wallet, Coi
     const bool fRejectLongChains = gArgs.GetBoolArg("-walletrejectlongchains", DEFAULT_WALLET_REJECT_LONG_CHAINS);
 
     // ELEMENTS: filter coins for assets we are interested in; always keep policyAsset for fees
-    std::set<COutPoint> outpoints;
+    std::unordered_set<COutPoint, SaltedOutpointHasher> outpoints;
     for (const auto& output : available_coins.All()) {
         if (output.asset != ::policyAsset  && value_to_select.find(output.asset) == value_to_select.end()) {
             outpoints.emplace(output.outpoint);

src/wallet/spend.h

@@ -49,13 +49,19 @@ struct CoinsResult {
      * i.e., methods can work with individual OutputType vectors or on the entire object */
     size_t Size() const;
     void Clear();
-    void Erase(std::set<COutPoint>& preset_coins);
+    void Erase(const std::unordered_set<COutPoint, SaltedOutpointHasher>& coins_to_remove);
     void Shuffle(FastRandomContext& rng_fast);
     void Add(OutputType type, const COutput& out);
 
     /** Sum of all available coins */
     // ELEMENTS: for each asset
-    CAmountMap total_amount;
+    CAmountMap GetTotalAmount() { return total_amount; }
+    CAmountMap GetEffectiveTotalAmount() {return total_effective_amount; }
+
+private:
+    CAmountMap total_amount{{::policyAsset, 0}};
+    /** Sum of effective value (each asset output value minus fees required to spend it) */
+    CAmountMap total_effective_amount{{::policyAsset, 0}};
 };
 
 struct CoinFilterParams {

src/wallet/test/coinselector_tests.cpp

@@ -91,7 +91,7 @@ static void add_coin(CoinsResult& available_coins, CWallet& wallet, const CAmoun
     assert(ret.second);
     CWalletTx& wtx = (*ret.first).second;
     const auto& txout = wtx.tx->vout.at(nInput);
-    available_coins.coins[OutputType::BECH32].emplace_back(COutPoint(wtx.GetHash(), nInput), txout, nAge, CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx.GetTxTime(), fIsFromMe, feerate);
+    available_coins.Add(OutputType::BECH32, {COutPoint(wtx.GetHash(), nInput), txout, nAge, CalculateMaximumSignedInputSize(txout, &wallet, /*coin_control=*/nullptr), /*spendable=*/ true, /*solvable=*/ true, /*safe=*/ true, wtx.GetTxTime(), fIsFromMe, feerate});
 }
 
 /** Check if SelectionResult a is equivalent to SelectionResult b.
@@ -350,7 +350,7 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
         coin_control.Select(select_coin.outpoint);
         PreSelectedInputs selected_input;
         selected_input.Insert(select_coin, coin_selection_params_bnb.m_subtract_fee_outputs);
-        available_coins.coins[OutputType::BECH32].erase(available_coins.coins[OutputType::BECH32].begin());
+        available_coins.Erase({available_coins.coins[OutputType::BECH32].begin()->outpoint});
         coin_selection_params_bnb.m_effective_feerate = CFeeRate(0);
         CAmountMap mapTargetValue;
         mapTargetValue[CAsset()] = 10 * CENT;
@@ -414,8 +414,8 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
         coin_control.Select(select_coin.outpoint);
         PreSelectedInputs selected_input;
         selected_input.Insert(select_coin, coin_selection_params_bnb.m_subtract_fee_outputs);
-        available_coins.coins[OutputType::BECH32].erase(++available_coins.coins[OutputType::BECH32].begin());
-        const auto result13 = SelectCoins(*wallet, available_coins, selected_input, mapTargetValue, coin_control, coin_selection_params_bnb);
+        available_coins.Erase({(++available_coins.coins[OutputType::BECH32].begin())->outpoint});
+	const auto result13 = SelectCoins(*wallet, available_coins, selected_input, mapTargetValue, coin_control, coin_selection_params_bnb);
         BOOST_CHECK(EquivalentResult(expected_result, *result13));
     }
 }
@@ -989,11 +989,51 @@ BOOST_AUTO_TEST_CASE(SelectCoins_effective_value_test)
     cc.SelectExternal(output.outpoint, output.txout);
 
     const auto preset_inputs = *Assert(FetchSelectedInputs(*wallet, cc, cs_params));
-    available_coins.coins[OutputType::BECH32].erase(available_coins.coins[OutputType::BECH32].begin());
+    available_coins.Erase({available_coins.coins[OutputType::BECH32].begin()->outpoint});
 
     const auto result = SelectCoins(*wallet, available_coins, preset_inputs, target, cc, cs_params);
     BOOST_CHECK(!result);
 }
 
+BOOST_FIXTURE_TEST_CASE(wallet_coinsresult_test, BasicTestingSetup)
+{
+    // Test case to verify CoinsResult object sanity.
+    CoinsResult available_coins;
+    {
+        std::unique_ptr<CWallet> dummyWallet = std::make_unique<CWallet>(m_node.chain.get(), "dummy", m_args, CreateMockWalletDatabase());
+        BOOST_CHECK_EQUAL(dummyWallet->LoadWallet(), DBErrors::LOAD_OK);
+        LOCK(dummyWallet->cs_wallet);
+        dummyWallet->SetWalletFlag(WALLET_FLAG_DESCRIPTORS);
+        dummyWallet->SetupDescriptorScriptPubKeyMans();
+
+        // Add some coins to 'available_coins'
+        for (int i=0; i<10; i++) {
+            add_coin(available_coins, *dummyWallet, 1 * COIN);
+        }
+    }
+
+    {
+        // First test case, check that 'CoinsResult::Erase' function works as expected.
+        // By trying to erase two elements from the 'available_coins' object.
+        std::unordered_set<COutPoint, SaltedOutpointHasher> outs_to_remove;
+        const auto& coins = available_coins.All();
+        for (int i = 0; i < 2; i++) {
+            outs_to_remove.emplace(coins[i].outpoint);
+        }
+        available_coins.Erase(outs_to_remove);
+
+        // Check that the elements were actually removed.
+        const auto& updated_coins = available_coins.All();
+        for (const auto& out: outs_to_remove) {
+            auto it = std::find_if(updated_coins.begin(), updated_coins.end(), [&out](const COutput &coin) {
+                return coin.outpoint == out;
+            });
+            BOOST_CHECK(it == updated_coins.end());
+        }
+        // And verify that no extra element were removed
+        BOOST_CHECK_EQUAL(available_coins.Size(), 8);
+    }
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 } // namespace wallet

src/wallet/test/spend_tests.cpp

@@ -120,5 +120,50 @@ BOOST_FIXTURE_TEST_CASE(FillInputToWeightTest, BasicTestingSetup)
     // Note: We don't test the next boundary because of memory allocation constraints.
 }
 
+BOOST_FIXTURE_TEST_CASE(wallet_duplicated_preset_inputs_test, TestChain100Setup)
+{
+    // Verify that the wallet's Coin Selection process does not include pre-selected inputs twice in a transaction.
+
+    // Add 4 spendable UTXO, 50 BTC each, to the wallet (total balance 200 BTC)
+    for (int i = 0; i < 4; i++) CreateAndProcessBlock({}, GetScriptForRawPubKey(coinbaseKey.GetPubKey()));
+    auto wallet = CreateSyncedWallet(*m_node.chain, WITH_LOCK(Assert(m_node.chainman)->GetMutex(), return m_node.chainman->ActiveChain()), m_args, coinbaseKey);
+
+    LOCK(wallet->cs_wallet);
+    auto available_coins = AvailableCoins(*wallet);
+    std::vector<COutput> coins = available_coins.All();
+    // Preselect the first 3 UTXO (150 BTC total)
+    std::set<COutPoint> preset_inputs = {coins[0].outpoint, coins[1].outpoint, coins[2].outpoint};
+
+    // Try to create a tx that spends more than what preset inputs + wallet selected inputs are covering for.
+    // The wallet can cover up to 200 BTC, and the tx target is 299 BTC.
+    std::vector<CRecipient> recipients = {{GetScriptForDestination(*Assert(wallet->GetNewDestination(OutputType::BECH32, "dummy"))),
+                                           /*nAmount=*/ 299 * COIN, CAsset(), CPubKey(), /*fSubtractFeeFromAmount=*/true}};
+    CCoinControl coin_control;
+    coin_control.m_allow_other_inputs = true;
+    for (const auto& outpoint : preset_inputs) {
+        coin_control.Select(outpoint);
+    }
+
+    // Attempt to send 299 BTC from a wallet that only has 200 BTC. The wallet should exclude
+    // the preset inputs from the pool of available coins, realize that there is not enough
+    // money to fund the 299 BTC payment, and fail with "Insufficient funds".
+    //
+    // Even with SFFO, the wallet can only afford to send 200 BTC.
+    // If the wallet does not properly exclude preset inputs from the pool of available coins
+    // prior to coin selection, it may create a transaction that does not fund the full payment
+    // amount or, through SFFO, incorrectly reduce the recipient's amount by the difference
+    // between the original target and the wrongly counted inputs (in this case 99 BTC)
+    // 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);
+    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);
+    BOOST_CHECK(!res_tx.has_value());
+}
+
 BOOST_AUTO_TEST_SUITE_END()
 } // namespace wallet

test/functional/rpc_fundrawtransaction.py

@@ -111,6 +111,7 @@ def run_test(self):
         self.generate(self.nodes[0], 121)
 
         self.test_add_inputs_default_value()
+        self.test_preset_inputs_selection()
         self.test_weight_calculation()
         self.test_change_position()
         self.test_simple()
@@ -1285,6 +1286,50 @@ def test_add_inputs_default_value(self):
 
         self.nodes[2].unloadwallet("test_preset_inputs")
 
+    def test_preset_inputs_selection(self):
+        self.log.info('Test wallet preset inputs are not double-counted or reused in coin selection')
+
+        # Create and fund the wallet with 4 UTXO of 5 BTC each (20 BTC total)
+        self.nodes[2].createwallet("test_preset_inputs_selection")
+        wallet = self.nodes[2].get_wallet_rpc("test_preset_inputs_selection")
+        outputs = {}
+        for _ in range(4):
+            outputs[wallet.getnewaddress(address_type="bech32")] = 5
+        self.nodes[0].sendmany("", outputs)
+        self.generate(self.nodes[0], 1)
+
+        # Select the preset inputs
+        coins = wallet.listunspent()
+        preset_inputs = [coins[0], coins[1], coins[2]]
+
+        # Now let's create the tx creation options
+        options = {
+            "inputs": preset_inputs,
+            "add_inputs": True,  # automatically add coins from the wallet to fulfill the target
+            "subtract_fee_from_outputs": [0],  # deduct fee from first output
+            "add_to_wallet": False
+        }
+
+        # Attempt to send 29 BTC from a wallet that only has 20 BTC. The wallet should exclude
+        # the preset inputs from the pool of available coins, realize that there is not enough
+        # money to fund the 29 BTC payment, and fail with "Insufficient funds".
+        #
+        # Even with SFFO, the wallet can only afford to send 20 BTC.
+        # If the wallet does not properly exclude preset inputs from the pool of available coins
+        # prior to coin selection, it may create a transaction that does not fund the full payment
+        # amount or, through SFFO, incorrectly reduce the recipient's amount by the difference
+        # between the original target and the wrongly counted inputs (in this case 9 BTC)
+        # so that the recipient's amount is no longer equal to the user's selected target of 29 BTC.
+
+        # First case, use 'subtract_fee_from_outputs = true'
+        assert_raises_rpc_error(-4, "Insufficient funds", wallet.send, outputs=[{wallet.getnewaddress(address_type="bech32"): 29}], options=options)
+
+        # Second case, don't use 'subtract_fee_from_outputs'
+        del options["subtract_fee_from_outputs"]
+        assert_raises_rpc_error(-4, "Insufficient funds", wallet.send, outputs=[{wallet.getnewaddress(address_type="bech32"): 29}], options=options)
+
+        self.nodes[2].unloadwallet("test_preset_inputs_selection")
+
     def test_weight_calculation(self):
         self.log.info("Test weight calculation with external inputs")