This PR splits off the next couple commits from #25268 that introduce the EvictionManager and use it for the inbound eviction logic.
One instance of the EvictionManager is created at start up and passed as a reference to the connection and peer managers. The connection and peer managers report all eviction relevant information (for inbound connections) to the eviction manager who ultimately suggests nodes to evict as the result of EvictionManager::SelectNodeToEvict.
250+ bool noban, ConnectionType conn_type)
251+{
252+ LOCK(m_candidates_mutex);
253+ // The default values used to initialise the eviction candidates result in
254+ // newer candidates being more likely to be evicted.
255+ NodeEvictionCandidate candidate{
I wonder if it might make sense to split NodeEvictionCandidate into 2 parts: the const properties that don’t change (like conn_type/noban) and the non-const state that’s intended to be/might be updated?
Maybe something like:
0class NodeEvictionCandidate {
1 class Properties {
2 NodeId m_id;
3 ConnectionType m_conn_type;
4 bool m_noban;
5 ...
6 };
7 NodeEvictionCandidate(NodeProperties properties) : m_properties(properties){}
8private:
9 const NodeProperties m_properties;
10 bool m_relay_txs;
11 bool fBloomFilter;
12 ...
13};
14...
15void EvictionManagerImpl::AddCandidate(NodeEvictionCandidate::Properties props) {...}
I think that might be more straightforward to use as it’s self-documenting, while also allowing for safe lock-free access to the stuff that can’t change.
Maybe that’s easier said than done though. Thoughts?
src/test/net_peer_eviction_tests.cpp tests sadly rely on no members of NodeEvictionCandidate being const. Would like to leave this for a follow up, since it requires changing those tests quite a bit.
Hmm, I’d say if the tests rely on non-const values, then the tests need to be reworked somewhat (I assume they’re just re-using existing candidates as opposed to creating new ones for the sake of simplicity). Either that or they’re testing state-changes that aren’t possible.
Agree that it’s not worth refactoring the tests in this PR, but could you maybe organize the members and add a note to the definition of NodeEvictionCandidate to make the possible state changes clear? I think there are basically 3 types, though I may be oversimplifying:
I have been looking a bit more at making the const NodeEvictionCandidate members const and it turns out that it is not just a test problem at the moment.
The eviction logic it self relies on non-constness due to multiple std::sort, vector::erase and std::remove_if calls. Will look at making a separate PR for this.
In the mean time i will add some comments in this PR like you suggested.
Concept ACK.
Added a high-level question about NodeEvictionCandidate to kick things off, will continue with review.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | theuni, jnewbery, naumenkogs |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
forceinbound to evict a random unprotected connection if all slots are otherwise full by pinheadmz)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.
2@@ -3,6 +3,7 @@
3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
4
5 #include <bench/bench.h>
6+#include <node/eviction_impl.h>
11@@ -12,6 +12,7 @@
12 #include <node/connection_types.h>
13 #include <consensus/amount.h>
14 #include <crypto/siphash.h>
15+#include <node/eviction.h>
38@@ -39,7 +39,7 @@ struct CNodeStateStats {
39 class PeerManager : public CValidationInterface, public NetEventsInterface
40 {
41 public:
42- static std::unique_ptr<PeerManager> make(CConnman& connman, AddrMan& addrman,
43+ static std::unique_ptr<PeerManager> make(CConnman& connman, AddrMan& addrman, EvictionManager& evictionman,
#include <node/eviction.h>
28- bool prefer_evict;
29- bool m_is_local;
30- Network m_network;
31- bool m_noban;
32- ConnectionType m_conn_type;
33+class EvictionManagerImpl;
75+ void UpdateLoadedBloomFilter(NodeId id, bool bloom_filter_loaded);
76+
77+ /** Set the candidates tx relay status to true. */
78+ void UpdateRelayTxs(NodeId id);
79 };
80
36+{
37+private:
38+ const std::unique_ptr<EvictionManagerImpl> m_impl;
39+
40+public:
41+ explicit EvictionManager();
explicit keyword, since this ctor doesn’t take any arguments.
21@@ -22,6 +22,7 @@ class CTxMemPool;
22 class ChainstateManager;
23 class NetGroupManager;
24 class PeerManager;
25+class EvictionManager;
1094@@ -1112,6 +1095,11 @@ void CConnman::DisconnectNodes()
1095 }
1096 }
1097 }
1098+
1099+ for (NodeId id : disconnected_eviction_candidates) {
1100+ m_evictionman.RemoveCandidate(id);
1101+ }
Delete disconnected nodes loop below?
I guess I don’t understand why the node can’t be removed from the EvictionManager either:
m_nodes and added to m_nodes_disconnected; orm_nodes_disconnected.Perhaps a code comment could be added to explain why.
Cory originally suggested to it this way to untangle the removal from m_nodes_mutex. (See #25268 (review))
I think it could make sense to do it in the Delete disconnected nodes loop below which is also independent of m_node_mutex. Another option would be to do it in PeerManagerImpl::FinalizeNode() but that might be weird since we add candidates in net.
Ah, thanks for the explanation.
I can’t see any reason why this call would have to be done outside the m_nodes_mutex. EvictionManager should never call into net, so there isn’t any chance of a lock inversion. Besides, there are other EvictionManager methods (eg the Get...() methods) which are called while holding m_nodes_mutex.
553@@ -582,7 +554,6 @@ class CNode
554 /** A ping-pong round trip has completed successfully. Update latest and minimum ping times. */
0 /** A ping-pong round trip has completed successfully. Update latest ping time. */
In fact, m_last_ping_time may as well be moved into Peer since it’s not used in any net level logic and is only for RPC/GUI stats. Could be done as a follow-up.
2553@@ -2555,6 +2554,22 @@ void CConnman::GetNodeStats(std::vector<CNodeStats>& vstats) const
2554 for (CNode* pnode : m_nodes) {
2555 vstats.emplace_back();
2556 pnode->CopyStats(vstats.back());
2557+
2558+ auto min_ping_time{m_evictionman.GetMinPingTime(pnode->GetId())};
2559+ if (min_ping_time) {
Can be shortened to
0 if (auto min_ping_time = m_evictionman.GetMinPingTime(pnode->GetId())) {
if you think that’s better. Same below.
58+
59+ /** A ping-pong round trip has completed successfully. Update minimum ping time. */
60+ void UpdateMinPingTime(NodeId id, std::chrono::microseconds ping_time);
61+ std::optional<std::chrono::microseconds> GetMinPingTime(NodeId id) const;
62+
63+ /** A new valid block was received. Update the candidates last block time. */
Grammar nit:
0 /** A new valid block was received. Update the candidate's last block time. */
Same below.
56+ */
57+ [[nodiscard]] std::optional<NodeId> SelectNodeToEvict() const;
58+
59+ /** A ping-pong round trip has completed successfully. Update minimum ping time. */
60+ void UpdateMinPingTime(NodeId id, std::chrono::microseconds ping_time);
61+ std::optional<std::chrono::microseconds> GetMinPingTime(NodeId id) const;
GetEvictionCandidateStats() or similar, since they’ll only be used for reporting stats to the user. If you think that’s a good idea, perhaps add that to the function comment for now.
288+ }
289+ }
290+ return ::SelectNodeToEvict(std::move(candidates));
291+}
292+
293+void EvictionManagerImpl::UpdateMinPingTime(NodeId id, std::chrono::microseconds ping_time)
I think something like this would work:
0class EvictionManagerImpl
1{
2 ...
3 template <typename Fn>
4 auto CandidateOp(NodeId id, Fn&& fn)
5 {
6 LOCK(m_candidates_mutex);
7 auto it = m_candidates.find(offset);
8 if (it != m_candidates.end()) {
9 return fn(it->second);
10 } else {
11 // return a dummy value of the right type, including void
12 return ([]() -> decltype(fn(it->second)) { return {}; })();
13 // EDIT: maybe nicer to write:
14 using T = decltype(fn(it->second));
15 return T();
16 }
17 }
18 ...
19};
20
21void EvictionManager::UpdateLastBlockTime(NodeId id, std::chrono::seconds block_time)
22{
23 m_impl->CandidateOp(id, [](NodeEvictionCandidate& nec) {
24 nec.m_last_block_time = block_time;
25 });
26}
27
28void EvictionManager::GetLastBlockTime(NodeId id) const
29{
30 return m_impl->CandidateOp(id, [](const NodeEvictionCandidate& nec) {
31 return nec.m_last_block_time;
32 });
33}
1@@ -2,6 +2,7 @@
2 // Distributed under the MIT software license, see the accompanying
3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
4
5+#include <node/eviction_impl.h>
31+ Network m_network;
32+ bool m_noban;
33+ ConnectionType m_conn_type;
34+};
35+
36+[[nodiscard]] std::optional<NodeId> SelectNodeToEvict(std::vector<NodeEvictionCandidate>&& vEvictionCandidates);
ProtectEvictionCandidatesByRatio() aren’t members of EvictionManagerImpl? I’m not saying they have to be, but I’m interested in the design choice.
I think that test refactoring would probably be a good thing, since it would ensure that the unit test is not testing a state that EvictionManager can’t get into in the product code. In general, that seems like a good model - unit tests use the public methods to update the state of the object being tested, but optionally also have access to const test-only methods to peek inside the state of the object if it’s difficult to expose that state in other ways.
Certainly not required in this PR. Could be done in a follow-up (or not at all).
Big concept ACK!
Minor cosmetic comments inline.
248+ uint64_t keyed_net_group, bool prefer_evict,
249+ bool is_local, Network network,
250+ bool noban, ConnectionType conn_type)
251+{
252+ LOCK(m_candidates_mutex);
253+ // The default values used to initialise the eviction candidates result in
why it is so
44@@ -45,10 +45,19 @@ BOOST_FIXTURE_TEST_SUITE(denialofservice_tests, TestingSetup)
45 // work.
46 BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction)
47 {
48- ConnmanTestMsg& connman = static_cast<ConnmanTestMsg&>(*m_node.connman);
49- // Disable inactivity checks for this test to avoid interference
50- connman.SetPeerConnectTimeout(99999s);
51- PeerManager& peerman = *m_node.peerman;
52+ auto evictionman = std::make_unique<EvictionManager>();
Why is this switched away from using m_node? Not that I object, it seems reasonable to me.
If it’s just because “it’s the only use of m_node.connman in these tests”, maybe break that change out as a separate commit?
You actually had this in your original branch and I just kept it this way but it made sense to me since all the other (dos) tests do it this way as well (i.e. “it’s the only use of m_node.connman in these tests”).
Added a commit that splits out this change.
NodeEvictionCandidate values.
490@@ -490,7 +491,7 @@ struct CNodeState {
491 class PeerManagerImpl final : public PeerManager
492 {
493 public:
494- PeerManagerImpl(CConnman& connman, AddrMan& addrman,
495+ PeerManagerImpl(CConnman& connman, AddrMan& addrman, EvictionManager& evictionman,
Why not just use the evictionman from connman directly (or have connman use its peerman’s eviction manager)? Is there any reason to ever want them to have different eviction managers?
Having done that, why not have CConnman (or PeerManager) create (and own) its own eviction manager, rather than needing one to be passed in?
I think it makes sense to not have the eviction manager be owned by another component, in the context of also moving the outbound eviction logic to the eviction manager. I see it as more of a standalone component like the addrman or banman.
This might also give us flexibility in mocking it if we wanted to for testing purposes (e.g. to test that net processing and net are updating the stats correctly, or have a FuzzedEvictionManager to let the fuzzer choose which peers to evict).
289+std::optional<NodeId> EvictionManagerImpl::SelectNodeToEvict() const
290+{
291+ std::vector<NodeEvictionCandidate> candidates;
292+ {
293+ LOCK(m_candidates_mutex);
294+ for (const auto& [id, candidate] : m_candidates) {
candidates.reserve(m_candidates.size()) ?
2609+ if (auto last_block_time = m_evictionman.GetLastBlockTime(pnode->GetId())) {
2610+ vstats.back().m_last_block_time = *last_block_time;
2611+ }
2612+
2613+ if (auto last_tx_time = m_evictionman.GetLastTxTime(pnode->GetId())) {
2614+ vstats.back().m_last_tx_time = *last_tx_time;
m_evictionman.CopyEvictionStats(pnode->GetId(), vstats.back()); ?
5014@@ -5008,7 +5015,7 @@ void PeerManagerImpl::EvictExtraOutboundPeers(std::chrono::seconds now)
5015 if (pnode->GetId() > youngest_peer.first) {
5016 next_youngest_peer = youngest_peer;
5017 youngest_peer.first = pnode->GetId();
5018- youngest_peer.second = pnode->m_last_block_time;
5019+ youngest_peer.second = *Assert(m_evictionman.GetLastBlockTime(pnode->GetId()));
Repeatedly looking up the eviction manager map and locking and unlocking access to it
Two things:
and introducing the potential for maps to be out of sync and having to add asserts to check for that doesn’t seem ideal…
I don’t remember what I was thinking at the time but now this Assert doesn’t seem great to me either. I think this is safe because ForEachNode locks m_nodes_mutex and only iterates over nodes that completed the version handshake but that is quite the spaghetti assumption.
Simply using the default value for the last block time if the eviction manager doesn’t know about this peer would probably be the right thing to do. And then without the assertions, I wouldn’t be concerned about the maps (assuming your are talking about the peer map and the eviction manager map) being out of sync.
(This also resolves itself if we end up moving the outbound eviction logic to the eviction manager as well, because then there is only one internal eviction map to think about)
923- .m_conn_type = node->m_conn_type,
924- };
925- vEvictionCandidates.push_back(candidate);
926+
927+ auto eviction_candidate{m_evictionman.GetCandidate(node->GetId())};
928+ assert(eviction_candidate);
Assume and continue in order to avoid the assert.
403@@ -404,7 +404,6 @@ class CNode
404 * from the wire. This cleaned string can safely be logged or displayed.
405 */
406 std::string cleanSubVer GUARDED_BY(m_subver_mutex){};
407- const bool m_prefer_evict{false}; // This peer is preferred for eviction.
prefer_evict could also be removed from CNodeOptions.
894@@ -895,20 +895,7 @@ size_t CConnman::SocketSendData(CNode& node) const
895 */
896 bool CConnman::AttemptToEvictConnection()
897 {
898- std::vector<NodeEvictionCandidate> vEvictionCandidates;
899- {
900- LOCK(m_nodes_mutex);
901- for (const CNode* node : m_nodes) {
902- if (node->fDisconnect)
node->fDisconnect, which I think could be dangerous:
We set fDisconnect in AttemptToEvictConnection(), but don’t immediately disconnect. The actual cleanup happens at some later point, when ThreadSocketHandler calls DisconnectNodes().
So if we have an attacker that makes new connections to us rapidly, there would be a race between ThreadSocketHandler and the attacker making the next connection to us. If the attacker wins this race, we could accept two inbound connections, but, without the check that is being removed here, AttemptToEvictConnection() could mark the same inbound peer for eviction twice, so that we’ll actually only evict one peer. Since inbound eviction is triggered only in CreateNodeFromAcceptedSocket() (and not through some regularly scheduled job), this could lead to us having an extra inbound peer above our limit indefinitely.
Now if the attacker repeats this scheme many times, they could make thousands of inbound connections to us / exhaust our resources / crash our node.
ThreadSocketHandler - after CConnman::SocketHandler() -> CConnman::SocketHandlerListening(), CConnman::DisconnectNodes() is called, so it’s not possible to accept new connections in between - however, CConnman::SocketHandlerListening() calls AcceptConnection() in a loop for multiple ListenSockets, so if we have multiple listening sockets and an attacker would connect to more than one at the same time, the scenario above may still be possible?
We temporarily introduce a method to get a specific
NodeEvictionCandidate from the eviction manager for use in
`CConnman::AttemptToEvictConnection()`. `EvictionManager::GetCandidate`
is removed once we are done moving the remaining eviction state from
`CNode` to the eviction manager.
Now that the eviction manager is aware of the keyed net group and
whether or not a peer is prefered for eviction, we can get rid of the
corresponding CNode members, since they are not used elsewhere.
🐙 This pull request conflicts with the target branch and needs rebase.
