refactor: Pass Peer& to Misbehaving()
ClosedPublic
Actions

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

Diff Detail

Repository

rABC Bitcoin ABC

Branch

pr25454

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 25815
Build 51210: Build Diff	lint-circular-dependencies · build-clang · build-diff · build-debug · build-clang-tidy · build-without-wallet
Build 51209: arc lint + arc unit

Event Timeline

PiRK created this revision.Dec 4 2023, 15:41

Herald added a reviewer: Restricted Project. · View Herald TranscriptDec 4 2023, 15:41

PiRK requested review of this revision.Dec 4 2023, 15:41

teamcity edited the summary of this revision. (Show Details)Dec 4 2023, 15:41

PiRK added inline comments.Dec 4 2023, 15:51

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

PiRK added inline comments.Dec 4 2023, 15:53

src/net_processing.cpp
2239	So we are stuck with all the `if (peer) {...}` repetitions.

Harbormaster completed remote builds in B25815: Diff 43384.Dec 4 2023, 15:56

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

Closed by commit rABC070a9269782f: refactor: Pass Peer& to Misbehaving() (authored by PiRK). · Explain WhyDec 5 2023, 07:09

This revision was automatically updated to reflect the committed changes.

PiRK added a commit: rABC070a9269782f: refactor: Pass Peer& to Misbehaving().