From 6b34bc6b6f54f85537494cbea3846d5d195a06d9 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Tue, 15 Jan 2019 15:54:04 -0500 Subject: [PATCH] Fix handling of invalid headers We only disconnect outbound peers (excluding HB compact block peers and manual connections) when receiving a CACHED_INVALID header. --- src/net_processing.cpp | 77 ++++++++++++++++-------------------------- 1 file changed, 30 insertions(+), 47 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0a78ad47e0..c19befcf88 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -351,7 +351,16 @@ struct CNodeState { TxDownloadState m_tx_download; - CNodeState(CAddress addrIn, std::string addrNameIn) : address(addrIn), name(addrNameIn) { + //! Whether this peer is an inbound connection + bool m_is_inbound; + + //! Whether this peer is a manual connection + bool m_is_manual_connection; + + CNodeState(CAddress addrIn, std::string addrNameIn, bool is_inbound, bool is_manual) : + address(addrIn), name(std::move(addrNameIn)), m_is_inbound(is_inbound), + m_is_manual_connection (is_manual) + { fCurrentlyConnected = false; nMisbehavior = 0; fShouldBan = false; @@ -747,7 +756,7 @@ void PeerLogicValidation::InitializeNode(CNode *pnode) { NodeId nodeid = pnode->GetId(); { LOCK(cs_main); - mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(addr, std::move(addrName))); + mapNodeState.emplace_hint(mapNodeState.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(addr, std::move(addrName), pnode->fInbound, pnode->m_manual_connection)); } if(!pnode->fInbound) PushNodeVersion(pnode, connman, GetTime()); @@ -994,9 +1003,22 @@ static bool MaybePunishNode(NodeId nodeid, const CValidationState& state, bool v return true; } break; - // Handled elsewhere for now case ValidationInvalidReason::CACHED_INVALID: - break; + { + LOCK(cs_main); + CNodeState *node_state = State(nodeid); + if (node_state == nullptr) { + break; + } + + // Ban outbound (but not inbound) peers if on an invalid chain. + // Exempt HB compact block peers and manual connections. + if (!via_compact_block && !node_state->m_is_inbound && !node_state->m_is_manual_connection) { + Misbehaving(nodeid, 100, message); + return true; + } + break; + } case ValidationInvalidReason::BLOCK_INVALID_HEADER: case ValidationInvalidReason::BLOCK_CHECKPOINT: case ValidationInvalidReason::BLOCK_INVALID_PREV: @@ -1556,7 +1578,7 @@ inline void static SendBlockTransactions(const CBlock& block, const BlockTransac connman->PushMessage(pfrom, msgMaker.Make(nSendFlags, NetMsgType::BLOCKTXN, resp)); } -bool static ProcessHeadersMessage(CNode *pfrom, CConnman *connman, const std::vector& headers, const CChainParams& chainparams, bool punish_duplicate_invalid) +bool static ProcessHeadersMessage(CNode *pfrom, CConnman *connman, const std::vector& headers, const CChainParams& chainparams, bool via_compact_block) { const CNetMsgMaker msgMaker(pfrom->GetSendVersion()); size_t nCount = headers.size(); @@ -1619,41 +1641,7 @@ bool static ProcessHeadersMessage(CNode *pfrom, CConnman *connman, const std::ve CBlockHeader first_invalid_header; if (!ProcessNewBlockHeaders(headers, state, chainparams, &pindexLast, &first_invalid_header)) { if (state.IsInvalid()) { - if (punish_duplicate_invalid && state.GetReason() == ValidationInvalidReason::CACHED_INVALID) { - // Goal: don't allow outbound peers to use up our outbound - // connection slots if they are on incompatible chains. - // - // We ask the caller to set punish_invalid appropriately based - // on the peer and the method of header delivery (compact - // blocks are allowed to be invalid in some circumstances, - // under BIP 152). - // Here, we try to detect the narrow situation that we have a - // valid block header (ie it was valid at the time the header - // was received, and hence stored in mapBlockIndex) but know the - // block is invalid, and that a peer has announced that same - // block as being on its active chain. - // Disconnect the peer in such a situation. - // - // Note: if the header that is invalid was not accepted to our - // mapBlockIndex at all, that may also be grounds for - // disconnecting the peer, as the chain they are on is likely - // to be incompatible. However, there is a circumstance where - // that does not hold: if the header's timestamp is more than - // 2 hours ahead of our current time. In that case, the header - // may become valid in the future, and we don't want to - // disconnect a peer merely for serving us one too-far-ahead - // block header, to prevent an attacker from splitting the - // network by mining a block right at the 2 hour boundary. - // - // TODO: update the DoS logic (or, rather, rewrite the - // DoS-interface between validation and net_processing) so that - // the interface is cleaner, and so that we disconnect on all the - // reasons that a peer's headers chain is incompatible - // with ours (eg block->nVersion softforks, MTP violations, - // etc), and not just the duplicate-invalid case. - pfrom->fDisconnect = true; - } - MaybePunishNode(pfrom->GetId(), state, /*via_compact_block*/ false, "invalid header received"); + MaybePunishNode(pfrom->GetId(), state, via_compact_block, "invalid header received"); return false; } } @@ -2781,7 +2769,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr // the peer if the header turns out to be for an invalid block. // Note that if a peer tries to build on an invalid chain, that // will be detected and the peer will be banned. - return ProcessHeadersMessage(pfrom, connman, {cmpctblock.header}, chainparams, /*punish_duplicate_invalid=*/false); + return ProcessHeadersMessage(pfrom, connman, {cmpctblock.header}, chainparams, /*via_compact_block=*/true); } if (fBlockReconstructed) { @@ -2924,12 +2912,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr ReadCompactSize(vRecv); // ignore tx count; assume it is 0. } - // Headers received via a HEADERS message should be valid, and reflect - // the chain the peer is on. If we receive a known-invalid header, - // disconnect the peer if it is using one of our outbound connection - // slots. - bool should_punish = !pfrom->fInbound && !pfrom->m_manual_connection; - return ProcessHeadersMessage(pfrom, connman, headers, chainparams, should_punish); + return ProcessHeadersMessage(pfrom, connman, headers, chainparams, /*via_compact_block=*/false); } if (strCommand == NetMsgType::BLOCK)