phantom/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-01-25 18:53:23 -03:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

Merge #19472: [net processing] Reduce cs_main scope in MaybeDiscourageAndDisconnect()

Browse source 
655b195747 [net processing] Continue SendMessages processing if not disconnecting peer (John Newbery)
a49781e56d [net processing] Only call MaybeDiscourageAndDisconnect from SendMessages (John Newbery)
a1d5a428a2 [net processing] Fix bad indentation in SendMessages() (John Newbery)
1a1c23f8d4 [net processing] Change cs_main TRY_LOCK to LOCK in SendMessages() (John Newbery)

Pull request description:

  The motivation for this PR is to reduce the scope of cs_main locking in misbehavior logic. It is the first set of commits from a larger branch to move the misbehavior data out of CNodeState and into a new struct that doesn't take cs_main.

  There are some very minor behavior changes in this branch, such as:

  - Not checking for discouragement/disconnect in `ProcessMessages()` (and instead relying on the following check in `SendMessages()`)
  - Checking for discouragement/disconnect as the first action in `SendMessages()` (and not doing ping message sending first)
  - Continuing through `SendMessages()` if `MaybeDiscourageAndDisconnect()` doesn't disconnect the peer (rather than dropping out of `SendMessages()`

ACKs for top commit:
  jonatack:
    re-ACK 655b195 per `git range-diff 505b4ed f54af5e 655b195`, code/commit messages review, a bit of code history, and debug build.
  MarcoFalke:
    ACK 655b195747 only some style-nits 🚁
  promag:
    Code review ACK 655b195747.
  ariard:
    Code Review ACK 655b195

Tree-SHA512: fd6d7bc6bb789f5fb7771fb6a45f61a8faba32af93b766554f562144f9631d15c9cc849a383e71743ef73e610b4ee14853666f6fbf08a3ae35176d48c76c65d3
This commit is contained in:
Wladimir J. van der Laan 2020-07-24 17:09:15 +02:00
commit 40a04814d1
No known key found for this signature in database
GPG key ID: 1E4AED62986CD25D
2 changed files with 81 additions and 66 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

145
src/net_processing.cpp
View file

