refactor: Introduce EvictionManager

dergoegge commented at 12:34 pm on June 2, 2022: member

At the moment, the eviction logic is mangled across two different components (CConnman, PeerManager), so we can’t really test it in isolation. This is not completely true for the inbound eviction logic as it exists as static functions in net.{h.cpp} for which tests already exist. However, the outbound eviction logic is not covered by any fuzz tests and is only testable by spinning up both a connman and peerman.

This PR splits out the eviction logic into its own component EvictionManager. In addition to isolating the eviction logic, we get rid of several layer violations (e.g. CConnman::ForEachNode/ForNode calls, CNode::m_last_block_time, etc.) between net and net processing.

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 to the eviction manager who ultimately suggests nodes to evict as the result of EvictionManager::SelectInboundNodeToEvict and EvictionManager::SelectOutboundNodesToEvict.

dergoegge force-pushed on Jun 2, 2022

DrahtBot added the label Refactoring on Jun 2, 2022

DrahtBot commented at 4:39 pm on June 2, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	jnewbery, theuni

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28222 (Use shared_ptr for CNode inside CConnman by willcl-ark)
#28170 (p2p: adaptive connections services flags by furszy)
#27912 (net: disconnect inside AttemptToEvictConnection by willcl-ark)
#27837 (net: introduce block tracker to retry to download blocks after failure by furszy)
#27600 (net: Add new permission forceinbound to evict a random unprotected connection if all slots are otherwise full by pinheadmz)
#27509 (Relay own transactions only via short-lived Tor or I2P connections by vasild)
#27385 (net, refactor: extract Network and BIP155Network logic to node/network by jonatack)
#27213 (p2p: Diversify automatic outbound connections with respect to networks by amitiuttarwar)
#26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)
#26621 (refactor: Continue moving application data from CNode to Peer by dergoegge)
#15218 (validation: Flush state after initial sync by andrewtoth)

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.

jnewbery commented at 10:20 am on June 3, 2022: contributor

Concept ACK. Very nice!

in src/net.cpp:19 in eac4a74f5e outdated

15@@ -16,6 +16,7 @@
16 #include <compat.h>
17 #include <consensus/consensus.h>
18 #include <crypto/sha256.h>
19+

jnewbery commented at 1:55 pm on June 3, 2022:

in def070810aca6da4fa603cfcc956975fef147346 [eviction] Create EvictionManager stub you’ve introduced an unnecessary newline.

jnewbery commented at 1:58 pm on June 3, 2022: contributor

Assuming this receives enough concept ACKs to move forwards, what do you think about splitting this into 3 PRs:

move eviction logic into its own translation unit (everything up to 0cc90971d9a1ba57681ece7761746bbe1259421e [net] Move eviction logic to its own file)
introduce EvictionManager and use for inbound eviction logic
use EvictionManager for outbound eviction logic.

dergoegge force-pushed on Jun 7, 2022

theuni commented at 9:18 pm on June 7, 2022: member

Concept ACK!

in src/net.h:194 in ef8edabe5e outdated

190-     */
191-    ADDR_FETCH,
192-};
193-
194-/** Convert ConnectionType enum to a string value */
195-std::string ConnectionTypeAsString(ConnectionType conn_type);

theuni commented at 7:18 pm on June 21, 2022:

Why not remove the old implementation here?

dergoegge commented at 4:03 pm on June 22, 2022:

Done.

in src/eviction.cpp:5 in a07eb1f403 outdated

0@@ -0,0 +1,227 @@
1+#include <eviction.h>

theuni commented at 7:18 pm on June 21, 2022:

Missing copyright stuff.

dergoegge commented at 4:03 pm on June 22, 2022:

Done.

in src/net.cpp:1010 in 1c805da562 outdated

