diff --git a/ci/dash/lint-tidy.sh b/ci/dash/lint-tidy.sh index 7786a58f7ef9..7010c065cfd5 100755 --- a/ci/dash/lint-tidy.sh +++ b/ci/dash/lint-tidy.sh @@ -20,8 +20,10 @@ iwyu_tool.py \ "src/init" \ "src/rpc/fees.cpp" \ "src/rpc/signmessage.cpp" \ + "src/test/fuzz/txorphan.cpp" \ "src/util/bip32.cpp" \ "src/util/bytevectorhash.cpp" \ + "src/util/check.cpp" \ "src/util/error.cpp" \ "src/util/getuniquepath.cpp" \ "src/util/hasher.cpp" \ diff --git a/src/Makefile.test.include b/src/Makefile.test.include index bed7081dd6d4..010fdfeb35bc 100644 --- a/src/Makefile.test.include +++ b/src/Makefile.test.include @@ -363,6 +363,7 @@ test_fuzz_fuzz_SOURCES = \ test/fuzz/tx_in.cpp \ test/fuzz/tx_out.cpp \ test/fuzz/tx_pool.cpp \ + test/fuzz/txorphan.cpp \ test/fuzz/utxo_snapshot.cpp \ test/fuzz/validation_load_mempool.cpp \ test/fuzz/versionbits.cpp diff --git a/src/init.cpp b/src/init.cpp index d2e165619bf1..6db4188c82cb 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -62,7 +62,6 @@ #include #include #include -#include #include #include #include diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 1d634907c5bc..b4030e1c5335 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -405,9 +405,6 @@ struct Peer { /** Total number of addresses that were processed (excludes rate-limited ones). */ std::atomic m_addr_processed{0}; - /** Set of txids to reconsider once their parent transactions have been accepted **/ - std::set m_orphan_work_set GUARDED_BY(g_cs_orphans); - /** Whether we've sent this peer a getheaders in response to an inv prior to initial-headers-sync completing */ bool m_inv_triggered_getheaders_before_sync GUARDED_BY(NetEventsInterface::g_msgproc_mutex){false}; @@ -711,8 +708,17 @@ class PeerManagerImpl final : public PeerManager */ bool MaybeDiscourageAndDisconnect(CNode& pnode, Peer& peer); - void ProcessOrphanTx(std::set& orphan_work_set) EXCLUSIVE_LOCKS_REQUIRED(cs_main, g_cs_orphans) - EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); + /** + * Reconsider orphan transactions after a parent has been accepted to the mempool. + * + * @param[in] node_id The peer whose orphan transactions we will reconsider. Generally only one + * orphan will be reconsidered on each call of this function. This set + * may be added to if accepting an orphan causes its children to be + * reconsidered. + * @return True if there are still orphans in this peer's work set. + */ + bool ProcessOrphanTx(NodeId node_id) + EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, cs_main); /** Process a single headers message from a peer. */ void ProcessHeadersMessage(CNode& pfrom, Peer& peer, const std::vector& headers, @@ -1040,14 +1046,14 @@ class PeerManagerImpl final : public PeerManager /** Storage for orphan information */ TxOrphanage m_orphanage; - void AddToCompactExtraTransactions(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans); + void AddToCompactExtraTransactions(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex); /** Orphan/conflicted/etc transactions that are kept for compact block reconstruction. * The last -blockreconstructionextratxn/DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN of * these are kept in a ring buffer */ - std::vector> vExtraTxnForCompact GUARDED_BY(g_cs_orphans); + std::vector> vExtraTxnForCompact GUARDED_BY(g_msgproc_mutex); /** Offset into vExtraTxnForCompact to insert the next tx */ - size_t vExtraTxnForCompactIt GUARDED_BY(g_cs_orphans) = 0; + size_t vExtraTxnForCompactIt GUARDED_BY(g_msgproc_mutex) = 0; }; // Keeps track of the time (in microseconds) when transactions were requested last time @@ -1674,7 +1680,7 @@ void PeerManagerImpl::FinalizeNode(const CNode& node) { for (const QueuedBlock& entry : state->vBlocksInFlight) { mapBlocksInFlight.erase(entry.pindex->GetBlockHash()); } - WITH_LOCK(g_cs_orphans, m_orphanage.EraseForPeer(nodeid)); + m_orphanage.EraseForPeer(nodeid); if (m_txreconciliation) m_txreconciliation->ForgetPeer(nodeid); m_num_preferred_download_peers -= state->fPreferredDownload; m_peers_downloading_from -= (state->nBlocksInFlight != 0); @@ -1768,7 +1774,7 @@ bool PeerManagerImpl::GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) c return true; } -void PeerManagerImpl::AddToCompactExtraTransactions(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans) +void PeerManagerImpl::AddToCompactExtraTransactions(const CTransactionRef& tx) { size_t max_extra_txn = gArgs.GetIntArg("-blockreconstructionextratxn", DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN); if (max_extra_txn <= 0) @@ -2006,13 +2012,14 @@ void PeerManagerImpl::StartScheduledTasks(CScheduler& scheduler) */ void PeerManagerImpl::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindex) { + // Candidates are sourced from a block and therefore cannot be attributed to a peer, we use -1 as the identifier + bool have_candidates{true}; { - LOCK2(::cs_main, g_cs_orphans); - - auto orphanWorkSet = m_orphanage.GetCandidatesForBlock(*pblock); - while (!orphanWorkSet.empty()) { - LogPrint(BCLog::MEMPOOL, "Trying to process %d orphans\n", orphanWorkSet.size()); - ProcessOrphanTx(orphanWorkSet); + LOCK(::cs_main); + m_orphanage.SetCandidatesByBlock(*pblock); + // Keep processing as valid orphans may enable processing of their descendants + while (have_candidates) { + have_candidates = ProcessOrphanTx(/*node_id=*/-1); } } @@ -3197,33 +3204,23 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer, return; } -/** - * Reconsider orphan transactions after a parent has been accepted to the mempool. - * - * @param[in,out] orphan_work_set The set of orphan transactions to reconsider. Generally only one - * orphan will be reconsidered on each call of this function. This set - * may be added to if accepting an orphan causes its children to be - * reconsidered. - */ -void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) +bool PeerManagerImpl::ProcessOrphanTx(NodeId node_id) { AssertLockHeld(cs_main); - AssertLockHeld(g_cs_orphans); - - while (!orphan_work_set.empty()) { - const uint256 orphanHash = *orphan_work_set.begin(); - orphan_work_set.erase(orphan_work_set.begin()); - const auto [porphanTx, from_peer] = m_orphanage.GetTx(orphanHash); - if (porphanTx == nullptr) continue; + CTransactionRef porphanTx = nullptr; + NodeId from_peer = -1; + bool more = false; + while (CTransactionRef porphanTx = m_orphanage.GetTxToReconsider(node_id, from_peer, more)) { const MempoolAcceptResult result = m_chainman.ProcessTransaction(porphanTx); const TxValidationState& state = result.m_state; + const uint256& orphanHash = porphanTx->GetHash(); if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) { LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanHash.ToString()); _RelayTransaction(porphanTx->GetHash()); - m_orphanage.AddChildrenToWorkSet(*porphanTx, orphan_work_set); + m_orphanage.AddChildrenToWorkSet(*porphanTx, node_id); m_orphanage.EraseTx(orphanHash); break; } else if (state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) { @@ -3243,6 +3240,10 @@ void PeerManagerImpl::ProcessOrphanTx(std::set& orphan_work_set) break; } } + + // We could either have more to process from the existing work set or processing this + // orphan has given us more potential work + return more || m_orphanage.HaveMoreWork(node_id); } bool PeerManagerImpl::PrepareBlockFilterRequest(CNode& node, Peer& peer, @@ -4469,7 +4470,7 @@ void PeerManagerImpl::ProcessMessage( } } - LOCK2(cs_main, g_cs_orphans); + LOCK(cs_main); if (AlreadyHave(inv)) { if (pfrom.HasPermission(NetPermissionFlags::ForceRelay)) { @@ -4499,7 +4500,7 @@ void PeerManagerImpl::ProcessMessage( } _RelayTransaction(tx.GetHash()); - m_orphanage.AddChildrenToWorkSet(tx, peer->m_orphan_work_set); + m_orphanage.AddChildrenToWorkSet(tx, peer->m_id); pfrom.m_last_tx_time = GetTime(); @@ -4509,7 +4510,7 @@ void PeerManagerImpl::ProcessMessage( m_mempool.size(), m_mempool.DynamicMemoryUsage() / 1000); // Recursively process any orphan transactions that depended on this one - ProcessOrphanTx(peer->m_orphan_work_set); + ProcessOrphanTx(peer->m_id); } else if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) { @@ -4550,10 +4551,7 @@ void PeerManagerImpl::ProcessMessage( // DoS prevention: do not allow m_orphans to grow unbounded (see CVE-2012-3789) unsigned int nMaxOrphanTxSize = (unsigned int)std::max((int64_t)0, gArgs.GetIntArg("-maxorphantxsize", DEFAULT_MAX_ORPHAN_TRANSACTIONS_SIZE)) * 1000000; - unsigned int nEvicted = m_orphanage.LimitOrphans(nMaxOrphanTxSize); - if (nEvicted > 0) { - LogPrint(BCLog::MEMPOOL, "orphanage overflow, removed %u tx\n", nEvicted); - } + m_orphanage.LimitOrphans(nMaxOrphanTxSize); } else { LogPrint(BCLog::MEMPOOL, "not keeping orphan with rejected parents %s\n",tx.GetHash().ToString()); // We will continue to reject this tx since it has rejected @@ -4651,7 +4649,7 @@ void PeerManagerImpl::ProcessMessage( bool fBlockReconstructed = false; { - LOCK2(cs_main, g_cs_orphans); + LOCK(cs_main); // If AcceptBlockHeader returned true, it set pindex assert(pindex); UpdateBlockAvailability(pfrom.GetId(), pindex->GetBlockHash()); @@ -5354,16 +5352,17 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic& interrupt } } + bool has_more_orphans; { - LOCK2(cs_main, g_cs_orphans); - if (!peer->m_orphan_work_set.empty()) { - ProcessOrphanTx(peer->m_orphan_work_set); - } + LOCK(cs_main); + has_more_orphans = ProcessOrphanTx(peer->m_id); } if (pfrom->fDisconnect) return false; + if (has_more_orphans) return true; + // this maintains the order of responses // and prevents m_getdata_requests to grow unbounded { @@ -5371,11 +5370,6 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic& interrupt if (!peer->m_getdata_requests.empty()) return true; } - { - LOCK(g_cs_orphans); - if (!peer->m_orphan_work_set.empty()) return true; - } - // Don't bother if send buffer is too full to respond anyway if (pfrom->fPauseSend) return false; diff --git a/src/rpc/net.cpp b/src/rpc/net.cpp index 69612e171dd1..ecc8eb9bf123 100644 --- a/src/rpc/net.cpp +++ b/src/rpc/net.cpp @@ -118,10 +118,10 @@ static RPCHelpMan getpeerinfo() { {RPCResult::Type::STR, "SERVICE_NAME", "the service name if it is recognised"} }}, - {RPCResult::Type::STR_HEX, "verified_proregtx_hash", true /*optional*/, "Only present when the peer is a masternode and successfully " + {RPCResult::Type::STR_HEX, "verified_proregtx_hash", "Only present when the peer is a masternode and successfully " "authenticated via MNAUTH. In this case, this field contains the " "protx hash of the masternode"}, - {RPCResult::Type::STR_HEX, "verified_pubkey_hash", true /*optional*/, "Only present when the peer is a masternode and successfully " + {RPCResult::Type::STR_HEX, "verified_pubkey_hash", "Only present when the peer is a masternode and successfully " "authenticated via MNAUTH. In this case, this field contains the " "hash of the masternode's operator public key"}, {RPCResult::Type::BOOL, "relaytxes", "Whether peer has asked us to relay transactions to it"}, @@ -143,16 +143,16 @@ static RPCHelpMan getpeerinfo() {RPCResult::Type::BOOL, "bip152_hb_from", "Whether peer selected us as (compact blocks) high-bandwidth peer"}, {RPCResult::Type::BOOL, "masternode", "Whether connection was due to masternode connection attempt"}, {RPCResult::Type::NUM, "banscore", "The ban score (DEPRECATED, returned only if config option -deprecatedrpc=banscore is passed)"}, - {RPCResult::Type::NUM, "startingheight", /*optional=*/true, "The starting height (block) of the peer"}, - {RPCResult::Type::NUM, "synced_headers", /*optional=*/true, "The last header we have in common with this peer"}, - {RPCResult::Type::NUM, "synced_blocks", /*optional=*/true, "The last block we have in common with this peer"}, - {RPCResult::Type::ARR, "inflight", /*optional=*/true, "", + {RPCResult::Type::NUM, "startingheight", "The starting height (block) of the peer"}, + {RPCResult::Type::NUM, "synced_headers", "The last header we have in common with this peer"}, + {RPCResult::Type::NUM, "synced_blocks", "The last block we have in common with this peer"}, + {RPCResult::Type::ARR, "inflight", "", { {RPCResult::Type::NUM, "n", "The heights of blocks we're currently asking from this peer"}, }}, - {RPCResult::Type::BOOL, "addr_relay_enabled", /*optional=*/true, "Whether we participate in address relay with this peer"}, - {RPCResult::Type::NUM, "addr_processed", /*optional=*/true, "The total number of addresses processed, excluding those dropped due to rate limiting"}, - {RPCResult::Type::NUM, "addr_rate_limited", /*optional=*/true, "The total number of addresses dropped due to rate limiting"}, + {RPCResult::Type::BOOL, "addr_relay_enabled", "Whether we participate in address relay with this peer"}, + {RPCResult::Type::NUM, "addr_processed", "The total number of addresses processed, excluding those dropped due to rate limiting"}, + {RPCResult::Type::NUM, "addr_rate_limited", "The total number of addresses dropped due to rate limiting"}, {RPCResult::Type::ARR, "permissions", "Any special permissions that have been granted to this peer", { {RPCResult::Type::STR, "permission_type", Join(NET_PERMISSIONS_DOC, ",\n") + ".\n"}, @@ -196,6 +196,14 @@ static RPCHelpMan getpeerinfo() UniValue obj(UniValue::VOBJ); CNodeStateStats statestats; bool fStateStats = peerman.GetNodeStateStats(stats.nodeid, statestats); + // GetNodeStateStats() requires the existence of a CNodeState and a Peer object + // to succeed for this peer. These are created at connection initialisation and + // exist for the duration of the connection - except if there is a race where the + // peer got disconnected in between the GetNodeStats() and the GetNodeStateStats() + // calls. In this case, the peer doesn't need to be reported here. + if (!fStateStats) { + continue; + } obj.pushKV("id", stats.nodeid); obj.pushKV("addr", stats.m_addr_name); if (stats.addrBind.IsValid()) { @@ -208,9 +216,10 @@ static RPCHelpMan getpeerinfo() if (stats.m_mapped_as != 0) { obj.pushKV("mapped_as", uint64_t(stats.m_mapped_as)); } - ServiceFlags services{fStateStats ? statestats.their_services : ServiceFlags::NODE_NONE}; + ServiceFlags services{statestats.their_services}; obj.pushKV("services", strprintf("%016x", services)); obj.pushKV("servicesnames", GetServicesNames(services)); + obj.pushKV("relaytxes", statestats.m_relay_txs); if (!stats.verifiedProRegTxHash.IsNull()) { obj.pushKV("verified_proregtx_hash", stats.verifiedProRegTxHash.ToString()); } @@ -231,7 +240,7 @@ static RPCHelpMan getpeerinfo() if (stats.m_min_ping_time < std::chrono::microseconds::max()) { obj.pushKV("minping", Ticks(stats.m_min_ping_time)); } - if (fStateStats && statestats.m_ping_wait > 0s) { + if (statestats.m_ping_wait > 0s) { obj.pushKV("pingwait", Ticks(statestats.m_ping_wait)); } obj.pushKV("version", stats.nVersion); @@ -243,24 +252,21 @@ static RPCHelpMan getpeerinfo() obj.pushKV("bip152_hb_to", stats.m_bip152_highbandwidth_to); obj.pushKV("bip152_hb_from", stats.m_bip152_highbandwidth_from); obj.pushKV("masternode", stats.m_masternode_connection); - if (fStateStats) { - if (IsDeprecatedRPCEnabled("banscore")) { - // TODO: banscore is deprecated in v21 for removal in v22, maybe impossible due to usages in p2p_quorum_data.py - obj.pushKV("banscore", statestats.m_misbehavior_score); - } - obj.pushKV("startingheight", statestats.m_starting_height); - obj.pushKV("synced_headers", statestats.nSyncHeight); - obj.pushKV("synced_blocks", statestats.nCommonHeight); - UniValue heights(UniValue::VARR); - for (const int height : statestats.vHeightInFlight) { - heights.push_back(height); - } - obj.pushKV("inflight", heights); - obj.pushKV("relaytxes", statestats.m_relay_txs); - obj.pushKV("addr_relay_enabled", statestats.m_addr_relay_enabled); - obj.pushKV("addr_processed", statestats.m_addr_processed); - obj.pushKV("addr_rate_limited", statestats.m_addr_rate_limited); + if (IsDeprecatedRPCEnabled("banscore")) { + // TODO: banscore is deprecated in v21 for removal in v22, maybe impossible due to usages in p2p_quorum_data.py + obj.pushKV("banscore", statestats.m_misbehavior_score); } + obj.pushKV("startingheight", statestats.m_starting_height); + obj.pushKV("synced_headers", statestats.nSyncHeight); + obj.pushKV("synced_blocks", statestats.nCommonHeight); + UniValue heights(UniValue::VARR); + for (const int height : statestats.vHeightInFlight) { + heights.push_back(height); + } + obj.pushKV("inflight", heights); + obj.pushKV("addr_relay_enabled", statestats.m_addr_relay_enabled); + obj.pushKV("addr_processed", statestats.m_addr_processed); + obj.pushKV("addr_rate_limited", statestats.m_addr_rate_limited); UniValue permissions(UniValue::VARR); for (const auto& permission : NetPermissions::ToStrings(stats.m_permission_flags)) { permissions.push_back(permission); @@ -776,6 +782,10 @@ static RPCHelpMan setban() if (request.params[3].isTrue()) absolute = true; + if (absolute && banTime < GetTime()) { + throw JSONRPCError(RPC_INVALID_PARAMETER, "Error: Absolute timestamp is in the past"); + } + if (isSubnet) { banman.Ban(subNet, banTime, absolute); if (node.connman) { @@ -999,7 +1009,7 @@ static RPCHelpMan addpeeraddress() } const std::string& addr_string{request.params[0].get_str()}; - const uint16_t port = request.params[1].getInt(); + const auto port{request.params[1].getInt()}; const bool tried{request.params[2].isTrue()}; UniValue obj(UniValue::VOBJ); diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp index 5b458ba0a541..a704adde5d40 100644 --- a/src/test/fuzz/process_message.cpp +++ b/src/test/fuzz/process_message.cpp @@ -19,7 +19,6 @@ #include #include #include -#include #include #include diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp index 8832ce2ccb46..e63e38854159 100644 --- a/src/test/fuzz/process_messages.cpp +++ b/src/test/fuzz/process_messages.cpp @@ -13,7 +13,6 @@ #include #include #include -#include #include #include diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp new file mode 100644 index 000000000000..25244771dfa9 --- /dev/null +++ b/src/test/fuzz/txorphan.cpp @@ -0,0 +1,148 @@ +// 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. + +#include +#include +#include +#include +#include +#include +#include