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

8b726bf556 test: Coin Selection, duplicated preset inputs selection (furszy)
9d73176d00 test: wallet, coverage for CoinsResult::Erase function (furszy)
195f0dfd0e wallet: bugfix, 'CoinsResult::Erase' is erasing only one output of the set (furszy)
e5d097b639 [test] Add p2p_tx_privacy.py (dergoegge)
c8426706de [net processing] Assume that TxRelay::m_tx_inventory_to_send is empty pre-verack (dergoegge)
e15b306017 [net processing] Ensure transaction announcements are only queued for fully connected peers (dergoegge)
95fded1069 wallet: Explicitly say migratewallet on encrypted wallets is unsupported (Andrew Chow)
d464b2af30 tests: Test for migrating encrypted wallets (Andrew Chow)
7a97a56ffb 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 8b726bf556

Tree-SHA512: db77ec1a63a7b6a4412750a0f4c0645681fc346a5df0a7cd38d5d27384e1d0fa95f3953af90042afe131ddbd4b6a6e009527095f13e9f58c0190cd378738a9e5

src/net_processing.cpp

@@ -2007,8 +2007,15 @@ void PeerManagerImpl::RelayTransaction(const uint256& txid, const uint256& wtxid
         auto tx_relay = peer.GetTxRelay();
         if (!tx_relay) continue;
 
-        const uint256& hash{peer.m_wtxid_relay ? wtxid : txid};
         LOCK(tx_relay->m_tx_inventory_mutex);
+        // Only queue transactions for announcement once the version handshake
+        // is completed. The time of arrival for these transactions is
+        // otherwise at risk of leaking to a spy, if the spy is able to
+        // distinguish transactions received during the handshake from the rest
+        // in the announcement.
+        if (tx_relay->m_next_inv_send_time == 0s) continue;
+
+        const uint256& hash{peer.m_wtxid_relay ? wtxid : txid};
         if (!tx_relay->m_tx_inventory_known_filter.contains(hash)) {
             tx_relay->m_tx_inventory_to_send.insert(hash);
         }
@@ -3396,6 +3403,21 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
             // they may wish to request compact blocks from us
             m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION));
         }
+
+        if (auto tx_relay = peer->GetTxRelay()) {
+            // `TxRelay::m_tx_inventory_to_send` must be empty before the
+            // version handshake is completed as
+            // `TxRelay::m_next_inv_send_time` is first initialised in
+            // `SendMessages` after the verack is received. Any transactions
+            // received during the version handshake would otherwise
+            // immediately be advertised without random delay, potentially
+            // leaking the time of arrival to a spy.
+            Assume(WITH_LOCK(
+                tx_relay->m_tx_inventory_mutex,
+                return tx_relay->m_tx_inventory_to_send.empty() &&
+                       tx_relay->m_next_inv_send_time == 0s));
+        }
+
         pfrom.fSuccessfullyConnected = true;
         return;
     }

src/wallet/rpc/wallet.cpp

@@ -730,7 +730,9 @@ static RPCHelpMan migratewallet()
             std::shared_ptr<CWallet> wallet = GetWalletForJSONRPCRequest(request);
             if (!wallet) return NullUniValue;
 
-            EnsureWalletIsUnlocked(*wallet);
+            if (wallet->IsCrypted()) {
+                throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: migratewallet on encrypted wallets is currently unsupported.");
+            }
 
             WalletContext& context = EnsureWalletContext(request.context);
 

src/wallet/spend.cpp

@@ -102,15 +102,13 @@ void CoinsResult::Clear() {
     coins.clear();
 }
 
-void CoinsResult::Erase(std::set<COutPoint>& preset_coins)
+void CoinsResult::Erase(const std::set<COutPoint>& 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) {
+            return coins_to_remove.count(coin.outpoint) == 1;
+        });
+        vec.erase(remove_it, vec.end());
     }
 }
 

src/wallet/spend.h

@@ -47,7 +47,7 @@ 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::set<COutPoint>& coins_to_remove);
     void Shuffle(FastRandomContext& rng_fast);
     void Add(OutputType type, const COutput& out);
 

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

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

src/wallet/wallet.cpp

@@ -4102,8 +4102,8 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
 
         // Make list of wallets to cleanup
         std::vector<std::shared_ptr<CWallet>> created_wallets;
