p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage #20197

jonatack · 2020-10-20T13:45:48Z

Now that #19991 and #20210 have been merged, we can determine inbound onion peers using CNode::m_inbound_onion and add it to the localhost peers protection in AttemptToEvictConnection, which was added in #19670 to address issue #19500.

Update 28 February 2021: I've updated this to follow gmaxwell's suggestion in #20197 (comment).

This branch now protects up to 1/4 onion peers (connected via our tor control service), if any, sorted by longest uptime. If any (or all) onion slots remain after that operation, they are then allocated to protect localhost peers, or a minimum of 2 localhost peers in the case that no onion slots remain and 2 or more onion peers were protected, sorted as before by longest uptime.

This patch also adds test coverage for the longest uptime, localhost, and onion peer eviction protection logic to build on the welcome initial unit testing of #20477.

Suggest reviewing the commits that move code with colorMoved = dimmed-zebra and colorMovedWs = allow-indentation-change.

Closes #11537.

sdaftuar · 2020-10-20T14:07:35Z

Concept ACK

DrahtBot · 2020-10-20T14:35:38Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

net/net processing: Move tx inventory into net_processing #21160 (net/net processing: Move tx inventory into net_processing by jnewbery)
net: fix GetListenPort() to derive the proper port #20196 (net: fix GetListenPort() to derive the proper port by vasild)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

practicalswift · 2020-10-20T15:00:06Z

Concept ACK

hebasto · 2020-10-20T16:51:02Z

Concept ACK.

jonatack · 2020-10-21T10:15:41Z

Added CNode::m_inbound_onion unit test coverage in #20210.

gmaxwell · 2020-10-21T20:43:06Z

Sounds good, although you might want to add a couple slots for actual localhost peers. :)

sipa · 2020-11-05T02:03:24Z

Sounds good, although you might want to add a couple slots for actual localhost peers. :)

Anyone with an old manually-configured hidden service not using the new bind style will have their inbound Tor connections not classified as such, so this may be degradation for them if there is no explicit affordance for localhost peers.

jonatack · 2020-11-05T07:58:10Z

Thank you for the feedback @gmaxwell and @sipa, will update soon.

practicalswift · 2020-12-23T14:09:44Z

