Page MenuHomePhabricator

refactor: Pass Peer& to Misbehaving()
ClosedPublic

Authored by PiRK on Dec 4 2023, 15:41.

Details

Reviewers
bytesofman
Group Reviewers
Restricted Project
Commits
rABC070a9269782f: refactor: Pass Peer& to Misbehaving()
Summary

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.

This is a backport of core#25144

See D384 for why we already had an overloaded Mishbehaving for CNode

Test Plan

With clang and debug

ninja all check-all

Event Timeline

PiRK requested review of this revision.Dec 4 2023, 15:41
src/net_processing.cpp
2239

there was some discussion in the original review about whether MaybePunish* should just skip peers that are already disconnected (like ProcessMessage already does). It was decided not to do it because in theory it could still be useful to see the log message, if there were any messages in such a case. https://github.com/bitcoin/bitcoin/pull/25144#discussion_r873885597

src/net_processing.cpp
2239

So we are stuck with all the if (peer) {...} repetitions.

bytesofman added a subscriber: bytesofman.

Tests pass. Follows pattern of core backport with extension to avalanche features not present in core.

I'm not totally sure how to confirm that everything has been modified appropriately (i.e. all of the places where this needs to be changed, since we have more than core).

This revision is now accepted and ready to land.Dec 4 2023, 17:24
This revision was automatically updated to reflect the committed changes.