1055@@ -1055,6 +1056,24 @@ void CConnman::CreateNodeFromAcceptedSocket(std::unique_ptr<Sock>&& sock,
1056         m_nodes.push_back(pnode);
1057     }
1058 
1059+    NodeEvictionCandidate candidate{
1060+        /*id=*/pnode->GetId(),
1061+        /*m_connected=*/pnode->m_connected,
1062+        /*m_min_ping_time=*/std::chrono::microseconds::max(),

theuni commented at 7:22 pm on June 21, 2022:

I think we need to justify these values for the sake of review.

For example, a default (unset) value for m_min_ping_time could reasonably also be 0, so it’s not clear in review why it’s set to max(). For values that have no real meaning until after version handshake, maybe std::optional would better communicate their state?

dergoegge commented at 4:19 pm on June 22, 2022:

I’m gonna try to slim down the NodeEvictionCandidate interface a bit because a lot of these values don’t need to be set by users of the eviction manager. They could just be default initialized internally in the eviction manager. I’m also gonna add some comments explaining the defaults.

I think std::optional would make it a bit to complicated but i’ll see maybe it works out nicely.

dergoegge commented at 5:40 pm on June 28, 2022:

I thought about using std::optional here but afaict we would end up treating a std::nullopt value for one of these in the same way that we treat them right now w.r.t. to the default values. The default values currently increase the likelihood of eviction for new peers (e.g. we prefer to disconnect peers with slow ping times, so the default is the max possible value) and i have added a comment to document that.

in src/net_processing.cpp:2633 in d8458fabaf outdated

2630@@ -2631,6 +2631,7 @@ void PeerManagerImpl::ProcessBlock(CNode& node, const std::shared_ptr<const CBlo
2631     m_chainman.ProcessNewBlock(block, force_processing, &new_block);
2632     if (new_block) {
2633         node.m_last_block_time = GetTime<std::chrono::seconds>();

theuni commented at 7:24 pm on June 21, 2022:

Please save a time and re-use it to avoid the slim chance of setting these to different values.

dergoegge commented at 4:04 pm on June 22, 2022:

Done.

in src/net_processing.cpp:4178 in 5101013ea6 outdated

3449@@ -3450,6 +3450,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3450             m_orphanage.AddChildrenToWorkSet(tx, peer->m_orphan_work_set);
3451 
3452             pfrom.m_last_tx_time = GetTime<std::chrono::seconds>();
3453+            m_evictionman.UpdateLastTxTime(pfrom.GetId(), GetTime<std::chrono::seconds>());

theuni commented at 7:25 pm on June 21, 2022:

Same thing here, use the same time.

in src/eviction.cpp:285 in 15fdf7ed93 outdated

281@@ -282,6 +282,14 @@ void EvictionManagerImpl::UpdateLoadedBloomFilter(NodeId id, bool bloom_filter_l
282     }
283 }
284 
285+void EvictionManagerImpl::UpdateRelayTxs(NodeId id, bool relay_txs)

theuni commented at 7:28 pm on June 21, 2022:

Since this can never become false again, I think we don’t need the bool?

That has the advantage of communicating that it can’t be unset.

dergoegge commented at 4:04 pm on June 22, 2022:

Done.

in src/net_processing.cpp:2776 in 15fdf7ed93 outdated

2770@@ -2771,7 +2771,10 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2771                 LOCK(tx_relay->m_bloom_filter_mutex);
2772                 tx_relay->m_relay_txs = fRelay; // set to true after we get the first filter* message
2773             }
2774-            if (fRelay) pfrom.m_relays_txs = true;
2775+            if (fRelay) {
2776+                pfrom.m_relays_txs = true;
2777+                m_evictionman.UpdateRelayTxs(pfrom.GetId(), fRelay);

theuni commented at 7:28 pm on June 21, 2022:

true would be more readable, but see above about dropping it entirely.

in src/eviction.cpp:309 in ba938cfd15 outdated

301@@ -302,6 +302,22 @@ void EvictionManagerImpl::UpdateRelayTxs(NodeId id, bool relay_txs)
302     }
303 }
304 
305+void EvictionManagerImpl::AddBlockInFlight(NodeId id)
306+{
307+    LOCK(m_candidates_mutex);
308+    if (const auto& it = m_candidates.find(id); it != m_candidates.end()) {
309+        ++it->second.m_blocks_in_flight;

theuni commented at 7:32 pm on June 21, 2022:

This is really confusing to read.

Instead of trying to convince myself that it’s correct, I’ll ask that you make it more obviously correct:it->second.m_blocks_in_flight++

dergoegge commented at 4:05 pm on June 22, 2022:

Good point. The dev docs recommend this way but in this case its obviously better to do foo++.

in src/eviction.cpp:317 in ba938cfd15 outdated

312+
313+void EvictionManagerImpl::RemoveBlockInFlight(NodeId id)
314+{
315+    LOCK(m_candidates_mutex);
316+    if (const auto& it = m_candidates.find(id); it != m_candidates.end()) {
317+        --it->second.m_blocks_in_flight;

theuni commented at 7:32 pm on June 21, 2022:

Here too.

in src/net_processing.cpp:1134 in ba938cfd15 outdated

962@@ -963,6 +963,7 @@ void PeerManagerImpl::RemoveBlockRequest(const uint256& hash)
963     state->vBlocksInFlight.erase(list_it);
964 
965     state->nBlocksInFlight--;
966+    m_evictionman.RemoveBlockInFlight(node_id);

theuni commented at 7:33 pm on June 21, 2022:

Since the value is checked in the next line, I assume we’re going to want RemoveBlockInFlight to return its new value.

dergoegge commented at 4:06 pm on June 22, 2022:

CNodeState::nBlocksInFlight is not removed in this PR so i think it would be confusing to also do the check with the value from the eviction manager. This is one of those cases that i didn’t like because the variables have to be kept in sync. (Arguably it’s pretty easy for this one since nBlocksInFlight is only modified in two places)

in src/net_processing.cpp:1188 in ba938cfd15 outdated

993@@ -993,6 +994,7 @@ bool PeerManagerImpl::BlockRequested(NodeId nodeid, const CBlockIndex& block, st
994     std::list<QueuedBlock>::iterator it = state->vBlocksInFlight.insert(state->vBlocksInFlight.end(),
995             {&block, std::unique_ptr<PartiallyDownloadedBlock>(pit ? new PartiallyDownloadedBlock(&m_mempool) : nullptr)});
996     state->nBlocksInFlight++;
997+    m_evictionman.AddBlockInFlight(nodeid);

theuni commented at 7:33 pm on June 21, 2022:

Same for AddBlockInFlight

in src/test/util/net.cpp:63 in ba938cfd15 outdated

59@@ -60,6 +60,7 @@ std::vector<NodeEvictionCandidate> GetRandomNodeEvictionCandidates(int n_candida
60             /*m_network=*/ALL_NETWORKS[random_context.randrange(ALL_NETWORKS.size())],
61             /*m_perm_flags=*/NetPermissionFlags::None,
62             /*m_conn_type=*/ConnectionType::INBOUND,
63+            /*m_blocks_in_flight=*/(int)random_context.randrange(5),

theuni commented at 7:33 pm on June 21, 2022:

I think MAX_BLOCKS_IN_TRANSIT_PER_PEER would be more reasonable here?

dergoegge commented at 4:06 pm on June 22, 2022:

Yes, i couldn’t use the MAX_BLOCKS_IN_TRANSIT_PER_PEER constant here but i changed to the right value and left a comment.

in src/net_processing.cpp:2270 in 534ae5dd17 outdated

2277@@ -2277,6 +2278,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, const Peer& peer,
2278 
2279         if (received_new_header && pindexLast->nChainWork > m_chainman.ActiveChain().Tip()->nChainWork) {
2280             nodestate->m_last_block_announcement = GetTime();
2281+            m_evictionman.UpdateLastBlockAnnounceTime(pfrom.GetId(), GetTime<std::chrono::seconds>());

theuni commented at 7:34 pm on June 21, 2022:

Please reuse GetTime.

in src/net_processing.cpp:4378 in 534ae5dd17 outdated

3658@@ -3657,6 +3659,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3659         // peer's last block announcement time
3660         if (received_new_header && pindex->nChainWork > m_chainman.ActiveChain().Tip()->nChainWork) {
3661             nodestate->m_last_block_announcement = GetTime();
3662+            m_evictionman.UpdateLastBlockAnnounceTime(pfrom.GetId(), GetTime<std::chrono::seconds>());

theuni commented at 7:35 pm on June 21, 2022:

Reuse GetTime here too.

in src/net_processing.cpp:2377 in 07732d03d0 outdated

2373@@ -2374,6 +2374,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, const Peer& peer,
2374             if (m_outbound_peers_with_protect_from_disconnect < MAX_OUTBOUND_PEERS_TO_PROTECT_FROM_DISCONNECT && nodestate->pindexBestKnownBlock->nChainWork >= m_chainman.ActiveChain().Tip()->nChainWork && !nodestate->m_chain_sync.m_protect) {
2375                 LogPrint(BCLog::NET, "Protecting outbound peer=%d from eviction\n", pfrom.GetId());
2376                 nodestate->m_chain_sync.m_protect = true;
2377+                m_evictionman.UpdateSlowChainProtected(pfrom.GetId(), true);

theuni commented at 7:37 pm on June 21, 2022:

Again, if this cannot become unset, I don’t think we need the bool param.

in src/net_processing.cpp:2926 in 8df3d9c5d7 outdated

2922@@ -2923,6 +2923,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2923             m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::SENDCMPCT, /*high_bandwidth=*/false, /*version=*/CMPCTBLOCKS_VERSION));
2924         }
2925         pfrom.fSuccessfullyConnected = true;
2926+        m_evictionman.UpdateSuccessfullyConnected(pfrom.GetId(), true);

theuni commented at 7:38 pm on June 21, 2022:

Another unneeded bool param.

in src/net.cpp:1115 in 1c805da562 outdated

1137@@ -1119,6 +1138,9 @@ void CConnman::DisconnectNodes()
1138                 // remove from m_nodes
1139                 m_nodes.erase(remove(m_nodes.begin(), m_nodes.end(), pnode), m_nodes.end());
1140 
1141+                // Tell the eviction manager to forget about this node
1142+                m_evictionman.RemoveCandidate(pnode->GetId());

theuni commented at 7:44 pm on June 21, 2022:

I think this could/should be done outside of m_nodes_mutex for the sake of untangling? I’m not actually sure, but if it wouldn’t break anything IMO it’d be preferable.

We’d need to create a list of nodes to be evicted, then do the removals outside of the lock.

dergoegge commented at 4:07 pm on June 22, 2022:

Done.

(You actually had it like that in your original branch and i changed it because it seemed simpler this way.)

in src/net_processing.cpp:4094 in 15fdf7ed93 outdated

4090@@ -4088,7 +4091,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4091                 tx_relay->m_relay_txs = true;
4092             }
4093             pfrom.m_bloom_filter_loaded = true;
4094+            pfrom.m_relays_txs = true;

theuni commented at 7:48 pm on June 21, 2022:

What’s this doing?

dergoegge commented at 4:07 pm on June 22, 2022:

Whoops forgot about this, it does not belong here (this line was accidentally removed in another PR). I opened a PR for that earlier today: #25446

in src/net_processing.cpp:1274 in 534ae5dd17 outdated

1268@@ -1269,8 +1269,9 @@ void PeerManagerImpl::AddTxAnnouncement(const CNode& node, const GenTxid& gtxid,
1269 void PeerManagerImpl::UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds)
1270 {
1271     LOCK(cs_main);
1272-    CNodeState *state = State(node);
1273+    CNodeState* state = State(node);
1274     if (state) state->m_last_block_announcement = time_in_seconds;
1275+    m_evictionman.UpdateLastBlockAnnounceTime(node, std::chrono::seconds{time_in_seconds});

theuni commented at 7:50 pm on June 21, 2022:

I think this can be done outside of cs_main ?

dergoegge commented at 4:07 pm on June 22, 2022:

PeerManagerImpl::UpdateLastBlockAnnounceTime is removed later on but i still went with your suggestion here for the commits before that.

theuni changes_requested

theuni commented at 7:58 pm on June 21, 2022: member

Thanks so much for working on this. I took a first pass through all commits except for the last few. I’ll need to give those another go with fresh eyes.

Some of these commits are mine, so several of these comments are probably aimed back at myself, but I’m not sparing myself from my review :)

I think we may need to restructure this PR a bit for the sake of readability. Again, I realize that you used my approach to the commit series, but now that I’m trying to review it, it’s not so easy to follow.

I think it may be much more reviewable if instead of adding all functionality to EvictionManager and then dropping the old stuff, they were done at the same time. So for ex in a commit that adds ping time to eviction manager, it should be hooked up and used, and the old logic dropped. Imo that would make this series much much easier to understand.

dergoegge force-pushed on Jun 22, 2022

dergoegge commented at 4:26 pm on June 22, 2022: member

Thanks for the review @theuni! I’ve addressed most of your comments (still working on #25268 (review) and restructuring the commits).

Do you also think it would be good to split this into multiple PRs, like John suggested #25268#pullrequestreview-994948655? I think that would also help with readability.

theuni commented at 5:50 pm on June 22, 2022: member

Do you also think it would be good to split this into multiple PRs, like John suggested #25268 (review)? I think that would also help with readability.

Yes, I agree with that. And in that case, as I mentioned above, I’d also request that the commits in the 2nd pull request would immediately use the new functionality as it’s added as opposed to adding it all, then switching it all in the end. To do that, I suspect that many of these current commits will need to be combined/dropped/re-worked. If that’s the case, don’t worry about git attribution or trying to use the original commits, I don’t care about that at all :)

DrahtBot added the label Needs rebase on Jun 23, 2022

jamesob commented at 5:23 pm on June 23, 2022: member

Is it possible to contextualize the benefit of this relatively large refactor? A lot of code is changing here, and the benefits listed in the PR description (resolution of layer violations) are somewhat abstract.

If this is, for example, more testable, wouldn’t it make sense to bundle the added test coverage with these changes so that potential reviewers are able to see the tangible benefit?

Is there some broader effort that this is part of?

dergoegge commented at 10:47 am on June 28, 2022: member

Is it possible to contextualize the benefit of this relatively large refactor?

I would say this PR is similar to #21148, #19988 or #787 in that it splits some logic (e.g. tx request, orphan handling) into its own component (some of the mentioned PRs also changed behavior which this PR does not), which is good for modularity and testability. At the moment, the eviction logic is mangled across two different components (CConnman, PeerManager), so we can’t really test it in isolation. This is not completely true for the inbound eviction logic as it exists as static functions in net but the outbound eviction logic is not covered by any fuzz tests and is only testable by spinning up both a connman and peerman.

I also think that isolating this logic will make it easier to reason about potential future changes to it.

The layer violation resolutions are more of a nice side effect and I should have mentioned/stressed the test/fuzz benefits in the PR description.

If this is, for example, more testable, wouldn’t it make sense to bundle the added test coverage with these changes so that potential reviewers are able to see the tangible benefit?

Good idea i will add some fuzz and isolated testing for the outbound eviction logic.

Is there some broader effort that this is part of?

Pinging @theuni for this question and to see if he has more to add here in general since this was originally his work.

dergoegge force-pushed on Jun 28, 2022

dergoegge commented at 5:58 pm on June 28, 2022: member

Rebased and restructured the commits a bit as suggested here: #25268#pullrequestreview-1014123554.

DrahtBot removed the label Needs rebase on Jun 28, 2022

dergoegge force-pushed on Jun 29, 2022

dergoegge force-pushed on Jun 30, 2022

dergoegge force-pushed on Jul 4, 2022

DrahtBot added the label Needs rebase on Jul 4, 2022

dergoegge force-pushed on Jul 5, 2022

DrahtBot removed the label Needs rebase on Jul 5, 2022

theuni commented at 2:42 pm on July 6, 2022: member

I think @dergoegge explained well, but I’ll add a bit on top.

The larger effort, which has been improving over the years, is the notion of self-contained subsystems. As it stands, in order to evict a peer, we need to gather the current states of several otherwise-unrelated things. Not only is this slow and error-prone (real-time locking for each individual state capture), it also makes the eviction logic impossible to reason about (and test) on its own.

So the solution/pattern that’s emerged to untangle similar problems in the past is: rather than doing real-time peeks into required subsystems every time we need to do something, instead broadcast the triggering events to a new subsystem that maintains a full view of whatever it needs. In practice, what this means is a producer/consumer pattern where multiple subsystems are notified when something happens, and it’s up to each to decide what to do with those changes. Yes, this may mean that several subsystems hold duplicate values for the same thing, but now they don’t have to cross over each-other for a full view.

Eventually these patterns could be formalized into something like the signals/slots we have elsewhere, which has the nice side-effect of defining triggers that other subsystems may be interested in as well. But as a first step, imo just untangling the layer violations is a worthy goal.

fanquake referenced this in commit d571cf2d24 on Jul 7, 2022

DrahtBot added the label Needs rebase on Jul 7, 2022

dergoegge force-pushed on Jul 8, 2022

DrahtBot removed the label Needs rebase on Jul 8, 2022

dergoegge commented at 4:41 pm on July 8, 2022: member

#25500 has been merged, the PR that splits off the next couple commits is #25572.

jnewbery commented at 10:42 am on July 13, 2022: contributor

.

DrahtBot added the label Needs rebase on Jul 13, 2022

dergoegge force-pushed on Jul 14, 2022

DrahtBot removed the label Needs rebase on Jul 14, 2022

DrahtBot added the label Needs rebase on Jul 19, 2022

dergoegge marked this as a draft on Oct 12, 2022

dergoegge force-pushed on Oct 28, 2022

DrahtBot removed the label Needs rebase on Oct 28, 2022

dergoegge force-pushed on Oct 28, 2022

DrahtBot added the label Needs rebase on Nov 2, 2022

dergoegge force-pushed on Nov 29, 2022

DrahtBot removed the label Needs rebase on Nov 29, 2022

dergoegge commented at 10:27 am on November 29, 2022: member

Rebased. This now also includes a fuzz target for EvictionManager, both inbound and outbound logic is being fuzzed.

DrahtBot added the label Needs rebase on Nov 30, 2022

dergoegge force-pushed on Dec 1, 2022

DrahtBot removed the label Needs rebase on Dec 1, 2022

dergoegge force-pushed on Dec 2, 2022

DrahtBot added the label Needs rebase on Dec 8, 2022

dergoegge force-pushed on Jan 24, 2023

DrahtBot removed the label Needs rebase on Jan 24, 2023

DrahtBot added the label Needs rebase on Mar 11, 2023

dergoegge force-pushed on Mar 15, 2023

DrahtBot removed the label Needs rebase on Mar 15, 2023

DrahtBot added the label Needs rebase on Mar 23, 2023

dergoegge force-pushed on Mar 27, 2023

DrahtBot removed the label Needs rebase on Mar 27, 2023

DrahtBot added the label Needs rebase on Mar 28, 2023

dergoegge force-pushed on Mar 28, 2023

DrahtBot removed the label Needs rebase on Mar 28, 2023

DrahtBot added the label Needs rebase on Apr 11, 2023

dergoegge force-pushed on Apr 12, 2023

DrahtBot removed the label Needs rebase on Apr 12, 2023

DrahtBot added the label Needs rebase on Apr 20, 2023

dergoegge force-pushed on Apr 21, 2023

DrahtBot removed the label Needs rebase on Apr 21, 2023

DrahtBot added the label Needs rebase on May 10, 2023

dergoegge force-pushed on May 30, 2023

DrahtBot added the label CI failed on May 30, 2023

DrahtBot removed the label Needs rebase on May 30, 2023

DrahtBot removed the label CI failed on May 31, 2023

DrahtBot added the label CI failed on Jul 15, 2023

dergoegge force-pushed on Jul 24, 2023

DrahtBot added the label Needs rebase on Jul 25, 2023

[test] Make all denial of service tests use their own connman/peerman e86cd1f977

dergoegge force-pushed on Jul 26, 2023

[eviction] Create EvictionManager stub 981617d338

[eviction] Make EvictionManager keep track of candidates 6de4364e8e

[net] Get candidates from eviction manager

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.

bc7e7fc60a

[net] Remove unused CNode state

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.

7f7d02cdb6

[eviction] Move ping/block/tx times from CNode to EvictionManager 157afa0b00

[eviction] Report services to EvictionManager 5a3ecac7c0

[eviction] Move BIP37 tx relay and bloom filter status to EvictionManager 98d484706e

[eviction] Move SelectNodeToEvict into EvictionManager 3bdaabd27d

[doc] Document relevant initial NodeEvictionCandidate values 6b53d0a2f5

[fuzz] Add EvictionManager target 2977ca3780

[eviction] Report number of blocks in flight to EvictionManager 3a459904ab

[eviction] Report block announce time to EvictionManager c9ebc0e51c

[eviction] Report nodes protected from slow chains to EvictionManager d006913263

[eviction] Report successful connections to EvictionManager 549a56f471

[eviction] Pass max outbound full/block relay connection counts to EvictionManager a583f630e9

[net processing] Move outbound eviction logic to EvictionManager e53cfc985b

[refactor] Cleanup EvictionManager::SelectOutboundNodesToEvict db72115eea

[refactor] Split internals of EvictionManager::SelectOutboundNodesToEvict into two functions 221f5aa429

scripted-diff: Rename SelectNodeToEvict for clarification

SelectNodeToEvict only suggests inbound nodes to evict.

-BEGIN VERIFY SCRIPT-
ren() { sed -i "s:\<$1\>:$2:g" $(git grep -l "\<$1\>" ./src ./test); }

ren SelectNodeToEvict SelectInboundNodeToEvict

-END VERIFY SCRIPT-

314b533ceb

[test] Replace DoS eviction tests with direct EvictionManger unit tests 62ccf8f80e

dergoegge force-pushed on Jul 26, 2023

DrahtBot removed the label Needs rebase on Jul 26, 2023

DrahtBot removed the label CI failed on Jul 26, 2023

DrahtBot added the label Needs rebase on Aug 6, 2023

DrahtBot commented at 5:48 pm on August 6, 2023: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

dergoegge closed this on Sep 28, 2023

refactor: Introduce EvictionManager #25268

Reviews

Conflicts