Now that we have unit testing of the eviction logic in master (see merged PR #20477): what about adding a small test for this change? :)

jonatack · 2020-12-23T14:41:55Z

Rebased, and updated to fall back to localhost if no onion peers are removed. Depending on feedback, this could go further and count onion peers removed to activate localhost if too few onion peers are removed.

Now that we have unit testing of the eviction logic in master (see merged PR #20477): what about adding a small test for this change? :)

@practicalswift I agree and spent time yesterday writing tests after extracting some of the untested parts of SelectNodeToEvict() to a separate function for deterministic testability of the current code. Edit: done.

jonatack · 2020-12-25T23:51:51Z

Rebased and updated after merge of #19972 that created a silent merge conflict.

vasild

ACK c16ed37

The code looks ok, after some staring at it. Just some minor notes below.

src/test/fuzz/node_eviction.cpp

src/net.h

src/net.cpp

…add getter, unit tests 86c4952 net: add CNode::IsInboundOnion() public getter and unit tests (Jon Atack) 6609eb8 net: assert CNode::m_inbound_onion is inbound in ctor (Jon Atack) 993d1ec test, fuzz: fix constructing CNode with invalid inbound_onion (Jon Atack) Pull request description: The goal of this PR is to be able to depend on `m_inbound_onion` in AttemptToEvictConnection in #20197: - asserts `CNode::m_inbound_onion` is inbound in the CNode ctor to have a validity check at the class boundary - fixes a unit test and a fuzz utility that were passing invalid inbound onion values to the CNode ctor - drops an unneeded check in `CNode::ConnectedThroughNetwork()` for its inbound status - adds a public getter `IsInboundOnion()` that also allows unit testing it - adds unit test coverage ACKs for top commit: sipa: utACK 86c4952 LarryRuane: ACK 86c4952 vasild: ACK 86c4952 MarcoFalke: review ACK 86c4952 🐍 Tree-SHA512: 21109105bc4e5e03076fadd489204be00eac710c9de0127708ca2d0a10a048ff81f640f589a7429967ac3eb51d35fe24bb2b12e53e7aa3efbc47aaff6396d204

… ctor, add getter, unit tests 86c4952 net: add CNode::IsInboundOnion() public getter and unit tests (Jon Atack) 6609eb8 net: assert CNode::m_inbound_onion is inbound in ctor (Jon Atack) 993d1ec test, fuzz: fix constructing CNode with invalid inbound_onion (Jon Atack) Pull request description: The goal of this PR is to be able to depend on `m_inbound_onion` in AttemptToEvictConnection in bitcoin#20197: - asserts `CNode::m_inbound_onion` is inbound in the CNode ctor to have a validity check at the class boundary - fixes a unit test and a fuzz utility that were passing invalid inbound onion values to the CNode ctor - drops an unneeded check in `CNode::ConnectedThroughNetwork()` for its inbound status - adds a public getter `IsInboundOnion()` that also allows unit testing it - adds unit test coverage ACKs for top commit: sipa: utACK 86c4952 LarryRuane: ACK 86c4952 vasild: ACK 86c4952 MarcoFalke: review ACK 86c4952 🐍 Tree-SHA512: 21109105bc4e5e03076fadd489204be00eac710c9de0127708ca2d0a10a048ff81f640f589a7429967ac3eb51d35fe24bb2b12e53e7aa3efbc47aaff6396d204

…ests dee2d6f fuzz: Avoid designated initialization (C++20) in fuzz tests (practicalswift) Pull request description: Avoid designated initialization (C++20) in fuzz tests. Context: bitcoin/bitcoin#20197 (comment), bitcoin/bitcoin#20936 (comment) ACKs for top commit: MarcoFalke: cr ACK dee2d6f dhruv: code review ACK dee2d6f ajtowns: utACK dee2d6f Tree-SHA512: 5940fab6e97a2b11dd3b1475d2cffa2840dc2e6ec34bd9f9df90f948709cab98fd1c513d5dd104816d33a525a6e9710b8715b02db941e35d84f92bc211f56d1d

… fuzz tests dee2d6f fuzz: Avoid designated initialization (C++20) in fuzz tests (practicalswift) Pull request description: Avoid designated initialization (C++20) in fuzz tests. Context: bitcoin#20197 (comment), bitcoin#20936 (comment) ACKs for top commit: MarcoFalke: cr ACK dee2d6f dhruv: code review ACK dee2d6f ajtowns: utACK dee2d6f Tree-SHA512: 5940fab6e97a2b11dd3b1475d2cffa2840dc2e6ec34bd9f9df90f948709cab98fd1c513d5dd104816d33a525a6e9710b8715b02db941e35d84f92bc211f56d1d

sdaftuar · 2021-02-17T20:59:26Z

Sounds good, although you might want to add a couple slots for actual localhost peers. :)

I interpreted this comment from @gmaxwell as suggesting that we should protect some actual localhost peers, even if we are also protecting some onion peers -- looks like this patch right now would only try to protect some localhost peers if there are no onion peers that we're protecting. Any reason not to protect some of each? I think that having more categories/metrics that we use to distinguish inbounds will make it harder for an adversary to take over all our inbound slots.

An unordered set can tell if an element is present in ~O(1) time (constant on average, worst case linear to the size of the container), which speeds up and simplifies the lookup in IsEvicted(). Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

Thank you to Vasil Dimov (vasild) for the suggestion to use std::unordered_set rather than std::vector for the IsProtected() peer id arguments.

and an `m_is_onion` struct member to NodeEvictionCandidate and tests. We'll use these in the peer eviction logic to protect inbound onion peers in addition to the existing protection of localhost peers.

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

Now that we have a reliable way to detect inbound onion peers, this commit updates our existing eviction protection of 1/4 localhost peers to instead protect up to 1/4 onion peers (connected via our tor control service), sorted by longest uptime. Any remaining slots of the 1/4 are then allocated to protect localhost peers, or 2 localhost peers if no slots remain and 2 or more onion peers are protected, sorted by longest uptime. The goal is to avoid penalizing onion peers, due to their higher min ping times relative to IPv4 and IPv6 peers, and improve our diversity of peer connections. Thank you to Gregory Maxwell, Suhas Daftuar, Vasil Dimov and Pieter Wuille for valuable review feedback that shaped the direction.

jonatack · 2021-03-19T19:30:30Z

Added documentation and renamed a function per #20197 (comment).

git diff c18fd59 0cca08a

diff --git a/src/net.cpp b/src/net.cpp
index be93426377..66d81b115f 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -887,7 +887,7 @@ static void EraseLastKElements(
     elements.erase(std::remove_if(elements.end() - eraseSize, elements.end(), predicate), elements.end());
 }
 
-void ProtectEvictionCandidates(std::vector<NodeEvictionCandidate>& vEvictionCandidates)
+void ProtectEvictionCandidatesByRatio(std::vector<NodeEvictionCandidate>& vEvictionCandidates)
 {
     // Protect the half of the remaining nodes which have been connected the longest.
     // This replicates the non-eviction implicit behavior, and precludes attacks that start later.
@@ -945,7 +945,9 @@ void ProtectEvictionCandidates(std::vector<NodeEvictionCandidate>& vEvictionCand
     // An attacker cannot manipulate this metric without performing useful work.
     EraseLastKElements(vEvictionCandidates, CompareNodeBlockTime, 4);
 
-    ProtectEvictionCandidates(vEvictionCandidates);
+    // Protect some of the remaining eviction candidates by ratios of desirable
+    // or disadvantaged characteristics.
+    ProtectEvictionCandidatesByRatio(vEvictionCandidates);
 
     if (vEvictionCandidates.empty()) return std::nullopt;
 
diff --git a/src/net.h b/src/net.h
index 449e0c54b7..eb7fa079ab 100644
--- a/src/net.h
+++ b/src/net.h
@@ -1285,8 +1285,36 @@ struct NodeEvictionCandidate
     bool m_is_onion;
 };
 
-void ProtectEvictionCandidates(std::vector<NodeEvictionCandidate>& vEvictionCandidates);
-
+/**
+ * Select an inbound peer to evict after filtering out (protecting) peers having
+ * distinct, difficult-to-forge characteristics. The protection logic picks out
+ * fixed numbers of desirable peers per various criteria, followed by (mostly)
+ * ratios of desirable or disadvantaged peers. If any eviction candidates
+ * remain, the selection logic chooses a peer to evict.
+ */
 [[nodiscard]] std::optional<NodeId> SelectNodeToEvict(std::vector<NodeEvictionCandidate>&& vEvictionCandidates);
 
+/** Protect desirable or disadvantaged inbound peers from eviction by ratio.
+ *
+ * This function protects half of the peers which have been connected the
+ * longest, to replicate the non-eviction implicit behavior and preclude attacks
+ * that start later.
+ *
+ * Half of these protected spots (1/4 of the total) are reserved for onion peers
+ * connected via our tor control service, if any, sorted by longest uptime, even
+ * if they're not longest uptime overall. Any remaining slots of the 1/4 are
+ * then allocated to protect localhost peers, if any (or up to 2 localhost peers
+ * if no slots remain and 2 or more onion peers were protected), sorted by
+ * longest uptime, as manually configured hidden services not using
+ * `-bind=addr[:port]=onion` will not be detected as inbound onion connections.
+ *
+ * This helps protect onion peers, which tend to be otherwise disadvantaged
+ * under our eviction criteria for their higher min ping times relative to IPv4
+ * and IPv6 peers, and favorise the diversity of peer connections.
+ *
+ * This function was extracted from SelectNodeToEvict() to be able to test the
+ * ratio-based protection logic deterministically.
+ */
+void ProtectEvictionCandidatesByRatio(std::vector<NodeEvictionCandidate>& vEvictionCandidates);
+
 #endif // BITCOIN_NET_H
diff --git a/src/test/net_peer_eviction_tests.cpp b/src/test/net_peer_eviction_tests.cpp
index 7b88a3e9b6..31d391bf7d 100644
--- a/src/test/net_peer_eviction_tests.cpp
+++ b/src/test/net_peer_eviction_tests.cpp
@@ -43,9 +43,9 @@ std::vector<NodeEvictionCandidate> GetRandomNodeEvictionCandidates(const int n_c
 }
 
 // Create `num_peers` random nodes, apply setup function `candidate_setup_fn`,
-// apply protection logic, and then return true if all of `protected_peer_ids`
-// and none of `unprotected_peer_ids` are protected from eviction, i.e. removed
-// from the eviction candidates.
+// call ProtectEvictionCandidatesByRatio() to apply protection logic, and then
+// return true if all of `protected_peer_ids` and none of `unprotected_peer_ids`
+// are protected from eviction, i.e. removed from the eviction candidates.
 bool IsProtected(int num_peers,
                  std::function<void(NodeEvictionCandidate&)> candidate_setup_fn,
                  const std::unordered_set<NodeId>& protected_peer_ids,
@@ -60,7 +60,7 @@ bool IsProtected(int num_peers,
 
     const size_t size{candidates.size()};
     const size_t expected{size - size / 2}; // Expect half the candidates will be protected.
-    ProtectEvictionCandidates(candidates);
+    ProtectEvictionCandidatesByRatio(candidates);
     BOOST_CHECK_EQUAL(candidates.size(), expected);

vasild

ACK 0cca08a

ghost · 2021-03-27T20:46:23Z

Interesting PR. Concept ACK. Compiled successfully on Ubuntu. Tests passed.

laanwj · 2021-03-30T14:20:18Z

Code review ACK 0cca08a
I did not do anything in terms of manual testing, but it looks like the change is well-covered by tests.

mzumsande

Post-Merge Comment (reviewed but didn't finish in time yesterday evening):

I understand the motivation, but just noting that the fixed localhost_min_protect_size in the ratio-based ProtectEvictionCandidatesByRatio makes things unintuitive at a lower initial_size:

Both cases are nicely covered by the unit tests, but it doesn't make a lot of sense to me that at 6 total peers, we'd protect 1 onion, 0 localhost and 2 by generic uptime, whereas at 8 peers, we'd protect 2 onion, 2 localhost and 0 by generic uptime (raising the percentage of potentially protected tor peers to 50% instead of the targeted 25%, so that no slots are left for protecting generic uptime peers).

src/net.cpp

jonatack · 2021-03-30T16:33:58Z

Thanks for having a look! Here we have an initial improvement and unit testing in place, and in the next step in #21261 we'll re-evaluate the algorithm and ratios between onion, localhost and I2P peers (considering that other privacy networks may be added soon as well). Please do weigh in there with the ratios you would suggest.

…), add …

jonatack · 2021-04-20T14:40:53Z

@mzumsande I've updated the inbound eviction protection proposal in #21261 to be generalizable to multiple networks (with I2P and CJDNS on the way) and fully ratio-based, which should address your feedback. Will be bringing it out of draft shortly.

…tworks, add I2P peers 1b1088d test: add combined I2P/onion/localhost eviction protection tests (Jon Atack) 7c2284e test: add tests for inbound eviction protection of I2P peers (Jon Atack) ce02dd1 p2p: extend inbound eviction protection by network to I2P peers (Jon Atack) 70bbc62 test: add combined onion/localhost eviction protection coverage (Jon Atack) 045cb40 p2p: remove unused m_is_onion member from NodeEvictionCandidate struct (Jon Atack) 310fab4 p2p: remove unused CompareLocalHostTimeConnected() (Jon Atack) 9e889e8 p2p: remove unused CompareOnionTimeConnected() (Jon Atack) 787d46b p2p: update ProtectEvictionCandidatesByRatio() doxygen docs (Jon Atack) 1e15acf p2p: make ProtectEvictionCandidatesByRatio() fully ratio-based (Jon Atack) 3f8105c test: remove combined onion/localhost eviction protection tests (Jon Atack) 38a81a8 p2p: add CompareNodeNetworkTime() comparator struct (Jon Atack) 4ee7aec p2p: add m_network to NodeEvictionCandidate struct (Jon Atack) 7321e6f p2p, refactor: rename vEvictionCandidates to eviction_candidates (Jon Atack) ec590f1 p2p, refactor: improve constness in ProtectEvictionCandidatesByRatio() (Jon Atack) 4a19f50 test: add ALL_NETWORKS to test utilities (Jon Atack) 519e76b test: speed up and simplify peer_eviction_test (Jon Atack) 1cde800 p2p, refactor: rm redundant erase_size calculation in SelectNodeToEvict() (Jon Atack) Pull request description: Continuing the work in #20197 and #20685, this pull updates and abstracts our inbound eviction protection to make it fully ratio-based and easily extensible to peers connected via high-latency privacy networks that we newly support, like I2P and perhaps others soon, as these peers are disadvantaged by the latency criteria of our eviction logic. It then adds eviction protection for peers connected over I2P. As described in #20685 (comment), we've observed over the past few months that I2P peers have a min ping latency similar to or greater than that of onion peers. The algorithm is a basically a multi-pass knapsack: - Count the number of eviction candidates in each of the disadvantaged privacy networks. - Sort the networks from lower to higher candidate counts, so that a network with fewer candidates will have the first opportunity for any unused slots remaining from the previous iteration. In the case of a tie in candidate counts, priority is given by array member order from first to last, guesstimated to favor more unusual networks. - Iterate through the networks in this order. On each iteration, allocate each network an equal number of protected slots targeting a total number of candidates to protect, provided any slots remain in the knapsack. - Protect the candidates in that network having the longest uptime, if any in that network are present. - Continue iterating as long as we have non-allocated slots remaining and candidates available to protect. The goal of this logic is to favorise the diversity of our peer connections. The individual commit messages describe each change in more detail. Special thank you to Vasil Dimov for the excellent review feedback and the algorithm improvement that made this change much better than it would have been otherwise. Thanks also to Antoine Riard, whose review feedback nudged this change to protect disadvantaged networks having fewer, rather than more, eviction candidates. ACKs for top commit: laanwj: Code review re-ACK 1b1088d vasild: ACK 1b1088d Tree-SHA512: 722f790ff11f2969c79e45a5e0e938d94df78df8687e77002f32e3ef5c72a9ac10ebf8c7a9eb7f71882c97ab0e67b2778191effdb747d9ca54d7c23c2ed19a90

…iple networks, add I2P peers 1b1088d test: add combined I2P/onion/localhost eviction protection tests (Jon Atack) 7c2284e test: add tests for inbound eviction protection of I2P peers (Jon Atack) ce02dd1 p2p: extend inbound eviction protection by network to I2P peers (Jon Atack) 70bbc62 test: add combined onion/localhost eviction protection coverage (Jon Atack) 045cb40 p2p: remove unused m_is_onion member from NodeEvictionCandidate struct (Jon Atack) 310fab4 p2p: remove unused CompareLocalHostTimeConnected() (Jon Atack) 9e889e8 p2p: remove unused CompareOnionTimeConnected() (Jon Atack) 787d46b p2p: update ProtectEvictionCandidatesByRatio() doxygen docs (Jon Atack) 1e15acf p2p: make ProtectEvictionCandidatesByRatio() fully ratio-based (Jon Atack) 3f8105c test: remove combined onion/localhost eviction protection tests (Jon Atack) 38a81a8 p2p: add CompareNodeNetworkTime() comparator struct (Jon Atack) 4ee7aec p2p: add m_network to NodeEvictionCandidate struct (Jon Atack) 7321e6f p2p, refactor: rename vEvictionCandidates to eviction_candidates (Jon Atack) ec590f1 p2p, refactor: improve constness in ProtectEvictionCandidatesByRatio() (Jon Atack) 4a19f50 test: add ALL_NETWORKS to test utilities (Jon Atack) 519e76b test: speed up and simplify peer_eviction_test (Jon Atack) 1cde800 p2p, refactor: rm redundant erase_size calculation in SelectNodeToEvict() (Jon Atack) Pull request description: Continuing the work in bitcoin#20197 and bitcoin#20685, this pull updates and abstracts our inbound eviction protection to make it fully ratio-based and easily extensible to peers connected via high-latency privacy networks that we newly support, like I2P and perhaps others soon, as these peers are disadvantaged by the latency criteria of our eviction logic. It then adds eviction protection for peers connected over I2P. As described in bitcoin#20685 (comment), we've observed over the past few months that I2P peers have a min ping latency similar to or greater than that of onion peers. The algorithm is a basically a multi-pass knapsack: - Count the number of eviction candidates in each of the disadvantaged privacy networks. - Sort the networks from lower to higher candidate counts, so that a network with fewer candidates will have the first opportunity for any unused slots remaining from the previous iteration. In the case of a tie in candidate counts, priority is given by array member order from first to last, guesstimated to favor more unusual networks. - Iterate through the networks in this order. On each iteration, allocate each network an equal number of protected slots targeting a total number of candidates to protect, provided any slots remain in the knapsack. - Protect the candidates in that network having the longest uptime, if any in that network are present. - Continue iterating as long as we have non-allocated slots remaining and candidates available to protect. The goal of this logic is to favorise the diversity of our peer connections. The individual commit messages describe each change in more detail. Special thank you to Vasil Dimov for the excellent review feedback and the algorithm improvement that made this change much better than it would have been otherwise. Thanks also to Antoine Riard, whose review feedback nudged this change to protect disadvantaged networks having fewer, rather than more, eviction candidates. ACKs for top commit: laanwj: Code review re-ACK 1b1088d vasild: ACK 1b1088d Tree-SHA512: 722f790ff11f2969c79e45a5e0e938d94df78df8687e77002f32e3ef5c72a9ac10ebf8c7a9eb7f71882c97ab0e67b2778191effdb747d9ca54d7c23c2ed19a90

Summary: to allow deterministic unit testing of the ratio-based peer eviction protection logic, which protects peers having longer connection times and those connected via higher-latency networks. Add documentation. This is a backport of [[bitcoin/bitcoin#20197 | core#20197]] [1/6] bitcoin/bitcoin@f126cbd Depends on D10973 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10975

Summary: out of net_tests, because the eviction tests: - are a different domain of test coverage, with different dependencies - run more slowly than the net tests - will be growing in size, in this PR branch and in the future, as eviction test coverage is improved This is a backport of [[bitcoin/bitcoin#20197 | core#20197]] [2/6] bitcoin/bitcoin@41f84d5 Depends on D10975 Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10976

Summary: An unordered set can tell if an element is present in ~O(1) time (constant on average, worst case linear to the size of the container), which speeds up and simplifies the lookup in IsEvicted(). Co-authored-by: Vasil Dimov <vd@FreeBSD.org> This is a backport of [[bitcoin/bitcoin#20197 | core#20197]] [3/6] bitcoin/bitcoin@ca63b53 Depends on D10976 Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10977

Summary: This is a backport of [[bitcoin/bitcoin#20197 | core#20197]] [4/6] bitcoin/bitcoin@72e30e8 Depends on D10977 Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10978

Summary: This combines `EraseLastKElementsIf` (introduced in D9863) with the existing `EraseLastKElements` function by using a default `return true` predicate. This is a backport of [[bitcoin/bitcoin#20197 | core#20197]] [5/6] bitcoin/bitcoin@8f1a53e Depends on D10978 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10979

Summary: bitcoin/bitcoin@8b1e156 > Add `m_inbound_onion` to `AttemptToEvictConnection()` and an `m_is_onion` struct member to NodeEvictionCandidate and tests. > > We'll use these in the peer eviction logic to protect inbound onion peers > in addition to the existing protection of localhost peers. bitcoin/bitcoin@caa21f5 > Now that we have a reliable way to detect inbound onion peers, this commit > updates our existing eviction protection of 1/4 localhost peers to instead > protect up to 1/4 onion peers (connected via our tor control service), sorted by > longest uptime. Any remaining slots of the 1/4 are then allocated to protect > localhost peers, or 2 localhost peers if no slots remain and 2 or more onion > peers are protected, sorted by longest uptime. > > The goal is to avoid penalizing onion peers, due to their higher min ping times > relative to IPv4 and IPv6 peers, and improve our diversity of peer connections. > > Thank you to Gregory Maxwell, Suhas Daftuar, Vasil Dimov and Pieter Wuille > for valuable review feedback that shaped the direction. bitcoin/bitcoin@0cca08a > Add unit test coverage for our onion peer eviction protectioni Note: the commits are squashed to have the feature and corresponding test at the same time This concludes backport of [[bitcoin/bitcoin#20197 | core#20197]] Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10980

…JDNS peers b7be28c test: add combined CJDNS/I2P/localhost/onion eviction protection tests (Jon Atack) 0a1bb84 test: add tests for inbound eviction protection of CJDNS peers (Jon Atack) 0c00c0c test: fix off-by-one logic in an eviction protection test (Jon Atack) f7b8094 p2p: extend inbound eviction protection by network to CJDNS peers (Jon Atack) Pull request description: Extend inbound eviction protection for peers connected over CJDNS, as is the case for peers connected via onion, localhost, and I2P since #21261 and #20197. CJDNS peers seem to have better min ping latency than onion and I2P peers but still higher than that of unencrypted IPv4/6 peers and can be disadvantaged under our eviction criteria. They are also very few in number, which is a further reason to protect them, as the goal of this logic is to favorise the diversity of our peer connections. CJDNS support was added in #23077 for the upcoming v23 release. ACKs for top commit: laanwj: Concept and code review ACK b7be28c w0xlt: tACK b7be28c Tree-SHA512: 89ebdd217602e16ae14b9bd0d5a25fc09f9b2384c951f820bc0f5a6d8452bbc9042065db817d5d5296c0ad22988491a83fc5b9a611e660c40ebd4f03448c4061

… fuzz tests dee2d6f fuzz: Avoid designated initialization (C++20) in fuzz tests (practicalswift) Pull request description: Avoid designated initialization (C++20) in fuzz tests. Context: bitcoin#20197 (comment), bitcoin#20936 (comment) ACKs for top commit: MarcoFalke: cr ACK dee2d6f dhruv: code review ACK dee2d6f ajtowns: utACK dee2d6f Tree-SHA512: 5940fab6e97a2b11dd3b1475d2cffa2840dc2e6ec34bd9f9df90f948709cab98fd1c513d5dd104816d33a525a6e9710b8715b02db941e35d84f92bc211f56d1d

fanquake added the P2P label Oct 20, 2020

DrahtBot mentioned this pull request Oct 20, 2020

fuzz: Add fuzzing harness for node eviction logic #19972

Merged

jonatack mentioned this pull request Oct 21, 2020

net: assert CNode::m_inbound_onion is inbound in ctor, add getter, unit tests #20210

Merged

DrahtBot mentioned this pull request Nov 4, 2020

net: Make it easier to reason about node eviction by removing unused NodeEvictionCandidate::addr (CAddress) #20302

Merged

DrahtBot added the Needs rebase label Nov 5, 2020

jonatack force-pushed the AttemptToEvictConnection-identify-onions-with-m_inbound_onion branch from e48f150 to 587e5c9 Compare December 23, 2020 01:54

DrahtBot removed the Needs rebase label Dec 23, 2020

jonatack force-pushed the AttemptToEvictConnection-identify-onions-with-m_inbound_onion branch from 587e5c9 to c16ed37 Compare December 25, 2020 23:45

vasild approved these changes Jan 1, 2021

View reviewed changes

src/test/fuzz/node_eviction.cpp Outdated Show resolved Hide resolved

src/net.h Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

jonatack mentioned this pull request Jan 26, 2021

Add I2P support using I2P SAM #20685

Merged

DrahtBot mentioned this pull request Jan 27, 2021

Make all of net_processing (and some of net) use std::chrono types #21015

Merged

practicalswift mentioned this pull request Jan 30, 2021

fuzz: Avoid designated initialization (C++20) in fuzz tests #21037

Merged

DrahtBot mentioned this pull request Feb 12, 2021

net/net processing: Move tx inventory into net_processing #21160

Merged

jonatack force-pushed the AttemptToEvictConnection-identify-onions-with-m_inbound_onion branch from c16ed37 to 2399356 Compare February 21, 2021 22:08

jonatack and others added 6 commits March 19, 2021 20:11

Add unit tests for ProtectEvictionCandidatesByRatio()

72e30e8

Thank you to Vasil Dimov (vasild) for the suggestion to use std::unordered_set rather than std::vector for the IsProtected() peer id arguments.

Add m_inbound_onion to AttemptToEvictConnection()

8b1e156

and an `m_is_onion` struct member to NodeEvictionCandidate and tests. We'll use these in the peer eviction logic to protect inbound onion peers in addition to the existing protection of localhost peers.

Use EraseLastKElements() throughout SelectNodeToEvict()

8f1a53e

Co-authored-by: Vasil Dimov <vd@FreeBSD.org>

Add unit test coverage for our onion peer eviction protection

0cca08a

jonatack force-pushed the AttemptToEvictConnection-identify-onions-with-m_inbound_onion branch from c18fd59 to 0cca08a Compare March 19, 2021 19:27

vasild approved these changes Mar 22, 2021

View reviewed changes

laanwj merged commit dede9eb into bitcoin:master Mar 30, 2021

jonatack deleted the AttemptToEvictConnection-identify-onions-with-m_inbound_onion branch March 30, 2021 14:23

mzumsande reviewed Mar 30, 2021

View reviewed changes

src/net.cpp Show resolved Hide resolved

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 30, 2021

Merge bitcoin#20197: p2p: protect onions in AttemptToEvictConnection(…

6161b67

…), add …

jonatack mentioned this pull request Jun 23, 2021

Add an option for evenly connecting to nodes on different types of networks #11537

Closed

jonatack mentioned this pull request Jan 26, 2022

p2p: extend inbound eviction protection by network to CJDNS peers #24165

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage #20197

p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage #20197

jonatack commented Oct 20, 2020 •

edited

Loading

sdaftuar commented Oct 20, 2020

DrahtBot commented Oct 20, 2020 •

edited

Loading

practicalswift commented Oct 20, 2020

hebasto commented Oct 20, 2020

jonatack commented Oct 21, 2020 •

edited

Loading

gmaxwell commented Oct 21, 2020

sipa commented Nov 5, 2020

jonatack commented Nov 5, 2020

practicalswift commented Dec 23, 2020 •

edited

Loading

jonatack commented Dec 23, 2020 •

edited

Loading

jonatack commented Dec 25, 2020

vasild left a comment

sdaftuar commented Feb 17, 2021

jonatack commented Mar 19, 2021

vasild left a comment

ghost commented Mar 27, 2021

laanwj commented Mar 30, 2021

mzumsande left a comment

jonatack commented Mar 30, 2021

jonatack commented Apr 20, 2021

p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage #20197

p2p: protect onions in AttemptToEvictConnection(), add eviction protection test coverage #20197

Conversation

jonatack commented Oct 20, 2020 • edited Loading

sdaftuar commented Oct 20, 2020

DrahtBot commented Oct 20, 2020 • edited Loading

Conflicts

practicalswift commented Oct 20, 2020

hebasto commented Oct 20, 2020

jonatack commented Oct 21, 2020 • edited Loading

gmaxwell commented Oct 21, 2020

sipa commented Nov 5, 2020

jonatack commented Nov 5, 2020

practicalswift commented Dec 23, 2020 • edited Loading

jonatack commented Dec 23, 2020 • edited Loading

jonatack commented Dec 25, 2020

vasild left a comment

Choose a reason for hiding this comment

sdaftuar commented Feb 17, 2021

jonatack commented Mar 19, 2021

vasild left a comment

Choose a reason for hiding this comment

ghost commented Mar 27, 2021

laanwj commented Mar 30, 2021

mzumsande left a comment

Choose a reason for hiding this comment

jonatack commented Mar 30, 2021

jonatack commented Apr 20, 2021

jonatack commented Oct 20, 2020 •

edited

Loading

DrahtBot commented Oct 20, 2020 •

edited

Loading

jonatack commented Oct 21, 2020 •

edited

Loading

practicalswift commented Dec 23, 2020 •

edited

Loading

jonatack commented Dec 23, 2020 •

edited

Loading