Problem: The orphanage can abort when more than 3,000 peers have orphan announcements.
The -maxconnections setting currently has no explicit upper bound.
Fix: Cap -maxconnections at 2,000 with a warning before reserving file descriptors, while retaining the default of 200.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | instagibbs, optout21 |
| Stale ACK | jeanpablojp, brunoerg, danielabrozzoni, vasild |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
No conflicts as of last run.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
ACK 1ac1f10e7b0e06e8d01a35cabf5bef49ea43c69a
Reproduced the abort on master 128456b62d with three peers against a global limit of 2. The two halves of the fix depend on each other: with only the clamp and the old > filter, every peer sits at exactly its floored share, the heap comes out empty and the loop pops from it, which segfaults here. So >= is not cleanup. Full unit suite and p2p_orphan_handling.py pass on the merge.
nit: the comment on MaxPeerLatencyScore() (txorphanage.cpp:210) still says the number of peers times that value adds up to MaxGlobalLatencyScore(), and that keeping every peer below 1 keeps the global limit. LimitOrphans() now enforces a floor of 1 instead, so that stops matching what runs once there are more peers than slots.
nit: the new test passes whichever peer gets trimmed. BOOST_CHECK(!orphanage->HaveTxFromPeer(txns.at(2)->GetWitnessHash(), 2)) pins it, and passes as written.
nit: #include <util/check.h> in orphanage_tests.cpp is added by de6342cd45 and nothing uses it after 1ac1f10e7b. test_bitcoin builds with the line removed.
Thanks @jeanpablojp, rebased and took all three suggestions (code comment, include and new test) - added you as co-author for the first commit.
re-ACK 5f4429c20c5cae0eb59baa867e131cc939e92d5d
Thanks for the co-author credit.
446 | @@ -448,17 +447,17 @@ void TxOrphanageImpl::LimitOrphans() 447 | 448 | // Even though it's possible for MaxPeerLatencyScore to increase within this call to LimitOrphans 449 | // (e.g. if a peer's orphans are removed entirely, changing the number of peers), use consistent limits throughout. 450 | - const auto max_lat{MaxPeerLatencyScore()}; 451 | + const auto max_lat{std::max<TxOrphanage::Count>(MaxPeerLatencyScore(), 1)};
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.
code review ACK 5f4429c20c5cae0eb59baa867e131cc939e92d5d
346 | + for (auto peer{0U}; peer < global_limit; ++peer) orphanage->AddTx(txns.at(peer), peer); 347 | + 348 | + BOOST_CHECK_EQUAL(orphanage->MaxPeerLatencyScore(), 1); 349 | + test_only_CheckFailuresAreExceptionsNotAborts mock_checks{}; 350 | + BOOST_CHECK_EXCEPTION(orphanage->AddTx(txns.at(2), /*peer=*/2), NonFatalCheckError, HasReason{"max_peer_latency_score > 0"}); // TODO: A zero share must not abort trimming. 351 | + BOOST_CHECK( orphanage->HaveTxFromPeer(txns.at(2)->GetWitnessHash(), 2)); // TODO: Trim the newly added peer.
nit: extra whitespace here
It's not extra, in the next commit it're replaced by a !, this way the diff is simpler :)
347 | + BOOST_CHECK_EQUAL(orphanage->MaxPeerLatencyScore(), 1); 348 | + BOOST_CHECK(orphanage->AddTx(txns.at(2), /*peer=*/2)); 349 | + BOOST_CHECK(!orphanage->HaveTxFromPeer(txns.at(2)->GetWitnessHash(), 2)); 350 | + 351 | + BOOST_CHECK_EQUAL(orphanage->TotalLatencyScore(), global_limit); 352 | + BOOST_CHECK_EQUAL(orphanage->CountAnnouncements(), global_limit);
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).
158 | @@ -159,7 +159,7 @@ class TxOrphanageImpl final : public TxOrphanage { 159 | * A peer having a DoS score > 1 does not necessarily mean that something is wrong, since we 160 | * do not trim unless the orphanage exceeds global limits, but it means that this peer will 161 | * be selected for trimming sooner. If the global latency score or global memory usage 162 | - * limits are exceeded, it must be that there is a peer whose DoS score > 1. */ 163 | + * limits are exceeded, it must be that there is a peer whose DoS score >= 1. */
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!
6 | @@ -7,6 +7,7 @@
7 | #include <consensus/validation.h>
(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!
light ACK 5f4429c20c5cae0eb59baa867e131cc939e92d5d
I'm light ACKing because it's my first time looking into the orphanage. Code looks good, I left a couple of non-blocking ideas.
Rebased, adjusted the commit structure, the PR description and pushed a version incorporating the review feedback:
SanityCheck() after trimming.txorphanage_sim now exercises global latency scores below the peer count and models the minimum share and score-one eligibility.GetDosScore() resource preconditions now use Assert().Thanks @brunoerg and @danielabrozzoni for the suggestions.
Rebased on master and simplified this to cap -maxconnections at 2,000 with a warning instead of changing orphanage trimming - seems simpler, thanks @instagibbs.
concept ACK, we really shouldn't let users configure a number where a deterministic crash is possible
1059 | @@ -1060,10 +1060,14 @@ bool AppInitParameterInteraction(const ArgsManager& args) 1060 | // Number of bound interfaces (we have at least one) 1061 | int num_p2p_bind = std::max(num_user_p2p_bind, size_t(1)); 1062 | // Maximum number of connections with other nodes, this accounts for all types of outbounds and inbounds except for manual 1063 | - int user_p2p_max_connections = args.GetIntArg("-maxconnections", DEFAULT_MAX_PEER_CONNECTIONS); 1064 | + int user_p2p_max_connections = args.GetArg<int>("-maxconnections", DEFAULT_MAX_PEER_CONNECTIONS);
We could do that for consistency, but here I changed it to avoid premature 32 bit casts. Given that we're checking the bounds here, whoever requests the param after this should be able to use GetIntArg.
crACK ec01169832452a818dcf3f0539150e87d3358309 LGTM!
79 | @@ -80,6 +80,8 @@ inline constexpr size_t MAX_PRIVATE_BROADCAST_CONNECTIONS{64}; 80 | inline constexpr bool DEFAULT_LISTEN = true; 81 | /** The maximum number of peer connections to maintain. */ 82 | inline constexpr unsigned int DEFAULT_MAX_PEER_CONNECTIONS{200}; 83 | +/** Maximum value of -maxconnections */ 84 | +inline constexpr int MAX_PEER_CONNECTIONS{2'000};
do we want to document this more? e.g., somewhere have
static_assert(MAX_PEER_CONNECTIONS + MAX_ADDNODE_CONNECTIONS +
MAX_PRIVATE_BROADCAST_CONNECTIONS <
node::DEFAULT_MAX_ORPHANAGE_LATENCY_SCORE);
I thinks this would be worthwhile to add.
ACK ec01169832452a818dcf3f0539150e87d3358309
The residual risk is the client adding 1000 more connections manually, which seems vanishingly small
ACK ec01169832452a818dcf3f0539150e87d3358309
<details> <summary>Show Signature</summary>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK ec01169832452a818dcf3f0539150e87d3358309
-----BEGIN PGP SIGNATURE-----
iQRPBAEBCAA5FiEE5k2NRWFNsHVF2czBVN8G9ktVy78FAmrXbu4bFIAAAAAABAAO
bWFudTIsMi41KzEuMTIsMiwzAAoJEFTfBvZLVcu/wRUf/2JmvVFzOMX79oBujbIu
pbH1LsqgnfybhKbo8Qs6suNeD1o+Q1fEVfuNDL+j0fRR2ziXWgvZu/NHxdMqF6Mv
d+2bp9i7rVYdiI7Xk3vz6Ha1dzwnu3z6CWTpLGQP7Lr/ScYck+QcJXI/xdGMaVIT
JRuI7KanlgWRd94HMRxy38gBWtOxcypF1YNkVUzYyUGNq+HCwg3yQ+4Nkj/5HXKS
TQQYQIlEmyXCqG3ham7mvOO3iZPreO/kkQH9lo7aWXAm4HGLzehtmh0fwRIp7znT
uC1Mz/zt4iDFoO2IwhkPIU/dUoWtHh7KIvIJ98sKhp1Dw7p664Gb7jdqlHFQ/RW9
N5wOSPAuy7RUEcVtYZpAAW2bkcN9PjMnJedu/hUjZGq/QMR9k+Ouejey9hZmCSa0
EN2MBKj8q/Fav4KE6jATsPxUbIWzQniQpHS27YGE84F4h1ytpLQcgTN8bzyszKN9
Y2BC8LDKWUJYcNBolTo5AYWVLwduU6NlJ+ZvrS9KiOGtikEJVyDjtxLmaCmKvhWf
F+xebzgzeLLFS2cczd5noz9raob5J3Z/FYvC5KENoxlLqiYsJDy6tw0xvhQd201t
GtNrD0vWgjGOa4G1ApBQsBLW4+XCMOrOtzDfdoCLrI2MUS5W7oaOsBWbvTQ+6NFA
Uk9nyCcbc1t8aYqoPJ/NWRZiAmueub+8fAli1oQz19bUT9xZMJHMrui/6Rx02Hfs
Fcak3BUJ5mwsvqUb0w+2SO5Mk7BXzft/Xawd2TiJlT6ovyaUpwUXyHPkAWkUt10y
Qd0UXrQTrO3xN3kINXiDW2NvTkgGqbwtfX3zkJVNa/sxe9NWmFyx5hmqIZcg2ll2
R2S6j1LY/k1FtO1y8scmDk2rMhT0oU+u/j64BgRfZJjTkziCbs3JvAX7P5eNO26n
nOMhWMvjjB/HqI3U5Izx6bb/dkiXA694IRIxzf3mo9CakWIFxqAPnMBk2vc3ATzY
rm1lM3eC6vmR0E10Db34BcSgvYHUozNQ4X8KPVmaT3I92EN2Vg6o2piGCzYYFHpx
QQZE0AlHjfOhwN2Qw6QQA5E2FF3mZcUtfkUcKa6F56ps/KuqSn/NB9FyEM/sOwBG
X8CV1jXevAM7AQrMjlzD9QwRiSA3k3b0gpefNvRl9psYMKUwOm29BrHGWNxfiCR+
lbhvt+L9FIgmCgnm4CbVfzka/uk0td0y4EL0vsvKXcpAQS0bJ3NBhUOZ08KgTEhN
ksCbJkIS1+aDKZ/2pfl+RtXlQFoanrDtpp5jGhQ3CLAKK9NebFZqNJ5N7TUGXDTD
r86Lx99WPHLp84WOgNmHPcGJ32n4I6UslJ2y0AHnCfZrPGfd3C8LSIw6PHlV/IsG
fhs=
=YbB3
-----END PGP SIGNATURE-----
vasild's public key is on openpgp.org
</details>
Limit the configurable automatic peer count to 2000, leaving headroom below the default orphanage latency limit.
Apply the cap before reserving file descriptors. Use saturating `int` parsing so oversized arguments cannot wrap before the cap is applied.
Thanks for the reviews, added a static_assert to explain why capping was necessary.
reACK b30f750de56c4e510f8af5a4dbba4e821b71a9b1