diff options
Diffstat (limited to 'src')
-rw-r--r-- | src/net_processing.cpp | 24 | ||||
-rw-r--r-- | src/wallet/rpc/wallet.cpp | 4 | ||||
-rw-r--r-- | src/wallet/spend.cpp | 14 | ||||
-rw-r--r-- | src/wallet/spend.h | 2 | ||||
-rw-r--r-- | src/wallet/test/coinselector_tests.cpp | 40 | ||||
-rw-r--r-- | src/wallet/test/spend_tests.cpp | 45 | ||||
-rw-r--r-- | src/wallet/wallet.cpp | 4 |
7 files changed, 120 insertions, 13 deletions
diff --git a/src/net_processing.cpp b/src/net_processing.cpp index a6299be403..3edc051034 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); } @@ -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; } diff --git a/src/wallet/rpc/wallet.cpp b/src/wallet/rpc/wallet.cpp index 675c4a759d..971814e9cd 100644 --- a/src/wallet/rpc/wallet.cpp +++ b/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); diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp index ce41a4e954..f534e10799 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<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()); } } diff --git a/src/wallet/spend.h b/src/wallet/spend.h index c29e5be5c7..009e680627 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<COutPoint>& preset_coins); + void Erase(const std::set<COutPoint>& coins_to_remove); void Shuffle(FastRandomContext& rng_fast); void Add(OutputType type, const COutput& out); 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 diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index ac7bf46a14..36fe32e54d 100644 --- a/src/wallet/wallet.cpp +++ b/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) { |