refactor: Pass Peer& to Misbehaving()#25144
Hidden character warning
Conversation
There was a problem hiding this comment.
Seems like the MaybePunish* calls could accept a Peer& (and CNodeState&) as well, and also not be called when the peer is already disconnected?
There was a problem hiding this comment.
Yes, but only because the message happens to be empty() in cases where the peer might be nullptr/disconnected. I wasn't sure if we want to enforce this correlation.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
vasild
left a comment
vasild
left a comment
There was a problem hiding this comment.
ACK fa825d1903a3d7268104df33b7dcf1a5e685302d
The commit message is rather scarce. It is better if it contains all the info from the PR description. The latter is not part of the immutable git history and may be harder to access, especially if we move away from github at some point.
`Misbehaving` has several coding related issues (ignoring the conceptual issues here for now): * It is public, but it is not supposed to be called from outside of net_processing. Fix that by making it private and creating a public `UnitTestMisbehaving` method for unit testing only. * It doesn't do anything if a `nullptr` is passed. It would be less confusing to just skip the call instead. Fix that by passing `Peer&` to `Misbehaving()`. * It calls `GetPeerRef`, causing `!m_peer_mutex` lock annotations to be propagated. This is harmless, but verbose. Fix it by removing the no longer needed call to `GetPeerRef` and the no longer needed lock annotations.
|
LGTM, otherwise, but did only cursory code review. |
8 participants
Misbehavinghas several coding related issues (ignoring the conceptual issues here for now):UnitTestMisbehavingmethod for unit testing only.nullptris passed. It would be less confusing to just skip the call instead. Fix that by passingPeer&toMisbehaving().GetPeerRef, causing!m_peer_mutexlock annotations to be propagated. This is harmless, but verbose. Fix it by removing the no longer needed call toGetPeerRefand the no longer needed lock annotations.