maxmustermann/dogecoin
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

[net processing] Only call MaybeDiscourageAndDisconnect from SendMessages

Browse source 
`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.
This commit is contained in:
John Newbery 2020-06-15 09:28:56 -04:00
parent a1d5a428a2
commit a49781e56d
2 changed files with 6 additions and 7 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

11
src/net_processing.cpp
View file

@ -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<bool>& 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

2
src/net_processing.h
View file

@ -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);