From a1a07cfe99fc8cee30ba5976dc36b47b1f6532ab Mon Sep 17 00:00:00 2001 From: John Newbery Date: Mon, 30 Sep 2019 16:25:04 -0400 Subject: [validation] Fix peer punishment for bad blocks Because the call to MaybePunishNode() in PeerLogicValidation::BlockChecked() only previously happened if the REJECT code was > 0 and < REJECT_INTERNAL, then there are cases were MaybePunishNode() can get called where it wasn't previously: - when AcceptBlockHeader() fails with CACHED_INVALID. - when AcceptBlockHeader() fails with BLOCK_MISSING_PREV. Note that BlockChecked() cannot fail with an 'internal' reject code. The only internal reject code was REJECT_HIGHFEE, which was only set in ATMP. This change restores the behaviour pre-commit 5d08c9c579ba8cc7b684105c6a08263992b08d52 which did punish nodes that sent us CACHED_INVALID and BLOCK_MISSING_PREV blocks. --- src/net_processing.cpp | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) (limited to 'src/net_processing.cpp') diff --git a/src/net_processing.cpp b/src/net_processing.cpp index b6839dcf2..8d5cccc7f 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1234,11 +1234,12 @@ void PeerLogicValidation::BlockChecked(const CBlock& block, const CValidationSta const uint256 hash(block.GetHash()); std::map>::iterator it = mapBlockSource.find(hash); - if (state.IsInvalid()) { - // 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 the block failed validation, we know where it came from and we're still connected + // to that peer, maybe punish. + if (state.IsInvalid() && + it != mapBlockSource.end() && + State(it->second.first)) { MaybePunishNode(/*nodeid=*/ it->second.first, state, /*via_compact_block=*/ !it->second.second); - } } // Check that: // 1. The block is valid -- cgit v1.2.3 From 04a2f326ec0f06fb4fce1c4f93500752f05dede8 Mon Sep 17 00:00:00 2001 From: John Newbery Date: Thu, 10 Oct 2019 11:28:22 -0400 Subject: [validation] Fix REJECT message comments --- src/net_processing.cpp | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) (limited to 'src/net_processing.cpp') diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 8d5cccc7f..1425f091c 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -117,8 +117,8 @@ namespace { int nSyncStarted GUARDED_BY(cs_main) = 0; /** - * Sources of received blocks, saved to be able to send them reject - * messages or ban them when processing happens afterwards. + * Sources of received blocks, saved to be able punish them when processing + * happens afterwards. * Set mapBlockSource[hash].second to false if the node should not be * punished if the block is invalid. */ @@ -2861,11 +2861,12 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr // been run). This is handled below, so just treat this as // though the block was successfully read, and rely on the // handling in ProcessNewBlock to ensure the block index is - // updated, reject messages go out, etc. + // updated, etc. MarkBlockAsReceived(resp.blockhash); // it is now an empty pointer fBlockRead = true; - // mapBlockSource is only used for sending reject messages and DoS scores, - // so the race between here and cs_main in ProcessNewBlock is fine. + // mapBlockSource is used for potentially punishing peers and + // updating which peers send us compact blocks, so the race + // between here and cs_main in ProcessNewBlock is fine. // BIP 152 permits peers to relay compact blocks after validating // the header only; we should not punish peers if the block turns // out to be invalid. @@ -2937,8 +2938,9 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr // Also always process if we requested the block explicitly, as we may // need it even though it is not a candidate for a new best tip. forceProcessing |= MarkBlockAsReceived(hash); - // mapBlockSource is only used for sending reject messages and DoS scores, - // so the race between here and cs_main in ProcessNewBlock is fine. + // mapBlockSource is only used for punishing peers and setting + // which peers send us compact blocks, so the race between here and + // cs_main in ProcessNewBlock is fine. mapBlockSource.emplace(hash, std::make_pair(pfrom->GetId(), true)); } bool fNewBlock = false; -- cgit v1.2.3