refactor: Introduce EvictionManager #25268
Conversation
a55e25b to
eac4a74
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK. Very nice! |
There was a problem hiding this comment.
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 0cc9097 [net] Move eviction logic to its own file)
- introduce
EvictionManagerand use for inbound eviction logic - use
EvictionManagerfor outbound eviction logic.
eac4a74 to
767f41d
Compare
|
Concept ACK! |
There was a problem hiding this comment.
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.
src/net.cpp
Outdated
| NodeEvictionCandidate candidate{ | ||
| /*id=*/pnode->GetId(), | ||
| /*m_connected=*/pnode->m_connected, | ||
| /*m_min_ping_time=*/std::chrono::microseconds::max(), |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
767f41d to
43c8eb6
Compare
|
Thanks for the review @theuni! I've addressed most of your comments (still working on #25268 (comment) and restructuring the commits). 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 :) |
|
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? |
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 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.
Good idea i will add some fuzz and isolated testing for the outbound eviction logic.
Pinging @theuni for this question and to see if he has more to add here in general since this was originally his work. |
43c8eb6 to
397848b
Compare
|
Rebased and restructured the commits a bit as suggested here: #25268 (review). |
…vict into two functions
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-
0141606 to
62ccf8f
Compare
|
🐙 This pull request conflicts with the target branch and needs rebase. |
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 innet.{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/ForNodecalls,CNode::m_last_block_time, etc.) betweennetandnet processing.One instance of the
EvictionManageris 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 ofEvictionManager::SelectInboundNodeToEvictandEvictionManager::SelectOutboundNodesToEvict.