-        created_wallets.push_back(std::move(res.watchonly_wallet));
-        created_wallets.push_back(std::move(res.solvables_wallet));
+        if (res.watchonly_wallet) created_wallets.push_back(std::move(res.watchonly_wallet));
+        if (res.solvables_wallet) created_wallets.push_back(std::move(res.solvables_wallet));
 
         // Get the directories to remove after unloading
         for (std::shared_ptr<CWallet>& w : created_wallets) {

test/functional/p2p_tx_privacy.py

@@ -0,0 +1,78 @@
+#!/usr/bin/env python3
+# Copyright (c) 2022 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""
+Test that transaction announcements are only queued for peers that have
+successfully completed the version handshake.
+
+Topology:
+
+  tx_originator ----> node[0] <---- spy
+
+We test that a transaction sent by tx_originator is only relayed to spy
+if it was received after spy's version handshake completed.
+
+1. Fully connect tx_originator
+2. Connect spy (no version handshake)
+3. tx_originator sends tx1
+4. spy completes the version handshake
+5. tx_originator sends tx2
+6. We check that only tx2 is announced on the spy interface
+"""
+from test_framework.messages import (
+    msg_wtxidrelay,
+    msg_verack,
+    msg_tx,
+    CInv,
+    MSG_WTX,
+)
+from test_framework.p2p import (
+    P2PInterface,
+)
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.wallet import MiniWallet
+
+class P2PTxSpy(P2PInterface):
+    def __init__(self):
+        super().__init__()
+        self.all_invs = []
+
+    def on_version(self, message):
+        self.send_message(msg_wtxidrelay())
+
+    def on_inv(self, message):
+        self.all_invs += message.inv
+
+    def wait_for_inv_match(self, expected_inv):
+        self.wait_until(lambda: len(self.all_invs) == 1 and self.all_invs[0] == expected_inv)
+
+class TxPrivacyTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.num_nodes = 1
+
+    def run_test(self):
+        self.wallet = MiniWallet(self.nodes[0])
+        self.wallet.rescan_utxos()
+
+        tx_originator = self.nodes[0].add_p2p_connection(P2PInterface())
+        spy = self.nodes[0].add_p2p_connection(P2PTxSpy(), wait_for_verack=False)
+        spy.wait_for_verack()
+
+        # tx_originator sends tx1
+        tx1 = self.wallet.create_self_transfer()["tx"]
+        tx_originator.send_and_ping(msg_tx(tx1))
+
+        # Spy sends the verack
+        spy.send_and_ping(msg_verack())
+
+        # tx_originator sends tx2
+        tx2 = self.wallet.create_self_transfer()["tx"]
+        tx_originator.send_and_ping(msg_tx(tx2))
+
+        # Spy should only get an inv for the second transaction as the first
+        # one was received pre-verack with the spy
+        spy.wait_for_inv_match(CInv(MSG_WTX, tx2.calc_sha256(True)))
+
+if __name__ == '__main__':
+    TxPrivacyTest().main()

test/functional/rpc_fundrawtransaction.py

@@ -107,6 +107,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()
@@ -1189,6 +1190,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")
 

test/functional/test_runner.py

@@ -316,6 +316,7 @@ BASE_SCRIPTS = [
     'rpc_deriveaddresses.py',
     'rpc_deriveaddresses.py --usecli',
     'p2p_ping.py',
+    'p2p_tx_privacy.py',
     'rpc_scantxoutset.py',
     'feature_txindex_compatibility.py',
     'feature_unsupported_utxo_db.py',

test/functional/wallet_migration.py

@@ -393,6 +393,15 @@ class WalletMigrationTest(BitcoinTestFramework):
 
         assert_equal(bals, wallet.getbalances())
 
+    def test_encrypted(self):
+        self.log.info("Test migration of an encrypted wallet")
+        wallet = self.create_legacy_wallet("encrypted")
+
+        wallet.encryptwallet("pass")
+
+        assert_raises_rpc_error(-15, "Error: migratewallet on encrypted wallets is currently unsupported.", wallet.migratewallet)
+        # TODO: Fix migratewallet so that we can actually migrate encrypted wallets
+
     def run_test(self):
         self.generate(self.nodes[0], 101)
 
@@ -402,6 +411,7 @@ class WalletMigrationTest(BitcoinTestFramework):
         self.test_other_watchonly()
         self.test_no_privkeys()
         self.test_pk_coinbases()
+        self.test_encrypted()
 
 if __name__ == '__main__':
     WalletMigrationTest().main()