From a49781e56d2bd6a61ec027a09c1db9ee1a4abf2e Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 15 Jun 2020 09:28:56 -0400 Subject: [PATCH] [net processing] Only call MaybeDiscourageAndDisconnect from SendMessages `nMisbehavior` is a tally in `CNodeState` that can be incremented from anywhere. That almost always happens inside a `ProcessMessages()` call (because we increment the misbehavior score when receiving a bad messages from a peer), but not always. See, for example, the call to `MaybePunishNodeForBlock()` inside `BlockChecked()`, which is an asynchronous callback from the validation interface, executed on the scheduler thread. As long as `MaybeDiscourageAndDisconnect()` is called regularly for the node, then the misbehavior score exceeding the 100 threshold will eventually result in the peer being punished. It doesn't really matter where that `MaybeDiscourageAndDisconnect()` happens, but it makes most sense in `SendMessages()` which is where we do general peer housekeeping/maintenance. Therefore, remove the `MaybeDiscourageAndDisconnect()` call in `ProcessMessages()` and move the `MaybeDiscourageAndDisconnect()` call in `SendMessages()` to the top of the function. This moves it out of the cs_main lock scope, so take that lock directly inside `MaybeDiscourageAndDisconnect()`. Historic note: `MaybeDiscourageAndDisconnect()` was previously `SendRejectsAndCheckIfBanned()`, and before that was just sending rejects. All of those things required cs_main, which is why `MaybeDiscourageAndDisconnect()` was called after the ping logic. --- src/net_processing.cpp | 11 +++++------ src/net_processing.h | 2 +- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f8b22cd7c8c..570cca0c66a 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -3559,7 +3559,7 @@ void ProcessMessage( bool PeerLogicValidation::MaybeDiscourageAndDisconnect(CNode& pnode) { - AssertLockHeld(cs_main); + LOCK(cs_main); CNodeState &state = *State(pnode.GetId()); if (state.m_should_discourage) { @@ -3675,9 +3675,6 @@ bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic& inter LogPrint(BCLog::NET, "%s(%s, %u bytes): Unknown exception caught\n", __func__, SanitizeString(msg_type), nMessageSize); } - LOCK(cs_main); - MaybeDiscourageAndDisconnect(*pfrom); - return fMoreWork; } @@ -3839,6 +3836,10 @@ bool PeerLogicValidation::SendMessages(CNode* pto) { const Consensus::Params& consensusParams = Params().GetConsensus(); + // We must call MaybeDiscourageAndDisconnect first, to ensure that we'll + // disconnect misbehaving peers even before the version handshake is complete. + if (MaybeDiscourageAndDisconnect(*pto)) return true; + // Don't send anything until the version handshake is complete if (!pto->fSuccessfullyConnected || pto->fDisconnect) return true; @@ -3878,8 +3879,6 @@ bool PeerLogicValidation::SendMessages(CNode* pto) { LOCK(cs_main); - if (MaybeDiscourageAndDisconnect(*pto)) return true; - CNodeState &state = *State(pto->GetId()); // Address refresh broadcast diff --git a/src/net_processing.h b/src/net_processing.h index eadf29e59fc..48bcbc1a6c6 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -31,7 +31,7 @@ private: ChainstateManager& m_chainman; CTxMemPool& m_mempool; - bool MaybeDiscourageAndDisconnect(CNode& pnode) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + bool MaybeDiscourageAndDisconnect(CNode& pnode); public: PeerLogicValidation(CConnman* connman, BanMan* banman, CScheduler& scheduler, ChainstateManager& chainman, CTxMemPool& pool);