Merge bitcoin/bitcoin#26616: [24.x] Backports for 24.0.1

8b726bf556e05edf02946d4b1c3356df17fd0d57 test: Coin Selection, duplicated preset inputs selection (furszy)
9d73176d00a013e1383ae18cb5c0f8cbdd186cba test: wallet, coverage for CoinsResult::Erase function (furszy)
195f0dfd0ec7fadfbbb3d86decb3f6d96beae159 wallet: bugfix, 'CoinsResult::Erase' is erasing only one output of the set (furszy)
e5d097b639c7f75b530349b524836804cb753597 [test] Add p2p_tx_privacy.py (dergoegge)
c8426706deda827231715a1e9afd2078026a5e49 [net processing] Assume that TxRelay::m_tx_inventory_to_send is empty pre-verack (dergoegge)
e15b3060179f94962eff82f3ed87a1d26ef65c88 [net processing] Ensure transaction announcements are only queued for fully connected peers (dergoegge)
95fded106979a523431863679107810db81ca4b3 wallet: Explicitly say migratewallet on encrypted wallets is unsupported (Andrew Chow)
d464b2af30f2b02be2ce0b5e45dc6c141529dba5 tests: Test for migrating encrypted wallets (Andrew Chow)
7a97a56ffb22fbf8ccb143a8a7da77e8c7e77069 wallet: Avoid null pointer deref when cleaning up migratewallet (Andrew Chow)

Pull request description:

  Backports remaining changes on the 24.0.1 milestone.

  Currently backports:
  * https://github.com/bitcoin/bitcoin/pull/26594
  * https://github.com/bitcoin/bitcoin/pull/26569
  * https://github.com/bitcoin/bitcoin/pull/26560

ACKs for top commit:
  josibake:
    ACK https://github.com/bitcoin/bitcoin/pull/26616/commits/8b726bf556e05edf02946d4b1c3356df17fd0d57

Tree-SHA512: db77ec1a63a7b6a4412750a0f4c0645681fc346a5df0a7cd38d5d27384e1d0fa95f3953af90042afe131ddbd4b6a6e009527095f13e9f58c0190cd378738a9e5

2 files changed, 85 insertions, 0 deletions

diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp
index 23f024247d..9bc6fafae7 100644
--- a/src/wallet/test/coinselector_tests.cpp
+++ b/src/wallet/test/coinselector_tests.cpp
@@ -969,5 +969,45 @@ BOOST_AUTO_TEST_CASE(SelectCoins_effective_value_test)
     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::set<COutPoint> 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 a75b014870..81a8883f85 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<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, /*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