Refactor MaybeDiscourageAndDisconnect to accept individual parameters (has_no_ban, is_manual, is_local_addr, is_inbound_onion, peer_addr) instead of taking a CNode& reference directly.
This decouples the function from CNode while preserving all original logic paths and behavior.
This change is just a refactor, so no tests were modified
This is a part of #33958. Let me know if this is not needed and I can close this PR
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33983.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process. A summary of reviews will appear here.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Refactor MaybeDiscourageAndDisconnect to accept individual parameters (has_no_ban, is_manual, is_local_addr, is_inbound_onion, peer_addr)
instead of taking a CNode& reference directly.
This decouples the function from CNode while preserving all original logic paths and behavior.
4973 | + if (is_local_addr) { 4974 | // We disconnect local peers for bad behavior but don't discourage (since that would discourage 4975 | // all peers on the same local address) 4976 | LogDebug(BCLog::NET, "Warning: disconnecting but not discouraging %s peer %d!\n", 4977 | - pnode.m_inbound_onion ? "inbound onion" : "local", peer.m_id); 4978 | - pnode.fDisconnect = true;
This should not be removed
I added a should_disconnect parameter and after this function is called we do the following
if (should_disconnect) {
pto->fDisconnect = true;
}
return true;
In my opinion this is too fine grained of a step and not worth a PR on its own, as it will cause more churn down the line (e.g. the signature of MaybeDiscourageAndDisconnect will likely have to change again).
In my opinion this is too fine grained of a step and not worth a PR on its own, as it will cause more churn down the line (e.g. the signature of
MaybeDiscourageAndDisconnectwill likely have to change again).
Gotcha, should I attempt to decouple more/all functions in PeerManagerImpl. I was worried about attempting to make too large a PR, but I guess I went the other extreme, haha
Can you explain this a bit better?
An (ugly) initial POC can be seen here: https://github.com/theuni/bitcoin/tree/multiprocess_p2p. Note that it will likely not be rebased because it was not intended to be used as-is. Instead, chunks will be extracted and PR'd separately.
Did you cherry-pick a commit from this branch? If not, why not? If yes, why did the author change?
Can you explain this a bit better?
An (ugly) initial POC can be seen here: https://github.com/theuni/bitcoin/tree/multiprocess_p2p. Note that it will likely not be rebased because it was not intended to be used as-is. Instead, chunks will be extracted and PR'd separately.
Did you cherry-pick a commit from this branch? If not, why not? If yes, why did the author change?
I did not cherry-pick commits from this branch but now looking at it, I prefer the approach here instead. Will close this PR