diff --git a/src/wallet/coinselection.cpp b/src/wallet/coinselection.cpp index a8be6cd83aa..b889d31928d 100644 --- a/src/wallet/coinselection.cpp +++ b/src/wallet/coinselection.cpp @@ -452,12 +452,16 @@ void SelectionResult::AddInputs(const std::set& inputs, bool subtract_f 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& SelectionResult::GetInputSet() const diff --git a/src/wallet/coinselection.h b/src/wallet/coinselection.h index b23dd10867f..2ab4061a9b1 100644 --- a/src/wallet/coinselection.h +++ b/src/wallet/coinselection.h @@ -110,6 +110,8 @@ public: assert(effective_value.has_value()); return effective_value.value(); } + + bool HasEffectiveValue() const { return effective_value.has_value(); } }; /** Parameters for one iteration of Coin Selection. */ @@ -314,6 +316,12 @@ public: 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 */ diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index 8c0d56a1cb0..52d10b74b53 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -102,15 +102,19 @@ void CoinsResult::Clear() { coins.clear(); } -void CoinsResult::Erase(std::set& preset_coins) +void CoinsResult::Erase(const std::unordered_set& 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.txout.nValue; + if (coin.HasEffectiveValue()) total_effective_amount = *total_effective_amount - coin.GetEffectiveValue(); + return true; + }); + vec.erase(remove_it, vec.end()); } } @@ -124,6 +128,11 @@ void CoinsResult::Shuffle(FastRandomContext& rng_fast) void CoinsResult::Add(OutputType type, const COutput& out) { coins[type].emplace_back(out); + total_amount += out.txout.nValue; + if (out.HasEffectiveValue()) { + total_effective_amount = total_effective_amount.has_value() ? + *total_effective_amount + out.GetEffectiveValue() : out.GetEffectiveValue(); + } } static OutputType GetOutputType(TxoutType type, bool is_from_p2sh) @@ -321,11 +330,9 @@ CoinsResult AvailableCoins(const CWallet& wallet, result.Add(GetOutputType(type, is_from_p2sh), COutput(outpoint, output, nDepth, input_bytes, spendable, solvable, safeTx, wtx.GetTxTime(), tx_from_me, feerate)); - // Cache total amount as we go - result.total_amount += output.nValue; // Checks the sum amount of all UTXO's. if (params.min_sum_amount != MAX_MONEY) { - if (result.total_amount >= params.min_sum_amount) { + if (result.GetTotalAmount() >= params.min_sum_amount) { return result; } } @@ -349,7 +356,7 @@ CoinsResult AvailableCoinsListUnspent(const CWallet& wallet, const CCoinControl* CAmount 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) @@ -577,6 +584,14 @@ std::optional SelectCoins(const CWallet& wallet, CoinsResult& a return result; } + // Return early if we cannot cover the target with the wallet's UTXO. + // We use the total effective value if we are not subtracting fee from outputs and 'available_coins' contains the data. + CAmount available_coins_total_amount = coin_selection_params.m_subtract_fee_outputs ? available_coins.GetTotalAmount() : + (available_coins.GetEffectiveTotalAmount().has_value() ? *available_coins.GetEffectiveTotalAmount() : 0); + 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; diff --git a/src/wallet/spend.h b/src/wallet/spend.h index ba2c6638c83..2b861c23616 100644 --- a/src/wallet/spend.h +++ b/src/wallet/spend.h @@ -47,12 +47,18 @@ 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& preset_coins); + void Erase(const std::unordered_set& coins_to_remove); void Shuffle(FastRandomContext& rng_fast); void Add(OutputType type, const COutput& out); - /** Sum of all available coins */ + CAmount GetTotalAmount() { return total_amount; } + std::optional GetEffectiveTotalAmount() {return total_effective_amount; } + +private: + /** Sum of all available coins raw value */ CAmount total_amount{0}; + /** Sum of all available coins effective value (each output value minus fees required to spend it) */ + std::optional total_effective_amount{0}; }; struct CoinFilterParams { diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index f9c8c8ee9d4..c764264cd7a 100644 --- a/src/wallet/test/coinselector_tests.cpp +++ b/src/wallet/test/coinselector_tests.cpp @@ -83,7 +83,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. @@ -342,7 +342,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); const auto result10 = SelectCoins(*wallet, available_coins, selected_input, 10 * CENT, coin_control, coin_selection_params_bnb); BOOST_CHECK(result10); @@ -402,7 +402,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}); const auto result13 = SelectCoins(*wallet, available_coins, selected_input, 10 * CENT, coin_control, coin_selection_params_bnb); BOOST_CHECK(EquivalentResult(expected_result, *result13)); } @@ -974,11 +974,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 dummyWallet = std::make_unique(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 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 diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp index a75b0148701..81a8883f855 100644 --- a/src/wallet/test/spend_tests.cpp +++ b/src/wallet/test/spend_tests.cpp @@ -112,5 +112,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 coins = available_coins.All(); + // Preselect the first 3 UTXO (150 BTC total) + std::set 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 recipients = {{GetScriptForDestination(*Assert(wallet->GetNewDestination(OutputType::BECH32, "dummy"))), + /*nAmount=*/299 * COIN, /*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 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 diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py index 0f77d6ad309..6874154f35f 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/test/functional/rpc_fundrawtransaction.py @@ -110,6 +110,7 @@ class RawTransactionsTest(BitcoinTestFramework): 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() @@ -1203,6 +1204,50 @@ class RawTransactionsTest(BitcoinTestFramework): 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")