diff --git a/doc/release-notes-31829.md b/doc/release-notes-31829.md new file mode 100644 index 00000000000..9c20a909785 --- /dev/null +++ b/doc/release-notes-31829.md @@ -0,0 +1,11 @@ +P2P + +- The transaction orphanage, which holds transactions with missing inputs temporarily while the node attempts to fetch +its parents, now has improved Denial of Service protections. Previously, it enforced a maximum number of unique +transactions (default 100, configurable using `-maxorphantx`). Now, its limits are as follows: the number of entries +(unique by wtxid and peer), plus each unique transaction's input count divided by 10, must not exceed 3,000. The total +weight of unique transactions must not exceed 404,000 Wu multiplied by the number of peers. + +- The `-maxorphantx` option no longer has any effect, since the orphanage no longer limits the number of unique +transactions. Users should remove this configuration option if they were using it, as the setting will cause an +error in future versions when it is no longer recognized. diff --git a/src/bench/txorphanage.cpp b/src/bench/txorphanage.cpp index b9bb7b41c22..dd614fa3f9d 100644 --- a/src/bench/txorphanage.cpp +++ b/src/bench/txorphanage.cpp @@ -68,7 +68,7 @@ static void OrphanageSinglePeerEviction(benchmark::Bench& bench) auto large_tx = MakeTransactionBulkedTo(1, MAX_STANDARD_TX_WEIGHT, det_rand); assert(GetTransactionWeight(*large_tx) <= MAX_STANDARD_TX_WEIGHT); - const auto orphanage{node::MakeTxOrphanage(/*max_global_ann=*/node::DEFAULT_MAX_ORPHANAGE_LATENCY_SCORE, /*reserved_peer_usage=*/node::DEFAULT_RESERVED_ORPHAN_WEIGHT_PER_PEER)}; + const auto orphanage{node::MakeTxOrphanage(/*max_global_latency_score=*/node::DEFAULT_MAX_ORPHANAGE_LATENCY_SCORE, /*reserved_peer_usage=*/node::DEFAULT_RESERVED_ORPHAN_WEIGHT_PER_PEER)}; // Populate the orphanage. To maximize the number of evictions, first fill up with tiny transactions, then add a huge one. NodeId peer{0}; @@ -97,7 +97,6 @@ static void OrphanageSinglePeerEviction(benchmark::Bench& bench) // Lastly, add the large transaction. const auto num_announcements_before_trim{orphanage->CountAnnouncements()}; assert(orphanage->AddTx(large_tx, peer)); - orphanage->LimitOrphans(); // If there are multiple peers, note that they all have the same DoS score. We will evict only 1 item at a time for each new DoSiest peer. const auto num_announcements_after_trim{orphanage->CountAnnouncements()}; @@ -132,7 +131,7 @@ static void OrphanageMultiPeerEviction(benchmark::Bench& bench) indexes.resize(NUM_UNIQUE_TXNS); std::iota(indexes.begin(), indexes.end(), 0); - const auto orphanage{node::MakeTxOrphanage(/*max_global_ann=*/node::DEFAULT_MAX_ORPHANAGE_LATENCY_SCORE, /*reserved_peer_usage=*/node::DEFAULT_RESERVED_ORPHAN_WEIGHT_PER_PEER)}; + const auto orphanage{node::MakeTxOrphanage(/*max_global_latency_score=*/node::DEFAULT_MAX_ORPHANAGE_LATENCY_SCORE, /*reserved_peer_usage=*/node::DEFAULT_RESERVED_ORPHAN_WEIGHT_PER_PEER)}; // Every peer sends the same transactions, all from shared_txs. // Each peer has 1 or 2 assigned transactions, which they must place as the last and second-to-last positions. // The assignments ensure that every transaction is in some peer's last 2 transactions, and is thus remains in the orphanage until the end of LimitOrphans. @@ -178,7 +177,6 @@ static void OrphanageMultiPeerEviction(benchmark::Bench& bench) const auto num_announcements_before_trim{orphanage->CountAnnouncements()}; // There is a small gap between the total usage and the max usage. Add a transaction to fill it. assert(orphanage->AddTx(last_tx, 0)); - orphanage->LimitOrphans(); // If there are multiple peers, note that they all have the same DoS score. We will evict only 1 item at a time for each new DoSiest peer. const auto num_evicted{num_announcements_before_trim - orphanage->CountAnnouncements() + 1}; @@ -191,7 +189,7 @@ static void OrphanageMultiPeerEviction(benchmark::Bench& bench) static void OrphanageEraseAll(benchmark::Bench& bench, bool block_or_disconnect) { FastRandomContext det_rand{true}; - const auto orphanage{node::MakeTxOrphanage(/*max_global_ann=*/node::DEFAULT_MAX_ORPHANAGE_LATENCY_SCORE, /*reserved_peer_usage=*/node::DEFAULT_RESERVED_ORPHAN_WEIGHT_PER_PEER)}; + const auto orphanage{node::MakeTxOrphanage(/*max_global_latency_score=*/node::DEFAULT_MAX_ORPHANAGE_LATENCY_SCORE, /*reserved_peer_usage=*/node::DEFAULT_RESERVED_ORPHAN_WEIGHT_PER_PEER)}; // This is an unrealistically large number of inputs for a block, as there is almost no room given to witness data, // outputs, and overhead for individual transactions. The entire block is 1 transaction with 20,000 inputs. constexpr unsigned int NUM_BLOCK_INPUTS{MAX_BLOCK_WEIGHT / APPROX_WEIGHT_PER_INPUT}; diff --git a/src/init.cpp b/src/init.cpp index 08817463b11..b659debf8c8 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -490,6 +490,8 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc) argsman.AddArg("-allowignoredconf", strprintf("For backwards compatibility, treat an unused %s file in the datadir as a warning, not an error.", BITCOIN_CONF_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-loadblock=", "Imports blocks from external file on startup", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-maxmempool=", strprintf("Keep the transaction memory pool below megabytes (default: %u)", DEFAULT_MAX_MEMPOOL_SIZE_MB), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); + // TODO: remove in v31.0 + argsman.AddArg("-maxorphantx=", strprintf("(Removed option, see release notes)"), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-mempoolexpiry=", strprintf("Do not keep transactions in the mempool longer than hours (default: %u)", DEFAULT_MEMPOOL_EXPIRY_HOURS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); argsman.AddArg("-minimumchainwork=", strprintf("Minimum work assumed to exist on a valid chain in hex (default: %s, testnet3: %s, testnet4: %s, signet: %s)", defaultChainParams->GetConsensus().nMinimumChainWork.GetHex(), testnetChainParams->GetConsensus().nMinimumChainWork.GetHex(), testnet4ChainParams->GetConsensus().nMinimumChainWork.GetHex(), signetChainParams->GetConsensus().nMinimumChainWork.GetHex()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS); argsman.AddArg("-par=", strprintf("Set the number of script verification threads (0 = auto, up to %d, <0 = leave that many cores free, default: %d)", @@ -893,6 +895,11 @@ bool AppInitParameterInteraction(const ArgsManager& args) InitWarning(_("Options '-datacarrier' or '-datacarriersize' are set but are marked as deprecated. They will be removed in a future version.")); } + // We no longer limit the orphanage based on number of transactions but keep the option to warn users who still have it in their config. + if (args.IsArgSet("-maxorphantx")) { + InitWarning(_("Option '-maxorphantx' is set but no longer has any effect (see release notes). Please remove it from your configuration.")); + } + // Error if network-specific options (-addnode, -connect, etc) are // specified in default section of config file, but not overridden // on the command line or in this chain's section of the config file. diff --git a/src/net_processing.cpp b/src/net_processing.cpp index d02e5c21458..61439f71883 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -533,7 +533,7 @@ public: std::optional FetchBlock(NodeId peer_id, const CBlockIndex& block_index) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); - std::vector GetOrphanTransactions() override EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex); + std::vector GetOrphanTransactions() override EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex); PeerManagerInfo GetInfo() const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); void RelayTransaction(const Txid& txid, const Wtxid& wtxid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); @@ -1754,7 +1754,7 @@ bool PeerManagerImpl::GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) c return true; } -std::vector PeerManagerImpl::GetOrphanTransactions() +std::vector PeerManagerImpl::GetOrphanTransactions() { LOCK(m_tx_download_mutex); return m_txdownloadman.GetOrphanTransactions(); diff --git a/src/net_processing.h b/src/net_processing.h index 9cf79512172..8c140d98ad6 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -109,7 +109,7 @@ public: /** Get statistics from node state */ virtual bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const = 0; - virtual std::vector GetOrphanTransactions() = 0; + virtual std::vector GetOrphanTransactions() = 0; /** Get peer manager info. */ virtual PeerManagerInfo GetInfo() const = 0; diff --git a/src/node/txdownloadman.h b/src/node/txdownloadman.h index 3ebf66324a8..90dc0e343af 100644 --- a/src/node/txdownloadman.h +++ b/src/node/txdownloadman.h @@ -170,7 +170,7 @@ public: void CheckIsEmpty(NodeId nodeid) const; /** Wrapper for TxOrphanage::GetOrphanTransactions */ - std::vector GetOrphanTransactions() const; + std::vector GetOrphanTransactions() const; }; } // namespace node #endif // BITCOIN_NODE_TXDOWNLOADMAN_H diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index 3822cc81fdc..0105fe3d609 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -83,7 +83,7 @@ void TxDownloadManager::CheckIsEmpty(NodeId nodeid) const { m_impl->CheckIsEmpty(nodeid); } -std::vector TxDownloadManager::GetOrphanTransactions() const +std::vector TxDownloadManager::GetOrphanTransactions() const { return m_impl->GetOrphanTransactions(); } @@ -188,7 +188,6 @@ bool TxDownloadManagerImpl::AddTxAnnouncement(NodeId peer, const GenTxid& gtxid, if (MaybeAddOrphanResolutionCandidate(unique_parents, *wtxid, peer, now)) { m_orphanage->AddAnnouncer(orphan_tx->GetWitnessHash(), peer); - m_orphanage->LimitOrphans(); } // Return even if the peer isn't an orphan resolution candidate. This would be caught by AlreadyHaveTx. @@ -419,9 +418,6 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction // Once added to the orphan pool, a tx is considered AlreadyHave, and we shouldn't request it anymore. m_txrequest.ForgetTxHash(tx.GetHash()); m_txrequest.ForgetTxHash(tx.GetWitnessHash()); - - // DoS prevention: do not allow m_orphanage to grow unbounded (see CVE-2012-3789) - m_orphanage->LimitOrphans(); } else { unique_parents.clear(); LogDebug(BCLog::MEMPOOL, "not keeping orphan with rejected parents %s (wtxid=%s)\n", @@ -576,11 +572,11 @@ void TxDownloadManagerImpl::CheckIsEmpty(NodeId nodeid) void TxDownloadManagerImpl::CheckIsEmpty() { assert(m_orphanage->TotalOrphanUsage() == 0); - assert(m_orphanage->Size() == 0); + assert(m_orphanage->CountUniqueOrphans() == 0); assert(m_txrequest.Size() == 0); assert(m_num_wtxid_peers == 0); } -std::vector TxDownloadManagerImpl::GetOrphanTransactions() const +std::vector TxDownloadManagerImpl::GetOrphanTransactions() const { return m_orphanage->GetOrphanTransactions(); } diff --git a/src/node/txdownloadman_impl.h b/src/node/txdownloadman_impl.h index b10c71d5d24..8af0cb227f5 100644 --- a/src/node/txdownloadman_impl.h +++ b/src/node/txdownloadman_impl.h @@ -188,7 +188,7 @@ public: void CheckIsEmpty(); void CheckIsEmpty(NodeId nodeid); - std::vector GetOrphanTransactions() const; + std::vector GetOrphanTransactions() const; protected: /** Helper for getting deduplicated vector of Txids in vin. */ diff --git a/src/node/txorphanage.cpp b/src/node/txorphanage.cpp index ce087d17ffb..164fecdc167 100644 --- a/src/node/txorphanage.cpp +++ b/src/node/txorphanage.cpp @@ -50,9 +50,9 @@ class TxOrphanageImpl final : public TxOrphanage { { } /** Get an approximation for "memory usage". The total memory is a function of the memory used to store the - * transaction itself, each entry in m_orphans, and each entry in m_outpoint_to_orphan_it. We use weight because - * it is often higher than the actual memory usage of the tranaction. This metric conveniently encompasses - * m_outpoint_to_orphan_it usage since input data does not get the witness discount, and makes it easier to + * transaction itself, each entry in m_orphans, and each entry in m_outpoint_to_orphan_wtxids. We use weight because + * it is often higher than the actual memory usage of the transaction. This metric conveniently encompasses + * m_outpoint_to_orphan_wtxids usage since input data does not get the witness discount, and makes it easier to * reason about each peer's limits using well-understood transaction attributes. */ TxOrphanage::Usage GetMemUsage() const { return GetTransactionWeight(*m_tx); @@ -60,7 +60,7 @@ class TxOrphanageImpl final : public TxOrphanage { /** Get an approximation of how much this transaction contributes to latency in EraseForBlock and EraseForPeer. * The computation time is a function of the number of entries in m_orphans (thus 1 per announcement) and the - * number of entries in m_outpoint_to_orphan_it (thus an additional 1 for every 10 inputs). Transactions with a + * number of entries in m_outpoint_to_orphan_wtxids (thus an additional 1 for every 10 inputs). Transactions with a * small number of inputs (9 or fewer) are counted as 1 to make it easier to reason about each peer's limits in * terms of "normal" transactions. */ TxOrphanage::Count GetLatencyScore() const { @@ -117,7 +117,10 @@ class TxOrphanageImpl final : public TxOrphanage { /** Index from the parents' outputs to wtxids that exist in m_orphans. Used to find children of * a transaction that can be reconsidered and to remove entries that conflict with a block.*/ - std::unordered_map, SaltedOutpointHasher> m_outpoint_to_orphan_it; + std::unordered_map, SaltedOutpointHasher> m_outpoint_to_orphan_wtxids; + + /** Set of Wtxids for which (exactly) one announcement with m_reconsider=true exists. */ + std::set m_reconsiderable_wtxids; struct PeerDoSInfo { TxOrphanage::Usage m_total_usage{0}; @@ -155,13 +158,13 @@ class TxOrphanageImpl final : public TxOrphanage { * do not trim unless the orphanage exceeds global limits, but it means that this peer will * be selected for trimming sooner. If the global latency score or global memory usage * limits are exceeded, it must be that there is a peer whose DoS score > 1. */ - FeeFrac GetDosScore(TxOrphanage::Count max_peer_latency_score, TxOrphanage::Usage max_peer_bytes) const + FeeFrac GetDosScore(TxOrphanage::Count max_peer_latency_score, TxOrphanage::Usage max_peer_memory) const { assert(max_peer_latency_score > 0); - assert(max_peer_bytes > 0); - const FeeFrac cpu_score(m_total_latency_score, max_peer_latency_score); - const FeeFrac mem_score(m_total_usage, max_peer_bytes); - return std::max(cpu_score, mem_score); + assert(max_peer_memory > 0); + const FeeFrac latency_score(m_total_latency_score, max_peer_latency_score); + const FeeFrac mem_score(m_total_usage, max_peer_memory); + return std::max(latency_score, mem_score); } }; /** Store per-peer statistics. Used to determine each peer's DoS score. The size of this map is used to determine the @@ -172,15 +175,22 @@ class TxOrphanageImpl final : public TxOrphanage { template void Erase(Iter it); + /** Erase by wtxid. */ + bool EraseTxInternal(const Wtxid& wtxid); + /** Check if there is exactly one announcement with the same wtxid as it. */ bool IsUnique(Iter it) const; /** Check if the orphanage needs trimming. */ bool NeedsTrim() const; + + /** Limit the orphanage to MaxGlobalLatencyScore and MaxGlobalUsage. */ + void LimitOrphans(); + public: TxOrphanageImpl() = default; - TxOrphanageImpl(Count max_global_ann, Usage reserved_peer_usage) : - m_max_global_latency_score{max_global_ann}, + TxOrphanageImpl(Count max_global_latency_score, Usage reserved_peer_usage) : + m_max_global_latency_score{max_global_latency_score}, m_reserved_usage_per_peer{reserved_peer_usage} {} ~TxOrphanageImpl() noexcept override = default; @@ -195,7 +205,7 @@ public: TxOrphanage::Count TotalLatencyScore() const override; TxOrphanage::Usage ReservedPeerUsage() const override; - /** Maximum allowed (deduplicated) latency score for all tranactions (see Announcement::GetLatencyScore()). Dynamic + /** Maximum allowed (deduplicated) latency score for all transactions (see Announcement::GetLatencyScore()). Dynamic * based on number of peers. Each peer has an equal amount, but the global maximum latency score stays constant. The * number of peers times MaxPeerLatencyScore() (rounded) adds up to MaxGlobalLatencyScore(). As long as every peer's * m_total_latency_score / MaxPeerLatencyScore() < 1, MaxGlobalLatencyScore() is not exceeded. */ @@ -216,12 +226,10 @@ public: bool EraseTx(const Wtxid& wtxid) override; void EraseForPeer(NodeId peer) override; void EraseForBlock(const CBlock& block) override; - void LimitOrphans() override; std::vector> AddChildrenToWorkSet(const CTransaction& tx, FastRandomContext& rng) override; bool HaveTxToReconsider(NodeId peer) override; std::vector GetChildrenFromSamePeer(const CTransactionRef& parent, NodeId nodeid) const override; - size_t Size() const override { return m_unique_orphans; } - std::vector GetOrphanTransactions() const override; + std::vector GetOrphanTransactions() const override; TxOrphanage::Usage TotalOrphanUsage() const override; void SanityCheck() const override; }; @@ -242,19 +250,24 @@ void TxOrphanageImpl::Erase(Iter it) m_unique_rounded_input_scores -= it->GetLatencyScore() - 1; m_unique_orphan_usage -= it->GetMemUsage(); - // Remove references in m_outpoint_to_orphan_it + // Remove references in m_outpoint_to_orphan_wtxids const auto& wtxid{it->m_tx->GetWitnessHash()}; for (const auto& input : it->m_tx->vin) { - auto it_prev = m_outpoint_to_orphan_it.find(input.prevout); - if (it_prev != m_outpoint_to_orphan_it.end()) { + auto it_prev = m_outpoint_to_orphan_wtxids.find(input.prevout); + if (it_prev != m_outpoint_to_orphan_wtxids.end()) { it_prev->second.erase(wtxid); // Clean up keys if they point to an empty set. if (it_prev->second.empty()) { - m_outpoint_to_orphan_it.erase(it_prev); + m_outpoint_to_orphan_wtxids.erase(it_prev); } } } } + + // If this was the (unique) reconsiderable announcement for its wtxid, then the wtxid won't + // have any reconsiderable announcements left after erasing. + if (it->m_reconsider) m_reconsiderable_wtxids.erase(it->m_tx->GetWitnessHash()); + m_orphans.get().erase(it); } @@ -313,10 +326,10 @@ bool TxOrphanageImpl::AddTx(const CTransactionRef& tx, NodeId peer) auto& peer_info = m_peer_orphanage_info.try_emplace(peer).first->second; peer_info.Add(*iter); - // Add links in m_outpoint_to_orphan_it + // Add links in m_outpoint_to_orphan_wtxids if (brand_new) { for (const auto& input : tx->vin) { - auto& wtxids_for_prevout = m_outpoint_to_orphan_it.try_emplace(input.prevout).first->second; + auto& wtxids_for_prevout = m_outpoint_to_orphan_wtxids.try_emplace(input.prevout).first->second; wtxids_for_prevout.emplace(wtxid); } @@ -325,13 +338,16 @@ bool TxOrphanageImpl::AddTx(const CTransactionRef& tx, NodeId peer) m_unique_rounded_input_scores += iter->GetLatencyScore() - 1; LogDebug(BCLog::TXPACKAGES, "stored orphan tx %s (wtxid=%s), weight: %u (mapsz %u outsz %u)\n", - txid.ToString(), wtxid.ToString(), sz, m_orphans.size(), m_outpoint_to_orphan_it.size()); + txid.ToString(), wtxid.ToString(), sz, m_orphans.size(), m_outpoint_to_orphan_wtxids.size()); Assume(IsUnique(iter)); } else { LogDebug(BCLog::TXPACKAGES, "added peer=%d as announcer of orphan tx %s (wtxid=%s)\n", peer, txid.ToString(), wtxid.ToString()); Assume(!IsUnique(iter)); } + + // DoS prevention: do not allow m_orphanage to grow unbounded (see CVE-2012-3789) + LimitOrphans(); return brand_new; } @@ -360,10 +376,13 @@ bool TxOrphanageImpl::AddAnnouncer(const Wtxid& wtxid, NodeId peer) peer, txid.ToString(), wtxid.ToString()); Assume(!IsUnique(iter)); + + // DoS prevention: do not allow m_orphanage to grow unbounded (see CVE-2012-3789) + LimitOrphans(); return true; } -bool TxOrphanageImpl::EraseTx(const Wtxid& wtxid) +bool TxOrphanageImpl::EraseTxInternal(const Wtxid& wtxid) { auto& index_by_wtxid = m_orphans.get(); @@ -378,12 +397,21 @@ bool TxOrphanageImpl::EraseTx(const Wtxid& wtxid) Erase(it++); num_ann += 1; } - LogDebug(BCLog::TXPACKAGES, "removed orphan tx %s (wtxid=%s) (%u announcements)\n", txid.ToString(), wtxid.ToString(), num_ann); return true; } +bool TxOrphanageImpl::EraseTx(const Wtxid& wtxid) +{ + const auto ret = EraseTxInternal(wtxid); + + // Deletions can cause the orphanage's MaxGlobalUsage to decrease, so we may need to trim here. + LimitOrphans(); + + return ret; +} + /** Erase all entries by this peer. */ void TxOrphanageImpl::EraseForPeer(NodeId peer) { @@ -393,13 +421,16 @@ void TxOrphanageImpl::EraseForPeer(NodeId peer) unsigned int num_ann{0}; while (it != index_by_peer.end() && it->m_announcer == peer) { - // Delete item, cleaning up m_outpoint_to_orphan_it iff this entry is unique by wtxid. + // Delete item, cleaning up m_outpoint_to_orphan_wtxids iff this entry is unique by wtxid. Erase(it++); num_ann += 1; } Assume(!m_peer_orphanage_info.contains(peer)); if (num_ann > 0) LogDebug(BCLog::TXPACKAGES, "Erased %d orphan transaction(s) from peer=%d\n", num_ann, peer); + + // Deletions can cause the orphanage's MaxGlobalUsage to decrease, so we may need to trim here. + LimitOrphans(); } /** If the data structure needs trimming, evicts announcements by selecting the DoSiest peer and evicting its oldest @@ -416,7 +447,7 @@ void TxOrphanageImpl::LimitOrphans() // Even though it's possible for MaxPeerLatencyScore to increase within this call to LimitOrphans // (e.g. if a peer's orphans are removed entirely, changing the number of peers), use consistent limits throughout. - const auto max_ann{MaxPeerLatencyScore()}; + const auto max_lat{MaxPeerLatencyScore()}; const auto max_mem{ReservedPeerUsage()}; // We have exceeded the global limit(s). Now, identify who is using too much and evict their orphans. @@ -425,7 +456,7 @@ void TxOrphanageImpl::LimitOrphans() heap_peer_dos.reserve(m_peer_orphanage_info.size()); for (const auto& [nodeid, entry] : m_peer_orphanage_info) { // Performance optimization: only consider peers with a DoS score > 1. - const auto dos_score = entry.GetDosScore(max_ann, max_mem); + const auto dos_score = entry.GetDosScore(max_lat, max_mem); if (dos_score >> FeeFrac{1, 1}) { heap_peer_dos.emplace_back(nodeid, dos_score); } @@ -438,11 +469,11 @@ void TxOrphanageImpl::LimitOrphans() std::make_heap(heap_peer_dos.begin(), heap_peer_dos.end(), compare_score); unsigned int num_erased{0}; - // This outer loop finds the peer with the highest DoS score, which is a fraction of {usage, announcements} used + // This outer loop finds the peer with the highest DoS score, which is a fraction of memory and latency scores // over the respective allowances. We continue until the orphanage is within global limits. That means some peers // might still have a DoS score > 1 at the end. - // Note: if ratios are the same, FeeFrac tiebreaks by denominator. In practice, since the CPU denominator (number of - // announcements) is always lower, this means that a peer with only high number of announcements will be targeted + // Note: if ratios are the same, FeeFrac tiebreaks by denominator. In practice, since the latency denominator (number of + // announcements and inputs) is always lower, this means that a peer with only high latency scores will be targeted // before a peer using a lot of memory, even if they have the same ratios. do { Assume(!heap_peer_dos.empty()); @@ -463,24 +494,28 @@ void TxOrphanageImpl::LimitOrphans() // The number of inner loop iterations is bounded by the total number of announcements. const auto& dos_threshold = heap_peer_dos.empty() ? FeeFrac{1, 1} : heap_peer_dos.front().second; auto it_ann = m_orphans.get().lower_bound(ByPeerView{worst_peer, false, 0}); + unsigned int num_erased_this_round{0}; + unsigned int starting_num_ann{it_worst_peer->second.m_count_announcements}; while (NeedsTrim()) { - if (!Assume(it_ann->m_announcer == worst_peer)) break; if (!Assume(it_ann != m_orphans.get().end())) break; + if (!Assume(it_ann->m_announcer == worst_peer)) break; Erase(it_ann++); num_erased += 1; + num_erased_this_round += 1; // If we erased the last orphan from this peer, it_worst_peer will be invalidated. it_worst_peer = m_peer_orphanage_info.find(worst_peer); - if (it_worst_peer == m_peer_orphanage_info.end() || it_worst_peer->second.GetDosScore(max_ann, max_mem) <= dos_threshold) break; + if (it_worst_peer == m_peer_orphanage_info.end() || it_worst_peer->second.GetDosScore(max_lat, max_mem) <= dos_threshold) break; } + LogDebug(BCLog::TXPACKAGES, "peer=%d orphanage overflow, removed %u of %u announcements\n", worst_peer, num_erased_this_round, starting_num_ann); if (!NeedsTrim()) break; // Unless this peer is empty, put it back in the heap so we continue to consider evicting its orphans. // We may select this peer for evictions again if there are multiple DoSy peers. if (it_worst_peer != m_peer_orphanage_info.end() && it_worst_peer->second.m_count_announcements > 0) { - heap_peer_dos.emplace_back(worst_peer, it_worst_peer->second.GetDosScore(max_ann, max_mem)); + heap_peer_dos.emplace_back(worst_peer, it_worst_peer->second.GetDosScore(max_lat, max_mem)); std::push_heap(heap_peer_dos.begin(), heap_peer_dos.end(), compare_score); } } while (true); @@ -494,12 +529,15 @@ std::vector> TxOrphanageImpl::AddChildrenToWorkSet(cons std::vector> ret; auto& index_by_wtxid = m_orphans.get(); for (unsigned int i = 0; i < tx.vout.size(); i++) { - const auto it_by_prev = m_outpoint_to_orphan_it.find(COutPoint(tx.GetHash(), i)); - if (it_by_prev != m_outpoint_to_orphan_it.end()) { + const auto it_by_prev = m_outpoint_to_orphan_wtxids.find(COutPoint(tx.GetHash(), i)); + if (it_by_prev != m_outpoint_to_orphan_wtxids.end()) { for (const auto& wtxid : it_by_prev->second) { - // Belt and suspenders, each entry in m_outpoint_to_orphan_it should always have at least 1 announcement. + // If a reconsiderable announcement for this wtxid already exists, skip it. + if (m_reconsiderable_wtxids.contains(wtxid)) continue; + + // Belt and suspenders, each entry in m_outpoint_to_orphan_wtxids should always have at least 1 announcement. auto it = index_by_wtxid.lower_bound(ByWtxidView{wtxid, MIN_PEER}); - if (!Assume(it != index_by_wtxid.end())) continue; + if (!Assume(it != index_by_wtxid.end() && it->m_tx->GetWitnessHash() == wtxid)) continue; // Select a random peer to assign orphan processing, reducing wasted work if the orphan is still missing // inputs. However, we don't want to create an issue in which the assigned peer can purposefully stop us @@ -513,10 +551,10 @@ std::vector> TxOrphanageImpl::AddChildrenToWorkSet(cons // Mark this orphan as ready to be reconsidered. static constexpr auto mark_reconsidered_modifier = [](auto& ann) { ann.m_reconsider = true; }; - if (!it->m_reconsider) { - index_by_wtxid.modify(it, mark_reconsidered_modifier); - ret.emplace_back(wtxid, it->m_announcer); - } + Assume(!it->m_reconsider); + index_by_wtxid.modify(it, mark_reconsidered_modifier); + ret.emplace_back(wtxid, it->m_announcer); + m_reconsiderable_wtxids.insert(wtxid); LogDebug(BCLog::TXPACKAGES, "added %s (wtxid=%s) to peer %d workset\n", it->m_tx->GetHash().ToString(), it->m_tx->GetWitnessHash().ToString(), it->m_announcer); @@ -554,6 +592,9 @@ CTransactionRef TxOrphanageImpl::GetTxToReconsider(NodeId peer) // reconsidered again until there is a new reason to do so. static constexpr auto mark_reconsidered_modifier = [](auto& ann) { ann.m_reconsider = false; }; m_orphans.get().modify(it, mark_reconsidered_modifier); + // As there is exactly one m_reconsider announcement per reconsiderable wtxids, flipping + // the m_reconsider flag means the wtxid is no longer reconsiderable. + m_reconsiderable_wtxids.erase(it->m_tx->GetWitnessHash()); return it->m_tx; } return nullptr; @@ -565,6 +606,7 @@ bool TxOrphanageImpl::HaveTxToReconsider(NodeId peer) auto it = m_orphans.get().lower_bound(ByPeerView{peer, true, 0}); return it != m_orphans.get().end() && it->m_announcer == peer && it->m_reconsider; } + void TxOrphanageImpl::EraseForBlock(const CBlock& block) { if (m_orphans.empty()) return; @@ -575,8 +617,8 @@ void TxOrphanageImpl::EraseForBlock(const CBlock& block) // Which orphan pool entries must we evict? for (const auto& input : block_tx.vin) { - auto it_prev = m_outpoint_to_orphan_it.find(input.prevout); - if (it_prev != m_outpoint_to_orphan_it.end()) { + auto it_prev = m_outpoint_to_orphan_wtxids.find(input.prevout); + if (it_prev != m_outpoint_to_orphan_wtxids.end()) { // Copy all wtxids to wtxids_to_erase. std::copy(it_prev->second.cbegin(), it_prev->second.cend(), std::inserter(wtxids_to_erase, wtxids_to_erase.end())); } @@ -585,17 +627,21 @@ void TxOrphanageImpl::EraseForBlock(const CBlock& block) unsigned int num_erased{0}; for (const auto& wtxid : wtxids_to_erase) { - num_erased += EraseTx(wtxid) ? 1 : 0; + // Don't use EraseTx here because it calls LimitOrphans and announcements deleted in that call are not reflected + // in its return result. Waiting until the end to do LimitOrphans helps save repeated computation and allows us + // to check that num_erased is what we expect. + num_erased += EraseTxInternal(wtxid) ? 1 : 0; } if (num_erased != 0) { LogDebug(BCLog::TXPACKAGES, "Erased %d orphan transaction(s) included or conflicted by block\n", num_erased); } Assume(wtxids_to_erase.size() == num_erased); + + // Deletions can cause the orphanage's MaxGlobalUsage to decrease, so we may need to trim here. + LimitOrphans(); } -/** Get all children that spend from this tx and were received from nodeid. Sorted from most - * recent to least recent. */ std::vector TxOrphanageImpl::GetChildrenFromSamePeer(const CTransactionRef& parent, NodeId peer) const { std::vector children_found; @@ -623,9 +669,9 @@ std::vector TxOrphanageImpl::GetChildrenFromSamePeer(const CTra return children_found; } -std::vector TxOrphanageImpl::GetOrphanTransactions() const +std::vector TxOrphanageImpl::GetOrphanTransactions() const { - std::vector result; + std::vector result; result.reserve(m_unique_orphans); auto& index_by_wtxid = m_orphans.get(); @@ -633,7 +679,7 @@ std::vector TxOrphanageImpl::GetOrphanTransactions() std::set this_orphan_announcers; while (it != index_by_wtxid.end()) { this_orphan_announcers.insert(it->m_announcer); - // If this is the last entry, or the next entry has a different wtxid, build a OrphanTxBase. + // If this is the last entry, or the next entry has a different wtxid, build a OrphanInfo. if (std::next(it) == index_by_wtxid.end() || std::next(it)->m_tx->GetWitnessHash() != it->m_tx->GetWitnessHash()) { result.emplace_back(it->m_tx, std::move(this_orphan_announcers)); this_orphan_announcers.clear(); @@ -651,6 +697,7 @@ void TxOrphanageImpl::SanityCheck() const std::unordered_map reconstructed_peer_info; std::map> unique_wtxids_to_scores; std::set all_outpoints; + std::set reconstructed_reconsiderable_wtxids; for (auto it = m_orphans.begin(); it != m_orphans.end(); ++it) { for (const auto& input : it->m_tx->vin) { @@ -662,14 +709,26 @@ void TxOrphanageImpl::SanityCheck() const peer_info.m_total_usage += it->GetMemUsage(); peer_info.m_count_announcements += 1; peer_info.m_total_latency_score += it->GetLatencyScore(); + + if (it->m_reconsider) { + auto [_, added] = reconstructed_reconsiderable_wtxids.insert(it->m_tx->GetWitnessHash()); + // Check that there is only ever 1 announcement per wtxid with m_reconsider set. + assert(added); + } } assert(reconstructed_peer_info.size() == m_peer_orphanage_info.size()); - // All outpoints exist in m_outpoint_to_orphan_it, all keys in m_outpoint_to_orphan_it correspond to some - // orphan, and all wtxids referenced in m_outpoint_to_orphan_it are also in m_orphans. - // This ensures m_outpoint_to_orphan_it is cleaned up. - assert(all_outpoints.size() == m_outpoint_to_orphan_it.size()); - for (const auto& [outpoint, wtxid_set] : m_outpoint_to_orphan_it) { + // Recalculated per-peer stats are identical to m_peer_orphanage_info + assert(reconstructed_peer_info == m_peer_orphanage_info); + + // Recalculated set of reconsiderable wtxids must match. + assert(m_reconsiderable_wtxids == reconstructed_reconsiderable_wtxids); + + // All outpoints exist in m_outpoint_to_orphan_wtxids, all keys in m_outpoint_to_orphan_wtxids correspond to some + // orphan, and all wtxids referenced in m_outpoint_to_orphan_wtxids are also in m_orphans. + // This ensures m_outpoint_to_orphan_wtxids is cleaned up. + assert(all_outpoints.size() == m_outpoint_to_orphan_wtxids.size()); + for (const auto& [outpoint, wtxid_set] : m_outpoint_to_orphan_wtxids) { assert(all_outpoints.contains(outpoint)); for (const auto& wtxid : wtxid_set) { assert(unique_wtxids_to_scores.contains(wtxid)); @@ -699,6 +758,8 @@ void TxOrphanageImpl::SanityCheck() const const auto summed_peer_latency_score = std::accumulate(m_peer_orphanage_info.begin(), m_peer_orphanage_info.end(), TxOrphanage::Count{0}, [](TxOrphanage::Count sum, const auto pair) { return sum + pair.second.m_total_latency_score; }); assert(summed_peer_latency_score >= m_unique_rounded_input_scores + m_orphans.size()); + + assert(!NeedsTrim()); } TxOrphanage::Count TxOrphanageImpl::MaxGlobalLatencyScore() const { return m_max_global_latency_score; } @@ -715,8 +776,8 @@ std::unique_ptr MakeTxOrphanage() noexcept { return std::make_unique(); } -std::unique_ptr MakeTxOrphanage(TxOrphanage::Count max_global_ann, TxOrphanage::Usage reserved_peer_usage) noexcept +std::unique_ptr MakeTxOrphanage(TxOrphanage::Count max_global_latency_score, TxOrphanage::Usage reserved_peer_usage) noexcept { - return std::make_unique(max_global_ann, reserved_peer_usage); + return std::make_unique(max_global_latency_score, reserved_peer_usage); } } // namespace node diff --git a/src/node/txorphanage.h b/src/node/txorphanage.h index ae639213944..5cbf3451d0d 100644 --- a/src/node/txorphanage.h +++ b/src/node/txorphanage.h @@ -41,13 +41,13 @@ public: using Count = unsigned int; /** Allows providing orphan information externally */ - struct OrphanTxBase { + struct OrphanInfo { CTransactionRef tx; /** Peers added with AddTx or AddAnnouncer. */ std::set announcers; // Constructor with moved announcers - OrphanTxBase(CTransactionRef tx, std::set&& announcers) : + OrphanInfo(CTransactionRef tx, std::set&& announcers) : tx(std::move(tx)), announcers(std::move(announcers)) {} @@ -88,24 +88,18 @@ public: /** Erase all orphans included in or invalidated by a new block */ virtual void EraseForBlock(const CBlock& block) = 0; - /** Limit the orphanage to MaxGlobalLatencyScore and MaxGlobalUsage. */ - virtual void LimitOrphans() = 0; - /** Add any orphans that list a particular tx as a parent into the from peer's work set */ virtual std::vector> AddChildrenToWorkSet(const CTransaction& tx, FastRandomContext& rng) = 0; /** Does this peer have any work to do? */ virtual bool HaveTxToReconsider(NodeId peer) = 0; - /** Get all children that spend from this tx and were received from nodeid. Sorted from most - * recent to least recent. */ + /** Get all children that spend from this tx and were received from nodeid. Sorted + * reconsiderable before non-reconsiderable, then from most recent to least recent. */ virtual std::vector GetChildrenFromSamePeer(const CTransactionRef& parent, NodeId nodeid) const = 0; - /** Return how many entries exist in the orphange */ - virtual size_t Size() const = 0; - /** Get all orphan transactions */ - virtual std::vector GetOrphanTransactions() const = 0; + virtual std::vector GetOrphanTransactions() const = 0; /** Get the total usage (weight) of all orphans. If an orphan has multiple announcers, its usage is * only counted once within this total. */ @@ -152,6 +146,6 @@ public: /** Create a new TxOrphanage instance */ std::unique_ptr MakeTxOrphanage() noexcept; -std::unique_ptr MakeTxOrphanage(TxOrphanage::Count max_global_ann, TxOrphanage::Usage reserved_peer_usage) noexcept; +std::unique_ptr MakeTxOrphanage(TxOrphanage::Count max_global_latency_score, TxOrphanage::Usage reserved_peer_usage) noexcept; } // namespace node #endif // BITCOIN_NODE_TXORPHANAGE_H diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 284d93d0de7..df5d22828b3 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -843,7 +843,7 @@ static std::vector OrphanDescription() }; } -static UniValue OrphanToJSON(const node::TxOrphanage::OrphanTxBase& orphan) +static UniValue OrphanToJSON(const node::TxOrphanage::OrphanInfo& orphan) { UniValue o(UniValue::VOBJ); o.pushKV("txid", orphan.tx->GetHash().ToString()); @@ -899,7 +899,7 @@ static RPCHelpMan getorphantxs() { const NodeContext& node = EnsureAnyNodeContext(request.context); PeerManager& peerman = EnsurePeerman(node); - std::vector orphanage = peerman.GetOrphanTransactions(); + std::vector orphanage = peerman.GetOrphanTransactions(); int verbosity{ParseVerbosity(request.params[0], /*default_verbosity=*/0, /*allow_bool*/false)}; diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index 36d7af6cc7b..e6b3444749d 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/src/test/fuzz/txorphan.cpp @@ -122,37 +122,34 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) }, [&] { bool have_tx = orphanage->HaveTx(tx->GetWitnessHash()); + bool have_tx_and_peer = orphanage->HaveTxFromPeer(wtxid, peer_id); // AddTx should return false if tx is too big or already have it // tx weight is unknown, we only check when tx is already in orphanage { bool add_tx = orphanage->AddTx(tx, peer_id); // have_tx == true -> add_tx == false Assert(!have_tx || !add_tx); + // have_tx_and_peer == true -> add_tx == false + Assert(!have_tx_and_peer || !add_tx); + // After AddTx, the orphanage may trim itself, so the peer's usage may have gone up or down. if (add_tx) { - Assert(orphanage->UsageByPeer(peer_id) == tx_weight + total_peer_bytes_start); - Assert(orphanage->TotalOrphanUsage() == tx_weight + total_bytes_start); Assert(tx_weight <= MAX_STANDARD_TX_WEIGHT); } else { // Peer may have been added as an announcer. - if (orphanage->UsageByPeer(peer_id) == tx_weight + total_peer_bytes_start) { + if (orphanage->UsageByPeer(peer_id) > total_peer_bytes_start) { Assert(orphanage->HaveTxFromPeer(wtxid, peer_id)); - } else { - // Otherwise, there must not be any change to the peer byte count. - Assert(orphanage->UsageByPeer(peer_id) == total_peer_bytes_start); } - // Regardless, total bytes should not have changed. - Assert(orphanage->TotalOrphanUsage() == total_bytes_start); + // If announcement was added, total bytes does not increase. + // However, if eviction was triggered, the value may decrease. + Assert(orphanage->TotalOrphanUsage() <= total_bytes_start); } } - have_tx = orphanage->HaveTx(tx->GetWitnessHash()); - { - bool add_tx = orphanage->AddTx(tx, peer_id); - // if have_tx is still false, it must be too big - Assert(!have_tx == (tx_weight > MAX_STANDARD_TX_WEIGHT)); - Assert(!have_tx || !add_tx); - } + // We are not guaranteed to have_tx after AddTx. There are a few possibile reasons: + // - tx itself exceeds the per-peer memory usage limit, so LimitOrphans had to remove it immediately + // - tx itself exceeds the per-peer latency score limit, so LimitOrphans had to remove it immediately + // - the orphanage needed trim and all other announcements from this peer are reconsiderable }, [&] { bool have_tx = orphanage->HaveTx(tx->GetWitnessHash()); @@ -165,14 +162,9 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) // have_tx_and_peer == true -> added_announcer == false Assert(!have_tx_and_peer || !added_announcer); - // Total bytes should not have changed. If peer was added as announcer, byte - // accounting must have been updated. - Assert(orphanage->TotalOrphanUsage() == total_bytes_start); - if (added_announcer) { - Assert(orphanage->UsageByPeer(peer_id) == tx_weight + total_peer_bytes_start); - } else { - Assert(orphanage->UsageByPeer(peer_id) == total_peer_bytes_start); - } + // If announcement was added, total bytes does not increase. + // However, if eviction was triggered, the value may decrease. + Assert(orphanage->TotalOrphanUsage() <= total_bytes_start); } }, [&] { @@ -182,15 +174,11 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) { auto bytes_from_peer_before{orphanage->UsageByPeer(peer_id)}; Assert(have_tx == orphanage->EraseTx(tx->GetWitnessHash())); + // After EraseTx, the orphanage may trim itself, so all peers' usage may have gone up or down. if (have_tx) { - Assert(orphanage->TotalOrphanUsage() == total_bytes_start - tx_weight); - if (have_tx_and_peer) { - Assert(orphanage->UsageByPeer(peer_id) == bytes_from_peer_before - tx_weight); - } else { + if (!have_tx_and_peer) { Assert(orphanage->UsageByPeer(peer_id) == bytes_from_peer_before); } - } else { - Assert(orphanage->TotalOrphanUsage() == total_bytes_start); } } have_tx = orphanage->HaveTx(tx->GetWitnessHash()); @@ -218,13 +206,8 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage) Assert(!orphanage->HaveTx(tx_removed->GetWitnessHash())); Assert(!orphanage->HaveTxFromPeer(tx_removed->GetWitnessHash(), peer_id)); } - }, - [&] { - // test mocktime and expiry - SetMockTime(ConsumeTime(fuzzed_data_provider)); - orphanage->LimitOrphans(); - }); - + } + ); } // Set tx as potential parent to be used for future GetChildren() calls. @@ -345,7 +328,7 @@ FUZZ_TARGET(txorphan_protected, .init = initialize_orphanage) { if (peer_is_protected && !have_tx_and_peer && (orphanage->UsageByPeer(peer_id) + tx_weight > honest_mem_limit || - orphanage->LatencyScoreFromPeer(peer_id) + (tx->vin.size()) + 1 > honest_latency_limit)) { + orphanage->LatencyScoreFromPeer(peer_id) + (tx->vin.size() / 10) + 1 > honest_latency_limit)) { // We never want our protected peer oversized } else { orphanage->AddAnnouncer(tx->GetWitnessHash(), peer_id); @@ -369,33 +352,8 @@ FUZZ_TARGET(txorphan_protected, .init = initialize_orphanage) Assert(orphanage->LatencyScoreFromPeer(peer_id) == 0); Assert(orphanage->AnnouncementsFromPeer(peer_id) == 0); } - }, - [&] { // LimitOrphans - // Assert that protected peers are never affected by LimitOrphans. - unsigned int protected_count = 0; - unsigned int protected_bytes = 0; - for (unsigned int peer = 0; peer < num_peers; ++peer) { - if (protected_peers[peer]) { - protected_count += orphanage->LatencyScoreFromPeer(peer); - protected_bytes += orphanage->UsageByPeer(peer); - } - } - orphanage->LimitOrphans(); - Assert(orphanage->TotalLatencyScore() <= global_latency_score_limit); - Assert(orphanage->TotalOrphanUsage() <= per_peer_weight_reservation * num_peers); - - // Number of announcements and usage should never differ before and after since - // we've never exceeded the per-peer reservations. - for (unsigned int peer = 0; peer < num_peers; ++peer) { - if (protected_peers[peer]) { - protected_count -= orphanage->LatencyScoreFromPeer(peer); - protected_bytes -= orphanage->UsageByPeer(peer); - } - } - Assert(protected_count == 0); - Assert(protected_bytes == 0); - }); - + } + ); } } @@ -409,7 +367,7 @@ FUZZ_TARGET(txorphan_protected, .init = initialize_orphanage) FUZZ_TARGET(txorphanage_sim) { SeedRandomStateForTest(SeedRand::ZEROS); - // This is a comphehensive simulation fuzz test, which runs through a scenario involving up to + // This is a comprehensive simulation fuzz test, which runs through a scenario involving up to // 16 transactions (which may have simple or complex topology, and may have duplicate txids // with distinct wtxids, and up to 16 peers. The scenario is performed both on a real // TxOrphanage object and the behavior is compared with a naive reimplementation (just a vector @@ -512,9 +470,9 @@ FUZZ_TARGET(txorphanage_sim) // 3. Initialize real orphanage // - auto max_global_ann = provider.ConsumeIntegralInRange(NUM_PEERS, MAX_ANN); + auto max_global_latency_score = provider.ConsumeIntegralInRange(NUM_PEERS, MAX_ANN); auto reserved_peer_usage = provider.ConsumeIntegralInRange(1, total_usage); - auto real = node::MakeTxOrphanage(max_global_ann, reserved_peer_usage); + auto real = node::MakeTxOrphanage(max_global_latency_score, reserved_peer_usage); // // 4. Functions and data structures for the simulation. @@ -668,11 +626,11 @@ FUZZ_TARGET(txorphanage_sim) auto tx = read_tx_fn(); FastRandomContext rand_ctx(rng.rand256()); auto added = real->AddChildrenToWorkSet(*txn[tx], rand_ctx); - /** Map of all child wtxids, with value whether they already have a reconsiderable - announcement from some peer. */ - std::map child_wtxids; + /** Set of not-already-reconsiderable child wtxids. */ + std::set child_wtxids; for (unsigned child_tx = 0; child_tx < NUM_TX; ++child_tx) { if (!have_tx_fn(child_tx)) continue; + if (have_reconsiderable_fn(child_tx)) continue; bool child_of = false; for (auto& txin : txn[child_tx]->vin) { if (txin.prevout.hash == txn[tx]->GetHash()) { @@ -681,11 +639,11 @@ FUZZ_TARGET(txorphanage_sim) } } if (child_of) { - child_wtxids[txn[child_tx]->GetWitnessHash()] = have_reconsiderable_fn(child_tx); + child_wtxids.insert(txn[child_tx]->GetWitnessHash()); } } for (auto& [wtxid, peer] : added) { - // Wtxid must be a child of tx. + // Wtxid must be a child of tx that is not yet reconsiderable. auto child_wtxid_it = child_wtxids.find(wtxid); assert(child_wtxid_it != child_wtxids.end()); // Announcement must exist. @@ -695,18 +653,13 @@ FUZZ_TARGET(txorphanage_sim) assert(sim_ann_it->reconsider == false); // Make reconsiderable. sim_ann_it->reconsider = true; - } - for (auto& [wtxid, peer] : added) { - // Remove from child_wtxids map, so we can check that only already-reconsiderable - // ones are missing from the result. + // Remove from child_wtxids map, to disallow it being reported a second time in added. child_wtxids.erase(wtxid); } // Verify that AddChildrenToWorkSet does not select announcements that were already reconsiderable: // Check all child wtxids which did not occur at least once in the result were already reconsiderable // due to a previous AddChildrenToWorkSet. - for (auto& [wtxid, already_reconsider] : child_wtxids) { - assert(already_reconsider); - } + assert(child_wtxids.empty()); break; } else if (command-- == 0) { // GetTxToReconsider. @@ -727,59 +680,56 @@ FUZZ_TARGET(txorphanage_sim) assert(!have_reconsider_fn(peer)); } break; - } else if (command-- == 0) { - // LimitOrphans - const auto max_ann = max_global_ann / std::max(1, count_peers_fn()); - const auto max_mem = reserved_peer_usage; - while (true) { - // Count global usage and number of peers. - node::TxOrphanage::Usage total_usage{0}; - node::TxOrphanage::Count total_latency_score = sim_announcements.size(); - for (unsigned tx = 0; tx < NUM_TX; ++tx) { - if (have_tx_fn(tx)) { - total_usage += GetTransactionWeight(*txn[tx]); - total_latency_score += txn[tx]->vin.size() / 10; - } - } - auto num_peers = count_peers_fn(); - bool oversized = (total_usage > reserved_peer_usage * num_peers) || - (total_latency_score > real->MaxGlobalLatencyScore()); - if (!oversized) break; - // Find worst peer. - FeeFrac worst_dos_score{0, 1}; - unsigned worst_peer = unsigned(-1); - for (unsigned peer = 0; peer < NUM_PEERS; ++peer) { - auto dos_score = dos_score_fn(peer, max_ann, max_mem); - // Use >= so that the more recent peer (higher NodeId) wins in case of - // ties. - if (dos_score >= worst_dos_score) { - worst_dos_score = dos_score; - worst_peer = peer; - } - } - assert(worst_peer != unsigned(-1)); - assert(worst_dos_score >> FeeFrac(1, 1)); - // Find oldest announcement from worst_peer, preferring non-reconsiderable ones. - bool done{false}; - for (int reconsider = 0; reconsider < 2; ++reconsider) { - for (auto it = sim_announcements.begin(); it != sim_announcements.end(); ++it) { - if (it->announcer != worst_peer || it->reconsider != reconsider) continue; - sim_announcements.erase(it); - done = true; - break; - } - if (done) break; - } - assert(done); - } - real->LimitOrphans(); - // We must now be within limits, otherwise LimitOrphans should have continued further). - // We don't check the contents of the orphanage until the end to make fuzz runs faster. - assert(real->TotalLatencyScore() <= real->MaxGlobalLatencyScore()); - assert(real->TotalOrphanUsage() <= real->MaxGlobalUsage()); - break; } } + // Always trim after each command if needed. + const auto max_ann = max_global_latency_score / std::max(1, count_peers_fn()); + const auto max_mem = reserved_peer_usage; + while (true) { + // Count global usage and number of peers. + node::TxOrphanage::Usage total_usage{0}; + node::TxOrphanage::Count total_latency_score = sim_announcements.size(); + for (unsigned tx = 0; tx < NUM_TX; ++tx) { + if (have_tx_fn(tx)) { + total_usage += GetTransactionWeight(*txn[tx]); + total_latency_score += txn[tx]->vin.size() / 10; + } + } + auto num_peers = count_peers_fn(); + bool oversized = (total_usage > reserved_peer_usage * num_peers) || + (total_latency_score > real->MaxGlobalLatencyScore()); + if (!oversized) break; + // Find worst peer. + FeeFrac worst_dos_score{0, 1}; + unsigned worst_peer = unsigned(-1); + for (unsigned peer = 0; peer < NUM_PEERS; ++peer) { + auto dos_score = dos_score_fn(peer, max_ann, max_mem); + // Use >= so that the more recent peer (higher NodeId) wins in case of + // ties. + if (dos_score >= worst_dos_score) { + worst_dos_score = dos_score; + worst_peer = peer; + } + } + assert(worst_peer != unsigned(-1)); + assert(worst_dos_score >> FeeFrac(1, 1)); + // Find oldest announcement from worst_peer, preferring non-reconsiderable ones. + bool done{false}; + for (int reconsider = 0; reconsider < 2; ++reconsider) { + for (auto it = sim_announcements.begin(); it != sim_announcements.end(); ++it) { + if (it->announcer != worst_peer || it->reconsider != reconsider) continue; + sim_announcements.erase(it); + done = true; + break; + } + if (done) break; + } + assert(done); + } + // We must now be within limits, otherwise LimitOrphans should have continued further. + // We don't check the contents of the orphanage until the end to make fuzz runs faster. + assert(real->TotalLatencyScore() <= real->MaxGlobalLatencyScore()); + assert(real->TotalOrphanUsage() <= real->MaxGlobalUsage()); } // @@ -863,12 +813,12 @@ FUZZ_TARGET(txorphanage_sim) // CountUniqueOrphans assert(unique_orphans == real->CountUniqueOrphans()); // MaxGlobalLatencyScore - assert(max_global_ann == real->MaxGlobalLatencyScore()); + assert(max_global_latency_score == real->MaxGlobalLatencyScore()); // ReservedPeerUsage assert(reserved_peer_usage == real->ReservedPeerUsage()); // MaxPeerLatencyScore auto present_peers = count_peers_fn(); - assert(max_global_ann / std::max(1, present_peers) == real->MaxPeerLatencyScore()); + assert(max_global_latency_score / std::max(1, present_peers) == real->MaxPeerLatencyScore()); // MaxGlobalUsage assert(reserved_peer_usage * std::max(1, present_peers) == real->MaxGlobalUsage()); // TotalLatencyScore. diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp index d100f8bbee3..17ddc71a207 100644 --- a/src/test/orphanage_tests.cpp +++ b/src/test/orphanage_tests.cpp @@ -72,15 +72,6 @@ static bool EqualTxns(const std::set& set_txns, const std::vect return true; } -unsigned int CheckNumEvictions(node::TxOrphanage& orphanage) -{ - const auto original_total_count{orphanage.CountAnnouncements()}; - orphanage.LimitOrphans(); - assert(orphanage.TotalLatencyScore() <= orphanage.MaxGlobalLatencyScore()); - assert(orphanage.TotalOrphanUsage() <= orphanage.MaxGlobalUsage()); - return original_total_count - orphanage.CountAnnouncements(); -} - BOOST_AUTO_TEST_CASE(peer_dos_limits) { FastRandomContext det_rand{true}; @@ -103,11 +94,8 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) { // Test announcement limits NodeId peer{8}; - auto orphanage_low_ann = node::MakeTxOrphanage(/*max_global_ann=*/1, /*reserved_peer_usage=*/TX_SIZE * 10); - auto orphanage_low_mem = node::MakeTxOrphanage(/*max_global_ann=*/10, /*reserved_peer_usage=*/TX_SIZE); - - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage_low_mem), 0); - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage_low_ann), 0); + auto orphanage_low_ann = node::MakeTxOrphanage(/*max_global_latency_score=*/1, /*reserved_peer_usage=*/TX_SIZE * 10); + auto orphanage_low_mem = node::MakeTxOrphanage(/*max_global_latency_score=*/10, /*reserved_peer_usage=*/TX_SIZE); // Add the first transaction orphanage_low_ann->AddTx(txns.at(0), peer); @@ -115,30 +103,23 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) // Add more. One of the limits is exceeded, so LimitOrphans evicts 1. orphanage_low_ann->AddTx(txns.at(1), peer); - BOOST_CHECK(orphanage_low_ann->TotalLatencyScore() > orphanage_low_ann->MaxGlobalLatencyScore()); - BOOST_CHECK(orphanage_low_ann->TotalOrphanUsage() <= orphanage_low_ann->MaxGlobalUsage()); - orphanage_low_mem->AddTx(txns.at(1), peer); - BOOST_CHECK(orphanage_low_mem->TotalLatencyScore() <= orphanage_low_mem->MaxGlobalLatencyScore()); - BOOST_CHECK(orphanage_low_mem->TotalOrphanUsage() > orphanage_low_mem->MaxGlobalUsage()); - - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage_low_mem), 1); - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage_low_ann), 1); // The older transaction is evicted. BOOST_CHECK(!orphanage_low_ann->HaveTx(txns.at(0)->GetWitnessHash())); BOOST_CHECK(!orphanage_low_mem->HaveTx(txns.at(0)->GetWitnessHash())); BOOST_CHECK(orphanage_low_ann->HaveTx(txns.at(1)->GetWitnessHash())); BOOST_CHECK(orphanage_low_mem->HaveTx(txns.at(1)->GetWitnessHash())); + + orphanage_low_ann->SanityCheck(); + orphanage_low_mem->SanityCheck(); } // Single peer: latency score includes inputs { // Test latency score limits NodeId peer{10}; - auto orphanage_low_ann = node::MakeTxOrphanage(/*max_global_ann=*/5, /*reserved_peer_usage=*/TX_SIZE * 1000); - - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage_low_ann), 0); + auto orphanage_low_ann = node::MakeTxOrphanage(/*max_global_latency_score=*/5, /*reserved_peer_usage=*/TX_SIZE * 1000); // Add the first transaction orphanage_low_ann->AddTx(txns.at(0), peer); @@ -151,14 +132,11 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) auto ptx = MakeTransactionSpending(outpoints_45, det_rand); orphanage_low_ann->AddTx(ptx, peer); - BOOST_CHECK(orphanage_low_ann->TotalLatencyScore() > orphanage_low_ann->MaxGlobalLatencyScore()); - BOOST_CHECK(orphanage_low_ann->LatencyScoreFromPeer(peer) > orphanage_low_ann->MaxPeerLatencyScore()); - - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage_low_ann), 1); - // The older transaction is evicted. BOOST_CHECK(!orphanage_low_ann->HaveTx(txns.at(0)->GetWitnessHash())); BOOST_CHECK(orphanage_low_ann->HaveTx(ptx->GetWitnessHash())); + + orphanage_low_ann->SanityCheck(); } // Single peer: eviction order is FIFO on non-reconsiderable, then reconsiderable orphans. @@ -175,7 +153,7 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) // Test announcement limits NodeId peer{9}; - auto orphanage = node::MakeTxOrphanage(/*max_global_ann=*/3, /*reserved_peer_usage=*/TX_SIZE * 10); + auto orphanage = node::MakeTxOrphanage(/*max_global_latency_score=*/3, /*reserved_peer_usage=*/TX_SIZE * 10); // First add a tx which will be made reconsiderable. orphanage->AddTx(children.at(0), peer); @@ -191,15 +169,14 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) // Add 1 more orphan, causing the orphanage to be oversize. child1 is evicted. orphanage->AddTx(children.at(3), peer); - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage), 1); BOOST_CHECK(orphanage->HaveTx(children.at(0)->GetWitnessHash())); BOOST_CHECK(!orphanage->HaveTx(children.at(1)->GetWitnessHash())); BOOST_CHECK(orphanage->HaveTx(children.at(2)->GetWitnessHash())); BOOST_CHECK(orphanage->HaveTx(children.at(3)->GetWitnessHash())); + orphanage->SanityCheck(); // Add 1 more... child2 is evicted. orphanage->AddTx(children.at(4), peer); - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage), 1); BOOST_CHECK(orphanage->HaveTx(children.at(0)->GetWitnessHash())); BOOST_CHECK(!orphanage->HaveTx(children.at(2)->GetWitnessHash())); BOOST_CHECK(orphanage->HaveTx(children.at(3)->GetWitnessHash())); @@ -213,7 +190,6 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) // child5 is evicted immediately because it is the only non-reconsiderable orphan. orphanage->AddTx(children.at(5), peer); - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage), 1); BOOST_CHECK(orphanage->HaveTx(children.at(0)->GetWitnessHash())); BOOST_CHECK(orphanage->HaveTx(children.at(3)->GetWitnessHash())); BOOST_CHECK(orphanage->HaveTx(children.at(4)->GetWitnessHash())); @@ -222,7 +198,6 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) // Transactions are marked non-reconsiderable again when returned through GetTxToReconsider BOOST_CHECK_EQUAL(orphanage->GetTxToReconsider(peer), children.at(0)); orphanage->AddTx(children.at(6), peer); - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage), 1); BOOST_CHECK(!orphanage->HaveTx(children.at(0)->GetWitnessHash())); BOOST_CHECK(orphanage->HaveTx(children.at(3)->GetWitnessHash())); BOOST_CHECK(orphanage->HaveTx(children.at(4)->GetWitnessHash())); @@ -232,6 +207,8 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) // reconsideration earlier. BOOST_CHECK_EQUAL(orphanage->GetTxToReconsider(peer), children.at(3)); BOOST_CHECK_EQUAL(orphanage->GetTxToReconsider(peer), children.at(4)); + + orphanage->SanityCheck(); } // Multiple peers: when limit is exceeded, we choose the DoSiest peer and evict their oldest transaction. @@ -247,8 +224,8 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) // No evictions happen before the global limit is reached. for (unsigned int i{0}; i < max_announcements; ++i) { orphanage->AddTx(txns.at(i), peer_dosy); - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage), 0); } + orphanage->SanityCheck(); BOOST_CHECK_EQUAL(orphanage->AnnouncementsFromPeer(peer_dosy), max_announcements); BOOST_CHECK_EQUAL(orphanage->AnnouncementsFromPeer(peer1), 0); BOOST_CHECK_EQUAL(orphanage->AnnouncementsFromPeer(peer2), 0); @@ -260,7 +237,6 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) orphanage->AddTx(txns.at(max_announcements + i), peer1); // The announcement limit per peer has halved, but LimitOrphans does not evict beyond what is necessary to // bring the total announcements within its global limit. - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage), 1); BOOST_CHECK(orphanage->AnnouncementsFromPeer(peer_dosy) > orphanage->MaxPeerLatencyScore()); BOOST_CHECK_EQUAL(orphanage->AnnouncementsFromPeer(peer1), i + 1); @@ -276,9 +252,6 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) BOOST_CHECK(orphanage->HaveTxFromPeer(txns.at(i)->GetWitnessHash(), peer_dosy)); orphanage->AddTx(txns.at(i), peer2); - // Announcement limit is by entry, not by unique orphans - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage), 1); - // peer_dosy is still the only one getting evicted BOOST_CHECK_EQUAL(orphanage->AnnouncementsFromPeer(peer_dosy), max_announcements - i - 1); BOOST_CHECK_EQUAL(orphanage->AnnouncementsFromPeer(peer1), num_from_peer1); @@ -291,15 +264,18 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) // With 6 peers, each can add 10, and still only peer_dosy's orphans are evicted. const unsigned int max_per_peer{max_announcements / 6}; + const unsigned int num_announcements{orphanage->CountAnnouncements()}; for (NodeId peer{3}; peer < 6; ++peer) { for (unsigned int i{0}; i < max_per_peer; ++i) { + // Each addition causes 1 eviction. orphanage->AddTx(txns.at(peer * max_per_peer + i), peer); - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage), 1); + BOOST_CHECK_EQUAL(orphanage->CountAnnouncements(), num_announcements); } } for (NodeId peer{0}; peer < 6; ++peer) { BOOST_CHECK_EQUAL(orphanage->AnnouncementsFromPeer(peer), max_per_peer); } + orphanage->SanityCheck(); } // Limits change as more peers are added. @@ -355,6 +331,8 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) BOOST_CHECK_EQUAL(orphanage->ReservedPeerUsage(), node::DEFAULT_RESERVED_ORPHAN_WEIGHT_PER_PEER); BOOST_CHECK_EQUAL(orphanage->MaxGlobalUsage(), node::DEFAULT_RESERVED_ORPHAN_WEIGHT_PER_PEER); BOOST_CHECK_EQUAL(orphanage->MaxPeerLatencyScore(), node::DEFAULT_MAX_ORPHANAGE_LATENCY_SCORE); + + orphanage->SanityCheck(); } // Test eviction of multiple transactions at a time @@ -378,17 +356,17 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) } BOOST_CHECK(orphanage->TotalLatencyScore() <= orphanage->MaxGlobalLatencyScore()); BOOST_CHECK(orphanage->TotalOrphanUsage() <= orphanage->MaxGlobalUsage()); - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage), 0); // Add the large transaction. This should cause evictions of all the previous 10 transactions from that peer. orphanage->AddTx(ptx_large, peer_large); - BOOST_CHECK_EQUAL(CheckNumEvictions(*orphanage), 10); // peer_normal should still have 10 transactions, and peer_large should have 1. BOOST_CHECK_EQUAL(orphanage->AnnouncementsFromPeer(peer_normal), 10); BOOST_CHECK_EQUAL(orphanage->AnnouncementsFromPeer(peer_large), 1); BOOST_CHECK(orphanage->HaveTxFromPeer(ptx_large->GetWitnessHash(), peer_large)); BOOST_CHECK_EQUAL(orphanage->CountAnnouncements(), 11); + + orphanage->SanityCheck(); } // Test that latency score includes number of inputs. @@ -432,6 +410,8 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits) // Peer 1 sent 5 of the 10 transactions with many inputs BOOST_CHECK_EQUAL(orphanage->AnnouncementsFromPeer(1), 5); BOOST_CHECK_EQUAL(orphanage->LatencyScoreFromPeer(1), 5 + 5 * 5); + + orphanage->SanityCheck(); } } BOOST_AUTO_TEST_CASE(DoS_mapOrphans) @@ -518,11 +498,11 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) BOOST_CHECK(!orphanage->AddTx(MakeTransactionRef(tx), i)); } - size_t expected_num_orphans = orphanage->Size(); + size_t expected_num_orphans = orphanage->CountUniqueOrphans(); // Non-existent peer; nothing should be deleted orphanage->EraseForPeer(/*peer=*/-1); - BOOST_CHECK_EQUAL(orphanage->Size(), expected_num_orphans); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), expected_num_orphans); // Each of first three peers stored // two transactions each. @@ -530,7 +510,7 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) { orphanage->EraseForPeer(i); expected_num_orphans -= 2; - BOOST_CHECK(orphanage->Size() == expected_num_orphans); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), expected_num_orphans); } } @@ -752,7 +732,7 @@ BOOST_AUTO_TEST_CASE(process_block) BOOST_CHECK(!orphanage->HaveTx(expected_removed_wtxid)); } // Only remaining tx is control_tx - BOOST_CHECK_EQUAL(orphanage->Size(), 1); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), 1); BOOST_CHECK(orphanage->HaveTx(control_tx->GetWitnessHash())); } @@ -773,11 +753,11 @@ BOOST_AUTO_TEST_CASE(multiple_announcers) BOOST_CHECK(orphanage->AddTx(ptx, node0)); BOOST_CHECK(orphanage->HaveTx(wtxid)); expected_total_count += 1; - BOOST_CHECK_EQUAL(orphanage->Size(), expected_total_count); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), expected_total_count); // Adding again should do nothing. BOOST_CHECK(!orphanage->AddTx(ptx, node0)); - BOOST_CHECK_EQUAL(orphanage->Size(), expected_total_count); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), expected_total_count); // We can add another tx with the same txid but different witness. auto ptx_mutated{MakeMutation(ptx)}; @@ -789,7 +769,7 @@ BOOST_AUTO_TEST_CASE(multiple_announcers) // Adding a new announcer should not change overall accounting. BOOST_CHECK(orphanage->AddAnnouncer(ptx->GetWitnessHash(), node2)); - BOOST_CHECK_EQUAL(orphanage->Size(), expected_total_count); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), expected_total_count); // If we already have this announcer, AddAnnouncer returns false. BOOST_CHECK(orphanage->HaveTxFromPeer(ptx->GetWitnessHash(), node2)); @@ -797,7 +777,7 @@ BOOST_AUTO_TEST_CASE(multiple_announcers) // Same with using AddTx for an existing tx, which is equivalent to using AddAnnouncer BOOST_CHECK(!orphanage->AddTx(ptx, node1)); - BOOST_CHECK_EQUAL(orphanage->Size(), expected_total_count); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), expected_total_count); // if EraseForPeer is called for an orphan with multiple announcers, the orphanage should only // erase that peer from the announcers set. @@ -807,15 +787,15 @@ BOOST_AUTO_TEST_CASE(multiple_announcers) // node0 is the only one that announced ptx_mutated BOOST_CHECK(!orphanage->HaveTx(ptx_mutated->GetWitnessHash())); expected_total_count -= 1; - BOOST_CHECK_EQUAL(orphanage->Size(), expected_total_count); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), expected_total_count); // EraseForPeer should delete the orphan if it's the only announcer left. orphanage->EraseForPeer(node1); - BOOST_CHECK_EQUAL(orphanage->Size(), expected_total_count); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), expected_total_count); BOOST_CHECK(orphanage->HaveTx(ptx->GetWitnessHash())); orphanage->EraseForPeer(node2); expected_total_count -= 1; - BOOST_CHECK_EQUAL(orphanage->Size(), expected_total_count); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), expected_total_count); BOOST_CHECK(!orphanage->HaveTx(ptx->GetWitnessHash())); } @@ -829,13 +809,13 @@ BOOST_AUTO_TEST_CASE(multiple_announcers) expected_total_count += 1; - BOOST_CHECK_EQUAL(orphanage->Size(), expected_total_count); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), expected_total_count); orphanage->EraseForBlock(block); expected_total_count -= 1; - BOOST_CHECK_EQUAL(orphanage->Size(), expected_total_count); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), expected_total_count); } } BOOST_AUTO_TEST_CASE(peer_worksets) @@ -883,7 +863,7 @@ BOOST_AUTO_TEST_CASE(peer_worksets) // Delete this tx, clearing the orphanage. BOOST_CHECK_EQUAL(orphanage->EraseTx(orphan_wtxid), 1); - BOOST_CHECK_EQUAL(orphanage->Size(), 0); + BOOST_CHECK_EQUAL(orphanage->CountUniqueOrphans(), 0); for (NodeId node = node0; node <= node2; ++node) { BOOST_CHECK_EQUAL(orphanage->GetTxToReconsider(node), nullptr); BOOST_CHECK(!orphanage->HaveTxFromPeer(orphan_wtxid, node)); diff --git a/test/functional/p2p_orphan_handling.py b/test/functional/p2p_orphan_handling.py index ddaa8ac0ac9..f2eed48603c 100755 --- a/test/functional/p2p_orphan_handling.py +++ b/test/functional/p2p_orphan_handling.py @@ -830,6 +830,16 @@ class OrphanHandlingTest(BitcoinTestFramework): assert orphan["txid"] in final_mempool assert tx_replacer_C["txid"] in final_mempool + @cleanup + def test_maxorphantx_option(self): + # This test should be removed when -maxorphantx is removed. + self.log.info("Test that setting the -maxorphantx option does not error") + warning = "Warning: Option '-maxorphantx' is set but no longer has any effect (see release notes). Please remove it from your configuration." + self.restart_node(0, extra_args=["-maxorphantx=5"]) + assert_equal(self.nodes[0].getorphantxs(), []) + self.stop_node(0, expected_stderr=warning) + self.restart_node(0) + def run_test(self): self.nodes[0].setmocktime(int(time.time())) self.wallet_nonsegwit = MiniWallet(self.nodes[0], mode=MiniWalletMode.RAW_P2PK) @@ -850,6 +860,7 @@ class OrphanHandlingTest(BitcoinTestFramework): self.test_announcers_before_and_after() self.test_parents_change() self.test_maximal_package_protected() + self.test_maxorphantx_option() if __name__ == '__main__':