From 7a97a56ffb22fbf8ccb143a8a7da77e8c7e77069 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 28 Nov 2022 18:27:27 -0500 Subject: [PATCH 1/9] wallet: Avoid null pointer deref when cleaning up migratewallet If migratewallet fails, we do a cleanup which removes the watchonly and solvables wallets if they were created. However, if they were not, their pointers are nullptr and we don't check for that, which causes a segfault during the cleanup. So check that they aren't nullptr before cleaning them up. Github-Pull: #26594 Rebased-From: 86ef7b3c7be84e4183098f448c77ecc9ea7367ab --- src/wallet/wallet.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ac7bf46a1..36fe32e54 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -4102,8 +4102,8 @@ util::Result MigrateLegacyToDescriptor(std::shared_ptr // Make list of wallets to cleanup std::vector> 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& w : created_wallets) { From d464b2af30f2b02be2ce0b5e45dc6c141529dba5 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 28 Nov 2022 18:30:40 -0500 Subject: [PATCH 2/9] tests: Test for migrating encrypted wallets Due to an oversight, we cannot currently migrate encrypted wallets, regardless of whether they are unlocked. Migrating such wallets will trigger an error, and result in the cleanup being run. This conveniently allows us to check some parts of the cleanup code. Github-Pull: #26594 Rebased-From: 88afc73ae0c67a4482ecd3d77eb2a8fd2673f82d --- test/functional/wallet_migration.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 3c1cb6ac3..355046c9a 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -393,6 +393,16 @@ 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") + + wallet.walletpassphrase("pass", 10) + assert_raises_rpc_error(-4, "Error: Unable to produce descriptors for this legacy wallet. Make sure the wallet is unlocked first", 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 +412,7 @@ class WalletMigrationTest(BitcoinTestFramework): self.test_other_watchonly() self.test_no_privkeys() self.test_pk_coinbases() + self.test_encrypted() if __name__ == '__main__': WalletMigrationTest().main() From 95fded106979a523431863679107810db81ca4b3 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Tue, 29 Nov 2022 16:05:51 -0500 Subject: [PATCH 3/9] wallet: Explicitly say migratewallet on encrypted wallets is unsupported Github-Pull: #26594 Rebased-From: 5e65a216d1fd00c447757736d4f2899d235e731a --- src/wallet/rpc/wallet.cpp | 4 +++- test/functional/wallet_migration.py | 3 +-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 675c4a759..971814e9c 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/src/wallet/rpc/wallet.cpp @@ -730,7 +730,9 @@ static RPCHelpMan migratewallet() std::shared_ptr 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); diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py index 355046c9a..4f060f996 100755 --- a/test/functional/wallet_migration.py +++ b/test/functional/wallet_migration.py @@ -399,8 +399,7 @@ class WalletMigrationTest(BitcoinTestFramework): wallet.encryptwallet("pass") - wallet.walletpassphrase("pass", 10) - assert_raises_rpc_error(-4, "Error: Unable to produce descriptors for this legacy wallet. Make sure the wallet is unlocked first", wallet.migratewallet) + 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): From e15b3060179f94962eff82f3ed87a1d26ef65c88 Mon Sep 17 00:00:00 2001 From: dergoegge Date: Mon, 28 Nov 2022 16:34:50 +0000 Subject: [PATCH 4/9] [net processing] Ensure transaction announcements are only queued for fully connected peers Github-Pull: #26569 Rebased-From: 845e3a34c49abcc634b5a10ccdd6b10fb4fcf449 --- src/net_processing.cpp | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index a6299be40..1b333eb0b 100644 --- a/src/net_processing.cpp +++ b/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); } From c8426706deda827231715a1e9afd2078026a5e49 Mon Sep 17 00:00:00 2001 From: dergoegge Date: Mon, 28 Nov 2022 16:37:24 +0000 Subject: [PATCH 5/9] [net processing] Assume that TxRelay::m_tx_inventory_to_send is empty pre-verack This commit documents our assumption about TxRelay::m_tx_inventory_to_send being empty prior to version handshake completion. The added Assume acts as testing oracle for our fuzzing tests to potentially detect if the assumption is violated. Github-Pull: #26569 Rebased-From: ce63fca13e9b500e9f687d80a457175ac967a371 --- src/net_processing.cpp | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 1b333eb0b..3edc05103 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3403,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; } From e5d097b639c7f75b530349b524836804cb753597 Mon Sep 17 00:00:00 2001 From: dergoegge Date: Wed, 23 Nov 2022 18:12:42 +0000 Subject: [PATCH 6/9] [test] Add p2p_tx_privacy.py Github-Pull: #26569 Rebased-From: 8f2dac54096c20afd8fd12c21a2ee5465fea085e --- test/functional/p2p_tx_privacy.py | 78 +++++++++++++++++++++++++++++++ test/functional/test_runner.py | 1 + 2 files changed, 79 insertions(+) create mode 100755 test/functional/p2p_tx_privacy.py diff --git a/test/functional/p2p_tx_privacy.py b/test/functional/p2p_tx_privacy.py new file mode 100755 index 000000000..b885ccdf5 --- /dev/null +++ b/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() diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index d78c1c634..caa4af957 100755 --- a/test/functional/test_runner.py +++ b/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', From 195f0dfd0ec7fadfbbb3d86decb3f6d96beae159 Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 22 Nov 2022 11:39:35 -0300 Subject: [PATCH 7/9] wallet: bugfix, 'CoinsResult::Erase' is erasing only one output of the set The loop break shouldn't have being there. Github-Pull: #26560 Rebased-From: f930aefff9690a1e830d897d0a8c53f4219ae4a8 --- src/wallet/spend.cpp | 14 ++++++-------- src/wallet/spend.h | 2 +- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index ce41a4e95..f534e1079 100644 --- a/src/wallet/spend.cpp +++ b/src/wallet/spend.cpp @@ -102,15 +102,13 @@ void CoinsResult::Clear() { coins.clear(); } -void CoinsResult::Erase(std::set& preset_coins) +void CoinsResult::Erase(const std::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) { + return coins_to_remove.count(coin.outpoint) == 1; + }); + vec.erase(remove_it, vec.end()); } } diff --git a/src/wallet/spend.h b/src/wallet/spend.h index c29e5be5c..009e68062 100644 --- a/src/wallet/spend.h +++ b/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& preset_coins); + void Erase(const std::set& coins_to_remove); void Shuffle(FastRandomContext& rng_fast); void Add(OutputType type, const COutput& out); From 9d73176d00a013e1383ae18cb5c0f8cbdd186cba Mon Sep 17 00:00:00 2001 From: furszy Date: Tue, 22 Nov 2022 12:58:19 -0300 Subject: [PATCH 8/9] test: wallet, coverage for CoinsResult::Erase function Github-Pull: #26560 Rebased-From: 341ba7ffd8cdb56b4cde1f251768c3d2c2a9b4e9 --- src/wallet/test/coinselector_tests.cpp | 40 ++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) diff --git a/src/wallet/test/coinselector_tests.cpp b/src/wallet/test/coinselector_tests.cpp index 23f024247..9bc6fafae 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 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::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 From 8b726bf556e05edf02946d4b1c3356df17fd0d57 Mon Sep 17 00:00:00 2001 From: furszy Date: Wed, 23 Nov 2022 10:22:50 -0300 Subject: [PATCH 9/9] test: Coin Selection, duplicated preset inputs selection This exercises the bug inside CoinsResult::Erase that ends up on (1) a wallet crash or (2) a created and broadcasted tx that contains a reduced recipient's amount. This is covered by making the wallet selects the preset inputs twice during the coin selection process. Making the wallet think that the selection process result covers the entire tx target when it does not. It's actually creating a tx that sends more coins than what inputs are covering for. Which, combined with the SFFO option, makes the wallet incorrectly reduce the recipient's amount by the difference between the original target and the wrongly counted inputs. Which means, a created and relayed tx sending less coins to the destination than what the user inputted. Github-Pull: #26560 Rebased-From: cf793846978a8783c23b66ba6b4f3f30e83ff3eb --- src/wallet/test/spend_tests.cpp | 45 +++++++++++++++++++++++ test/functional/rpc_fundrawtransaction.py | 45 +++++++++++++++++++++++ 2 files changed, 90 insertions(+) diff --git a/src/wallet/test/spend_tests.cpp b/src/wallet/test/spend_tests.cpp index a75b01487..81a8883f8 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 a963bb5e2..9a3a35609 100755 --- a/test/functional/rpc_fundrawtransaction.py +++ b/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")