Merge bitcoin/bitcoin#32941: p2p: TxOrphanage revamp cleanups

c0642e5 [fuzz] fix latency score check in txorphan_protected (glozow)
3d4d4f0 scripted-diff: rename "ann" variables to "latency_score" (monlovesmango)
3b92448 [doc] comment fixups for orphanage changes (glozow)
1384dba [config] emit warning for -maxorphantx, but allow it to be set (glozow)
b10c55b fix up TxOrphanage lower_bound sanity checks (glozow)
cfd71c6 scripted-diff: rename TxOrphanage outpoints index (glozow)
edb97bb [logging] add logs for inner loop of LimitOrphans (glozow)
8a58d0e scripted-diff: rename OrphanTxBase to OrphanInfo (glozow)
cc50f2f [cleanup] replace TxOrphanage::Size() with CountUniqueOrphans (glozow)
ed24e01 [optimization] Maintain at most 1 reconsiderable announcement per wtxid (Pieter Wuille)
af7402c [refactor] make TxOrphanage keep itself trimmed (glozow)
d1fac25 [doc] 31829 release note (glozow)

Pull request description:

  Followup to #31829:
  - Release notes
  - Have the orphanage auto-trim itself whenever necessary (and test changes) bitcoin/bitcoin#31829 (comment)
  - Reduce duplicate reconsiderations by keeping track of which txns are already reconsiderable so we only mark it for reconsideration for 1 peer at a time bitcoin/bitcoin#31829 (comment)
  - Rename `OrphanTxBase` to `OrphanInfo`
  - Get rid of `Size()` method by replacing all calls with `CountUniqueOrphans`
  - Rename outpoints index since they point to wtxids, not iterators bitcoin/bitcoin#31829 (comment)
  - Add more logging in the `LimitOrphans` inner loop to make it easy to see which peers are being trimmed bitcoin/bitcoin#31829 (comment)

ACKs for top commit:
  sipa:
    utACK c0642e5
  marcofleon:
    Nice, ACK c0642e5

Tree-SHA512: f298eae92cf906ed5e4f15a24eeffa7b9e620bcff457772cd77522dd9f0b3b183ffc976871b1b0e6fe93009e64877d518e53d4b9e186e0df58fc16d17f6de90a

Author: fanquake

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.

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};

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=<file>", "Imports blocks from external file on startup", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-maxmempool=<n>", strprintf("Keep the transaction memory pool below <n> megabytes (default: %u)", DEFAULT_MAX_MEMPOOL_SIZE_MB), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
+    // TODO: remove in v31.0
+    argsman.AddArg("-maxorphantx=<n>", strprintf("(Removed option, see release notes)"), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-mempoolexpiry=<n>", strprintf("Do not keep transactions in the mempool longer than <n> hours (default: %u)", DEFAULT_MEMPOOL_EXPIRY_HOURS), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
     argsman.AddArg("-minimumchainwork=<hex>", 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=<n>", 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.

src/net_processing.cpp

@@ -533,7 +533,7 @@ class PeerManagerImpl final : public PeerManager
     std::optional<std::string> 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<node::TxOrphanage::OrphanTxBase> GetOrphanTransactions() override EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex);
+    std::vector<node::TxOrphanage::OrphanInfo> 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<node::TxOrphanage::OrphanTxBase> PeerManagerImpl::GetOrphanTransactions()
+std::vector<node::TxOrphanage::OrphanInfo> PeerManagerImpl::GetOrphanTransactions()
 {
     LOCK(m_tx_download_mutex);
     return m_txdownloadman.GetOrphanTransactions();

src/net_processing.h

@@ -109,7 +109,7 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
     /** Get statistics from node state */
     virtual bool GetNodeStateStats(NodeId nodeid, CNodeStateStats& stats) const = 0;
 
-    virtual std::vector<node::TxOrphanage::OrphanTxBase> GetOrphanTransactions() = 0;
+    virtual std::vector<node::TxOrphanage::OrphanInfo> GetOrphanTransactions() = 0;
 
     /** Get peer manager info. */
     virtual PeerManagerInfo GetInfo() const = 0;

src/node/txdownloadman.h

@@ -170,7 +170,7 @@ class TxDownloadManager {
     void CheckIsEmpty(NodeId nodeid) const;
 
     /** Wrapper for TxOrphanage::GetOrphanTransactions */
-    std::vector<TxOrphanage::OrphanTxBase> GetOrphanTransactions() const;
+    std::vector<TxOrphanage::OrphanInfo> GetOrphanTransactions() const;
 };
 } // namespace node
 #endif // BITCOIN_NODE_TXDOWNLOADMAN_H

src/node/txdownloadman_impl.cpp

@@ -83,7 +83,7 @@ void TxDownloadManager::CheckIsEmpty(NodeId nodeid) const
 {
     m_impl->CheckIsEmpty(nodeid);
 }
-std::vector<TxOrphanage::OrphanTxBase> TxDownloadManager::GetOrphanTransactions() const
+std::vector<TxOrphanage::OrphanInfo> 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<TxOrphanage::OrphanTxBase> TxDownloadManagerImpl::GetOrphanTransactions() const
+std::vector<TxOrphanage::OrphanInfo> TxDownloadManagerImpl::GetOrphanTransactions() const
 {
     return m_orphanage->GetOrphanTransactions();
 }

src/node/txdownloadman_impl.h

@@ -188,7 +188,7 @@ class TxDownloadManagerImpl {
     void CheckIsEmpty();
     void CheckIsEmpty(NodeId nodeid);
 
-    std::vector<TxOrphanage::OrphanTxBase> GetOrphanTransactions() const;
+    std::vector<TxOrphanage::OrphanInfo> GetOrphanTransactions() const;
 
 protected:
     /** Helper for getting deduplicated vector of Txids in vin. */