p2p: avoid orphanage abort at high peer counts #35919

It's worthing changing the fuzz targets to reach the zero-share path. See:

diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp
index 9466f77624..90f9fe4fa0 100644
--- a/src/test/fuzz/txorphan.cpp
+++ b/src/test/fuzz/txorphan.cpp
@@ -471,7 +471,7 @@ FUZZ_TARGET(txorphanage_sim)
     // 3. Initialize real orphanage
     //
 
-    auto max_global_latency_score = provider.ConsumeIntegralInRange<node::TxOrphanage::Count>(NUM_PEERS, MAX_ANN);
+    auto max_global_latency_score = provider.ConsumeIntegralInRange<node::TxOrphanage::Count>(1, MAX_ANN);
     auto reserved_peer_usage = provider.ConsumeIntegralInRange<node::TxOrphanage::Usage>(1, total_usage);
     auto real = node::MakeTxOrphanage(max_global_latency_score, reserved_peer_usage);
 
@@ -684,8 +684,8 @@ FUZZ_TARGET(txorphanage_sim)
             }
         }
         // Always trim after each command if needed.
-        const auto max_ann = max_global_latency_score / std::max<unsigned>(1, count_peers_fn());
-        const auto max_mem = reserved_peer_usage;
+        const auto max_ann = std::max<unsigned>(1, max_global_latency_score / std::max<unsigned>(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};
@@ -713,7 +713,7 @@ FUZZ_TARGET(txorphanage_sim)
                 }
             }
             assert(worst_peer != unsigned(-1));
-            assert(ByRatio{worst_dos_score} > ByRatio{FeeFrac(1, 1)});
+            assert(ByRatio{worst_dos_score} >= ByRatio{FeeFrac(1, 1)});
             // Find oldest announcement from worst_peer, preferring non-reconsiderable ones.
             bool done{false};
             for (int reconsider = 0; reconsider < 2; ++reconsider) {

Thanks, widened the simulation's limits and matched its trimming model to the minimum share of one, added you as coauthor.

nit: extra whitespace here

It's not extra, in the next commit it're replaced by a !, this way the diff is simpler :)

nit: (Found with Fable 5): every other block in the test ends with a orphanage->SanityCheck();, you should add it there too. It will initially fail, because the orphanage is not properly trimmed.

In the first commit:

diff --git a/src/node/txorphanage.cpp b/src/node/txorphanage.cpp
index 58880139b3..3285cf51f5 100644
--- a/src/node/txorphanage.cpp
+++ b/src/node/txorphanage.cpp
@@ -765,7 +765,7 @@ void TxOrphanageImpl::SanityCheck() const
         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());
+    Assert(!NeedsTrim());
 }
 
 TxOrphanage::Count TxOrphanageImpl::MaxGlobalLatencyScore() const { return m_max_global_latency_score; }
diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp
index 8a2c2b81ec..07fec603a8 100644
--- a/src/test/orphanage_tests.cpp
+++ b/src/test/orphanage_tests.cpp
@@ -352,6 +352,8 @@ BOOST_AUTO_TEST_CASE(peer_dos_limits)
 
         BOOST_CHECK_GT(orphanage->TotalLatencyScore(), global_limit); // TODO: Trim back to the global limit.
         BOOST_CHECK_GT(orphanage->CountAnnouncements(), global_limit); // TODO: Trim back to the global limit.
+
+        BOOST_CHECK_EXCEPTION(orphanage->SanityCheck(), NonFatalCheckError, HasReason{"!NeedsTrim()"}); // TODO: should pass sanity checks
     }
 
     // Test eviction of multiple transactions at a time

Then you change it to orphanage->SanityCheck(); in the second one.

Thanks, added the sanity check after trimming back to the limit (was originally skipped since the characterization test couldn't include it yet).

Might be worth it to clarify why it might be equal? Something like:

Note that the per-peer latency limit is floored at 1, so if there are more peers with orphans than the global latency limit, the global limit is exceeded even if every peer's DoS score is exactly 1.

Added a whole new commit with doc adjustments, thanks!

(comment placed in random location), in src/node/txorphanage.h, we say: https://github.com/bitcoin/bitcoin/blob/51ddab532cb38213e2258c24c492bc8a392ffc90/src/node/txorphanage.h#L35

"as long as they don't exceed their limits" is no longer accurate: a peer exactly at his limit isn't protected anymore if the number of peers holding orphans is higher than the global limit. You could rephrase this as "as long as they stay strictly below their limits"

Good point, fixed, thanks!