LBRYCommunity/lbrycrd
1
0
Fork 0
Code Issues 29 Pull requests 1 Projects Releases 32 Packages Wiki Activity

[refactor] Refactor misbehavior ban decisions to MaybePunishNode()

Browse source 
Isolate the decision of whether to ban a peer to one place in the
code, rather than having it sprinkled throughout net_processing.

Co-authored-by: Anthony Towns <aj@erisian.com.au>
                Suhas Daftuar <sdaftuar@gmail.com>
                John Newbery <john@johnnewbery.com>
This commit is contained in:
Matt Corallo 2018-04-16 12:52:03 -04:00 committed by Suhas Daftuar
parent 00e11e61c0
commit 8818729013
2 changed files with 58 additions and 34 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
src/consensus/validation.h
View file

@ -84,6 +84,7 @@ public:
void SetCorruptionPossible() { void SetCorruptionPossible() {
corruptionPossible = true; corruptionPossible = true;
} }
int GetDoS(void) const { return nDoS; }
unsigned int GetRejectCode() const { return chRejectCode; } unsigned int GetRejectCode() const { return chRejectCode; }
std::string GetRejectReason() const { return strRejectReason; } std::string GetRejectReason() const { return strRejectReason; }
std::string GetDebugMessage() const { return strDebugMessage; } std::string GetDebugMessage() const { return strDebugMessage; }

89
src/net_processing.cpp
View file

