refactor: Pass Peer& to Misbehaving()

MarcoFalke commented at 2:24 pm on May 16, 2022: member

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.

fanquake added the label Refactoring on May 16, 2022

MarcoFalke force-pushed on May 16, 2022

in src/net_processing.cpp:1525 in fa825d1903 outdated

1487     switch (state.GetResult()) {
1488     case TxValidationResult::TX_RESULT_UNSET:
1489         break;
1490     // The node is providing invalid data:
1491     case TxValidationResult::TX_CONSENSUS:
1492-        Misbehaving(nodeid, 100, message);

ajtowns commented at 3:09 pm on May 16, 2022:

Seems like the MaybePunish* calls could accept a Peer& (and CNodeState&) as well, and also not be called when the peer is already disconnected?

MarcoFalke commented at 3:45 pm on May 16, 2022:

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.

DrahtBot commented at 7:52 pm on May 16, 2022: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25454 (p2p, refactor: Avoid multiple getheaders messages in flight to the same peer by sdaftuar)
#25321 (p2p: Stop treating message size as misbehavior by MarcoFalke)
#24571 (p2p: Prevent block index fingerprinting by sending additional getheaders messages by dergoegge)

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.

DrahtBot added the label Needs rebase on May 19, 2022

in src/net_processing.cpp:489 in fa825d1903 outdated

481@@ -482,6 +482,12 @@ class PeerManagerImpl final : public PeerManager
482      *  May return an empty shared_ptr if the Peer object can't be found. */
483     PeerRef RemovePeer(NodeId id) EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
484 
485+    /**
486+     * Increment peer's misbehavior score. If the new value >= DISCOURAGEMENT_THRESHOLD, mark the node
487+     * to be discouraged, meaning the peer might be disconnected and added to the discouragement filter.
488+     */
489+    void Misbehaving(Peer& peer, const int howmuch, const std::string& message);

vasild commented at 12:23 pm on May 20, 2022:

nit:

0    void Misbehaving(Peer& peer, int howmuch, const std::string& message);

MarcoFalke commented at 3:01 pm on May 27, 2022:

thx, removed.

vasild approved

vasild commented at 12:44 pm on May 20, 2022: member

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.

Pass Peer& to Misbehaving()

`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.

fa8aa0aa81

MarcoFalke force-pushed on May 27, 2022

DrahtBot removed the label Needs rebase on May 27, 2022

vasild approved

vasild commented at 12:08 pm on June 2, 2022: member

ACK fa8aa0aa8180c3a0369c7589b8747666778a0deb

Slavo77AssMiner approved

in src/net_processing.h:75 in fa8aa0aa81

76-     * to be discouraged, meaning the peer might be disconnected and added to the discouragement filter.
77-     * Public for unit testing.
78-     */
79-    virtual void Misbehaving(const NodeId pnode, const int howmuch, const std::string& message) = 0;
80+    /* Public for unit testing. */
81+    virtual void UnitTestMisbehaving(NodeId peer_id, int howmuch) = 0;

laanwj commented at 9:14 pm on June 21, 2022:

Having a function in the API for unit testing is a bit strange to me, as it’s also there when not testing. I understand the whitebox testing idea, and don’t immediately know a better way of doing it, but it’s a pattern we imo should try to avoid

MarcoFalke commented at 5:43 am on June 22, 2022:

I don’t think there is any other way with the pimpl approach. Though, whenever I see a unit-test-only method in the “normal” interface, I like to prefix it with UnitTest... to make it absolutely obvious when someone is calling this from normal code.

It may be possible to remove by sending a mock violation, but I can look into this as a follow-up. (This pull doesn’t introduce the method, just renames it).

laanwj commented at 9:30 am on June 22, 2022:

Yes, fair enough. Agree with the naming convention, at least it discourages use in normal code. With LTO, it may even completely get optimized out.

laanwj commented at 9:18 pm on June 21, 2022: member

LGTM, otherwise, but did only cursory code review.

w0xlt approved

w0xlt commented at 10:43 pm on June 21, 2022: contributor

Code Review ACK https://github.com/bitcoin/bitcoin/pull/25144/commits/fa8aa0aa8180c3a0369c7589b8747666778a0deb

MarcoFalke merged this on Jun 27, 2022

MarcoFalke closed this on Jun 27, 2022

MarcoFalke deleted the branch on Jun 27, 2022

sidhujag referenced this in commit 65b8ac1ffd on Jun 27, 2022

DrahtBot locked this on Jun 27, 2023

refactor: Pass Peer& to Misbehaving() #25144

Conflicts