@ -3716,32 +3716,49 @@ void ProcessMessage(
return;
}
/** Maybe disconnect a peer and discourage future connections from its address.
*
* @param[in] pnode The node to check.
* @return True if the peer was marked for disconnection in this function
*/
bool PeerLogicValidation::MaybeDiscourageAndDisconnect(CNode& pnode)
{
AssertLockHeld(cs_main);
CNodeState &state = *State(pnode.GetId());
NodeId peer_id{pnode.GetId()};
{
LOCK(cs_main);
CNodeState &state = *State(peer_id);
if (state.m_should_discourage) {
// There's nothing to do if the m_should_discourage flag isn't set
if (!state.m_should_discourage) return false;
// Reset m_should_discourage
state.m_should_discourage = false;
if (pnode.HasPermission(PF_NOBAN)) {
LogPrintf("Warning: not punishing whitelisted peer %s!\n", pnode.addr.ToString());
} else if (pnode.m_manual_connection) {
LogPrintf("Warning: not punishing manually-connected peer %s!\n", pnode.addr.ToString());
} else if (pnode.addr.IsLocal()) {
// Disconnect but don't discourage this local node
LogPrintf("Warning: disconnecting but not discouraging local peer %s!\n", pnode.addr.ToString());
pnode.fDisconnect = true;
} else {
// Disconnect and discourage all nodes sharing the address
LogPrintf("Disconnecting and discouraging peer %s!\n", pnode.addr.ToString());
if (m_banman) {
m_banman->Discourage(pnode.addr);
}
connman->DisconnectNode(pnode.addr);
}
} // cs_main
if (pnode.HasPermission(PF_NOBAN)) {
// Peer has the NOBAN permission flag - log but don't disconnect
LogPrintf("Warning: not punishing noban peer %d!\n", peer_id);
return false;
}
if (pnode.m_manual_connection) {
// Peer is a manual connection - log but don't disconnect
LogPrintf("Warning: not punishing manually connected peer %d!\n", peer_id);
return false;
}
if (pnode.addr.IsLocal()) {
// Peer is on a local address. Disconnect this peer, but don't discourage the local address
LogPrintf("Warning: disconnecting but not discouraging local peer %d!\n", peer_id);
pnode.fDisconnect = true;
return true;
}
return false;
// Normal case: Disconnect the peer and discourage all nodes sharing the address
LogPrintf("Disconnecting and discouraging peer %d!\n", peer_id);
if (m_banman) m_banman->Discourage(pnode.addr);
connman->DisconnectNode(pnode.addr);
return true;
}
bool PeerLogicValidation::ProcessMessages(CNode* pfrom, std::atomic<bool>& interruptMsgProc)
@ -3834,9 +3851,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;
}
@ -3999,48 +4013,49 @@ public:
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;
// If we get here, the outgoing message serialization version is set and can't change.
const CNetMsgMaker msgMaker(pto->GetSendVersion());
//
// Message: ping
//
bool pingSend = false;
if (pto->fPingQueued) {
// RPC ping request by user
pingSend = true;
}
if (pto->nPingNonceSent == 0 && pto->m_ping_start.load() + PING_INTERVAL < GetTime<std::chrono::microseconds>()) {
// Ping automatically sent as a latency probe & keepalive.
pingSend = true;
}
if (pingSend) {
uint64_t nonce = 0;
while (nonce == 0) {
GetRandBytes((unsigned char*)&nonce, sizeof(nonce));
}
pto->fPingQueued = false;
pto->m_ping_start = GetTime<std::chrono::microseconds>();
if (pto->nVersion > BIP0031_VERSION) {
pto->nPingNonceSent = nonce;
connman->PushMessage(pto, msgMaker.Make(NetMsgType::PING, nonce));
} else {
// Peer is too old to support ping command with nonce, pong will never arrive.
pto->nPingNonceSent = 0;
connman->PushMessage(pto, msgMaker.Make(NetMsgType::PING));
}
}
{
// Don't send anything until the version handshake is complete
if (!pto->fSuccessfullyConnected || pto->fDisconnect)
return true;
// If we get here, the outgoing message serialization version is set and can't change.
const CNetMsgMaker msgMaker(pto->GetSendVersion());
//
// Message: ping
//
bool pingSend = false;
if (pto->fPingQueued) {
// RPC ping request by user
pingSend = true;
}
if (pto->nPingNonceSent == 0 && pto->m_ping_start.load() + PING_INTERVAL < GetTime<std::chrono::microseconds>()) {
// Ping automatically sent as a latency probe & keepalive.
pingSend = true;
}
if (pingSend) {
uint64_t nonce = 0;
while (nonce == 0) {
GetRandBytes((unsigned char*)&nonce, sizeof(nonce));
}
pto->fPingQueued = false;
pto->m_ping_start = GetTime<std::chrono::microseconds>();
if (pto->nVersion > BIP0031_VERSION) {
pto->nPingNonceSent = nonce;
connman->PushMessage(pto, msgMaker.Make(NetMsgType::PING, nonce));
} else {
// Peer is too old to support ping command with nonce, pong will never arrive.
pto->nPingNonceSent = 0;
connman->PushMessage(pto, msgMaker.Make(NetMsgType::PING));
}
}
TRY_LOCK(cs_main, lockMain);
if (!lockMain)
return true;
if (MaybeDiscourageAndDisconnect(*pto)) return true;
LOCK(cs_main);
CNodeState &state = *State(pto->GetId());
@ -4602,7 +4617,7 @@ bool PeerLogicValidation::SendMessages(CNode* pto)
pto->m_tx_relay->nextSendTimeFeeFilter = timeNow + GetRandInt(MAX_FEEFILTER_CHANGE_DELAY) * 1000000;
}
}
}
} // release cs_main
return true;
}

2
src/net_processing.h
View file

@ -34,7 +34,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);