@ -959,6 +959,34 @@ void Misbehaving(NodeId pnode, int howmuch, const std::string& message) EXCLUSIV
LogPrint(BCLog::NET, "%s: %s peer=%d (%d -> %d)%s\n", __func__, state->name, pnode, state->nMisbehavior-howmuch, state->nMisbehavior, message_prefixed); LogPrint(BCLog::NET, "%s: %s peer=%d (%d -> %d)%s\n", __func__, state->name, pnode, state->nMisbehavior-howmuch, state->nMisbehavior, message_prefixed);
} }
static bool TxRelayMayResultInDisconnect(const CValidationState& state)
{
return (state.GetDoS() > 0);
}
/**
* Potentially ban a node based on the contents of a CValidationState object
* TODO: net_processing should make the punish decision based on the reason
* a tx/block was invalid, rather than just the nDoS score handed back by validation.
*
* @parameter via_compact_block: this bool is passed in because net_processing should
* punish peers differently depending on whether the data was provided in a compact
* block message or not. If the compact block had a valid header, but contained invalid
* txs, the peer should not be punished. See BIP 152.
*/
static bool MaybePunishNode(NodeId nodeid, const CValidationState& state, bool via_compact_block, const std::string& message = "") {
int nDoS = state.GetDoS();
if (nDoS > 0 && !via_compact_block) {
LOCK(cs_main);
Misbehaving(nodeid, nDoS, message);
return true;
}
if (message != "") {
LogPrint(BCLog::NET, "peer=%d: %s\n", nodeid, message);
}
return false;
}
@ -1132,14 +1160,12 @@ void PeerLogicValidation::BlockChecked(const CBlock& block, const CValidationSta
const uint256 hash(block.GetHash()); const uint256 hash(block.GetHash());
std::map<uint256, std::pair<NodeId, bool>>::iterator it = mapBlockSource.find(hash); std::map<uint256, std::pair<NodeId, bool>>::iterator it = mapBlockSource.find(hash);
int nDoS = 0; if (state.IsInvalid()) {
if (state.IsInvalid(nDoS)) {
// Don't send reject message with code 0 or an internal reject code. // Don't send reject message with code 0 or an internal reject code.
if (it != mapBlockSource.end() && State(it->second.first) && state.GetRejectCode() > 0 && state.GetRejectCode() < REJECT_INTERNAL) { if (it != mapBlockSource.end() && State(it->second.first) && state.GetRejectCode() > 0 && state.GetRejectCode() < REJECT_INTERNAL) {
CBlockReject reject = {(unsigned char)state.GetRejectCode(), state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), hash}; CBlockReject reject = {(unsigned char)state.GetRejectCode(), state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), hash};
State(it->second.first)->rejects.push_back(reject); State(it->second.first)->rejects.push_back(reject);
if (nDoS > 0 && it->second.second) MaybePunishNode(/*nodeid=*/ it->second.first, state, /*via_compact_block=*/ !it->second.second);
Misbehaving(it->second.first, nDoS);
} }
} }
// Check that: // Check that:
@ -1551,14 +1577,7 @@ bool static ProcessHeadersMessage(CNode *pfrom, CConnman *connman, const std::ve
CValidationState state; CValidationState state;
CBlockHeader first_invalid_header; CBlockHeader first_invalid_header;
if (!ProcessNewBlockHeaders(headers, state, chainparams, &pindexLast, &first_invalid_header)) { if (!ProcessNewBlockHeaders(headers, state, chainparams, &pindexLast, &first_invalid_header)) {
int nDoS; if (state.IsInvalid()) {
if (state.IsInvalid(nDoS)) {
LOCK(cs_main);
if (nDoS > 0) {
Misbehaving(pfrom->GetId(), nDoS, "invalid header received");
} else {
LogPrint(BCLog::NET, "peer=%d: invalid header received\n", pfrom->GetId());
}
if (punish_duplicate_invalid && LookupBlockIndex(first_invalid_header.GetHash())) { if (punish_duplicate_invalid && LookupBlockIndex(first_invalid_header.GetHash())) {
// Goal: don't allow outbound peers to use up our outbound // Goal: don't allow outbound peers to use up our outbound
// connection slots if they are on incompatible chains. // connection slots if they are on incompatible chains.
@ -1593,6 +1612,7 @@ bool static ProcessHeadersMessage(CNode *pfrom, CConnman *connman, const std::ve
// etc), and not just the duplicate-invalid case. // etc), and not just the duplicate-invalid case.
pfrom->fDisconnect = true; pfrom->fDisconnect = true;
} }
MaybePunishNode(pfrom->GetId(), state, /*via_compact_block*/ false, "invalid header received");
return false; return false;
} }
} }
@ -1727,9 +1747,9 @@ void static ProcessOrphanTx(CConnman* connman, std::set<uint256>& orphan_work_se
const CTransaction& orphanTx = *porphanTx; const CTransaction& orphanTx = *porphanTx;
NodeId fromPeer = orphan_it->second.fromPeer; NodeId fromPeer = orphan_it->second.fromPeer;
bool fMissingInputs2 = false; bool fMissingInputs2 = false;
// Use a dummy CValidationState so someone can't setup nodes to counter-DoS based on orphan // Use a new CValidationState because orphans come from different peers (and we call
// resolution (that is, feeding people an invalid transaction based on LegitTxX in order to get // MaybePunishNode based on the source peer from the orphan map, not based on the peer
// anyone relaying LegitTxX banned) // that relayed the previous transaction).
CValidationState orphan_state; CValidationState orphan_state;
if (setMisbehaving.count(fromPeer)) continue; if (setMisbehaving.count(fromPeer)) continue;
@ -1747,11 +1767,11 @@ void static ProcessOrphanTx(CConnman* connman, std::set<uint256>& orphan_work_se
EraseOrphanTx(orphanHash); EraseOrphanTx(orphanHash);
done = true; done = true;
} else if (!fMissingInputs2) { } else if (!fMissingInputs2) {
int nDos = 0; if (orphan_state.IsInvalid()) {
if (orphan_state.IsInvalid(nDos) && nDos > 0) {
// Punish peer that gave us an invalid orphan tx // Punish peer that gave us an invalid orphan tx
Misbehaving(fromPeer, nDos); if (MaybePunishNode(fromPeer, orphan_state, /*via_compact_block*/ false)) {
setMisbehaving.insert(fromPeer); setMisbehaving.insert(fromPeer);
}
LogPrint(BCLog::MEMPOOL, " invalid orphan tx %s\n", orphanHash.ToString()); LogPrint(BCLog::MEMPOOL, " invalid orphan tx %s\n", orphanHash.ToString());
} }
// Has inputs but not accepted to mempool // Has inputs but not accepted to mempool
@ -2496,8 +2516,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
// Never relay transactions that we would assign a non-zero DoS // Never relay transactions that we would assign a non-zero DoS
// score for, as we expect peers to do the same with us in that // score for, as we expect peers to do the same with us in that
// case. // case.
int nDoS = 0; if (!state.IsInvalid() || !TxRelayMayResultInDisconnect(state)) {
if (!state.IsInvalid(nDoS) || nDoS == 0) {
LogPrintf("Force relaying tx %s from whitelisted peer=%d\n", tx.GetHash().ToString(), pfrom->GetId()); LogPrintf("Force relaying tx %s from whitelisted peer=%d\n", tx.GetHash().ToString(), pfrom->GetId());
RelayTransaction(tx, connman); RelayTransaction(tx, connman);
} else { } else {
@ -2526,8 +2545,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
// peer simply for relaying a tx that our recentRejects has caught, // peer simply for relaying a tx that our recentRejects has caught,
// regardless of false positives. // regardless of false positives.
int nDoS = 0; if (state.IsInvalid())
if (state.IsInvalid(nDoS))
{ {
LogPrint(BCLog::MEMPOOLREJ, "%s from peer=%d was not accepted: %s\n", tx.GetHash().ToString(), LogPrint(BCLog::MEMPOOLREJ, "%s from peer=%d was not accepted: %s\n", tx.GetHash().ToString(),
pfrom->GetId(), pfrom->GetId(),
@ -2536,9 +2554,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::REJECT, strCommand, (unsigned char)state.GetRejectCode(), connman->PushMessage(pfrom, msgMaker.Make(NetMsgType::REJECT, strCommand, (unsigned char)state.GetRejectCode(),
state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), inv.hash)); state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), inv.hash));
} }
if (nDoS > 0) { MaybePunishNode(pfrom->GetId(), state, /*via_compact_block*/ false);
Misbehaving(pfrom->GetId(), nDoS);
}
} }
return true; return true;
} }
@ -2574,14 +2590,21 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
const CBlockIndex *pindex = nullptr; const CBlockIndex *pindex = nullptr;
CValidationState state; CValidationState state;
if (!ProcessNewBlockHeaders({cmpctblock.header}, state, chainparams, &pindex)) { if (!ProcessNewBlockHeaders({cmpctblock.header}, state, chainparams, &pindex)) {
int nDoS; if (state.IsInvalid() && received_new_header) {
if (state.IsInvalid(nDoS)) { // In this situation, the block header is known to be invalid.
if (nDoS > 0) { // If we never created a CBlockIndex entry for it, then the
LOCK(cs_main); // header must be bad just by inspection (and is not one that
Misbehaving(pfrom->GetId(), nDoS, strprintf("Peer %d sent us invalid header via cmpctblock\n", pfrom->GetId())); // looked okay but the block later turned out to be invalid for
} else { // some other reason).
LogPrint(BCLog::NET, "Peer %d sent us invalid header via cmpctblock\n", pfrom->GetId()); // We should punish compact block peers that give us an invalid
} // header (other than a "duplicate-invalid" one, see
// ProcessHeadersMessage), so set via_compact_block to false
// here.
// TODO: when we switch from DoS scores to reasons that
// tx/blocks are invalid, this call should set
// via_compact_block to true, since MaybePunishNode will have
// sufficient information to act correctly.
MaybePunishNode(pfrom->GetId(), state, /*via_compact_block*/ false, "invalid header via cmpctblock");
return true; return true;
} }
} }