From 00e11e61c0211a62788611cd6a6714a393fdc26c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 22 Jan 2019 15:20:34 -0500 Subject: [refactor] rename stateDummy -> orphan_state Co-authored-by: Anthony Towns Suhas Daftuar --- src/net_processing.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'src/net_processing.cpp') diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 74e33189d..7f3af6804 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1730,10 +1730,10 @@ void static ProcessOrphanTx(CConnman* connman, std::set& orphan_work_se // Use a dummy CValidationState so someone can't setup nodes to counter-DoS based on orphan // resolution (that is, feeding people an invalid transaction based on LegitTxX in order to get // anyone relaying LegitTxX banned) - CValidationState stateDummy; + CValidationState orphan_state; if (setMisbehaving.count(fromPeer)) continue; - if (AcceptToMemoryPool(mempool, stateDummy, porphanTx, &fMissingInputs2, &removed_txn, false /* bypass_limits */, 0 /* nAbsurdFee */)) { + if (AcceptToMemoryPool(mempool, orphan_state, porphanTx, &fMissingInputs2, &removed_txn, false /* bypass_limits */, 0 /* nAbsurdFee */)) { LogPrint(BCLog::MEMPOOL, " accepted orphan tx %s\n", orphanHash.ToString()); RelayTransaction(orphanTx, connman); for (unsigned int i = 0; i < orphanTx.vout.size(); i++) { @@ -1748,7 +1748,7 @@ void static ProcessOrphanTx(CConnman* connman, std::set& orphan_work_se done = true; } else if (!fMissingInputs2) { int nDos = 0; - if (stateDummy.IsInvalid(nDos) && nDos > 0) { + if (orphan_state.IsInvalid(nDos) && nDos > 0) { // Punish peer that gave us an invalid orphan tx Misbehaving(fromPeer, nDos); setMisbehaving.insert(fromPeer); @@ -1757,7 +1757,7 @@ void static ProcessOrphanTx(CConnman* connman, std::set& orphan_work_se // Has inputs but not accepted to mempool // Probably non-standard or insufficient fee LogPrint(BCLog::MEMPOOL, " removed orphan tx %s\n", orphanHash.ToString()); - if (!orphanTx.HasWitness() && !stateDummy.CorruptionPossible()) { + if (!orphanTx.HasWitness() && !orphan_state.CorruptionPossible()) { // Do not use rejection cache for witness transactions or // witness-stripped transactions, as they can have been malleated. // See https://github.com/bitcoin/bitcoin/issues/8279 for details. -- cgit v1.2.3 From 8818729013e17c650a25f030b2b80e0997389155 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 16 Apr 2018 12:52:03 -0400 Subject: [refactor] Refactor misbehavior ban decisions to MaybePunishNode() 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 Suhas Daftuar John Newbery --- src/net_processing.cpp | 91 +++++++++++++++++++++++++++++++------------------- 1 file changed, 57 insertions(+), 34 deletions(-) (limited to 'src/net_processing.cpp') diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 7f3af6804..a416093db 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -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); } +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()); std::map>::iterator it = mapBlockSource.find(hash); - int nDoS = 0; - if (state.IsInvalid(nDoS)) { + 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) { CBlockReject reject = {(unsigned char)state.GetRejectCode(), state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), hash}; State(it->second.first)->rejects.push_back(reject); - if (nDoS > 0 && it->second.second) - Misbehaving(it->second.first, nDoS); + MaybePunishNode(/*nodeid=*/ it->second.first, state, /*via_compact_block=*/ !it->second.second); } } // Check that: @@ -1551,14 +1577,7 @@ bool static ProcessHeadersMessage(CNode *pfrom, CConnman *connman, const std::ve CValidationState state; CBlockHeader first_invalid_header; if (!ProcessNewBlockHeaders(headers, state, chainparams, &pindexLast, &first_invalid_header)) { - int nDoS; - 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 (state.IsInvalid()) { if (punish_duplicate_invalid && LookupBlockIndex(first_invalid_header.GetHash())) { // Goal: don't allow outbound peers to use up our outbound // 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. pfrom->fDisconnect = true; } + MaybePunishNode(pfrom->GetId(), state, /*via_compact_block*/ false, "invalid header received"); return false; } } @@ -1727,9 +1747,9 @@ void static ProcessOrphanTx(CConnman* connman, std::set& orphan_work_se const CTransaction& orphanTx = *porphanTx; NodeId fromPeer = orphan_it->second.fromPeer; bool fMissingInputs2 = false; - // Use a dummy CValidationState so someone can't setup nodes to counter-DoS based on orphan - // resolution (that is, feeding people an invalid transaction based on LegitTxX in order to get - // anyone relaying LegitTxX banned) + // Use a new CValidationState because orphans come from different peers (and we call + // MaybePunishNode based on the source peer from the orphan map, not based on the peer + // that relayed the previous transaction). CValidationState orphan_state; if (setMisbehaving.count(fromPeer)) continue; @@ -1747,11 +1767,11 @@ void static ProcessOrphanTx(CConnman* connman, std::set& orphan_work_se EraseOrphanTx(orphanHash); done = true; } else if (!fMissingInputs2) { - int nDos = 0; - if (orphan_state.IsInvalid(nDos) && nDos > 0) { + if (orphan_state.IsInvalid()) { // Punish peer that gave us an invalid orphan tx - Misbehaving(fromPeer, nDos); - setMisbehaving.insert(fromPeer); + if (MaybePunishNode(fromPeer, orphan_state, /*via_compact_block*/ false)) { + setMisbehaving.insert(fromPeer); + } LogPrint(BCLog::MEMPOOL, " invalid orphan tx %s\n", orphanHash.ToString()); } // 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 // score for, as we expect peers to do the same with us in that // case. - int nDoS = 0; - if (!state.IsInvalid(nDoS) || nDoS == 0) { + if (!state.IsInvalid() || !TxRelayMayResultInDisconnect(state)) { LogPrintf("Force relaying tx %s from whitelisted peer=%d\n", tx.GetHash().ToString(), pfrom->GetId()); RelayTransaction(tx, connman); } 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, // regardless of false positives. - int nDoS = 0; - if (state.IsInvalid(nDoS)) + if (state.IsInvalid()) { LogPrint(BCLog::MEMPOOLREJ, "%s from peer=%d was not accepted: %s\n", tx.GetHash().ToString(), 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(), state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), inv.hash)); } - if (nDoS > 0) { - Misbehaving(pfrom->GetId(), nDoS); - } + MaybePunishNode(pfrom->GetId(), state, /*via_compact_block*/ false); } return true; } @@ -2574,14 +2590,21 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr const CBlockIndex *pindex = nullptr; CValidationState state; if (!ProcessNewBlockHeaders({cmpctblock.header}, state, chainparams, &pindex)) { - int nDoS; - if (state.IsInvalid(nDoS)) { - if (nDoS > 0) { - LOCK(cs_main); - Misbehaving(pfrom->GetId(), nDoS, strprintf("Peer %d sent us invalid header via cmpctblock\n", pfrom->GetId())); - } else { - LogPrint(BCLog::NET, "Peer %d sent us invalid header via cmpctblock\n", pfrom->GetId()); - } + if (state.IsInvalid() && received_new_header) { + // In this situation, the block header is known to be invalid. + // If we never created a CBlockIndex entry for it, then the + // header must be bad just by inspection (and is not one that + // looked okay but the block later turned out to be invalid for + // some other reason). + // 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; } } -- cgit v1.2.3 From 34477ccd39a8d4bfa8ad612f22d5a46291922185 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 16 Jan 2019 13:11:13 +1000 Subject: [refactor] Add useful-for-dos "reason" field to CValidationState This is a first step towards cleaning up our DoS interface - make validation return *why* something is invalid, and let net_processing figure out what that implies in terms of banning/disconnection/etc. Behavior change: peers will now be banned for providing blocks with premature coinbase spends. Co-authored-by: Anthony Towns Suhas Daftuar --- src/net_processing.cpp | 2 ++ 1 file changed, 2 insertions(+) (limited to 'src/net_processing.cpp') diff --git a/src/net_processing.cpp b/src/net_processing.cpp index a416093db..489ffcdc6 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -961,6 +961,7 @@ void Misbehaving(NodeId pnode, int howmuch, const std::string& message) EXCLUSIV static bool TxRelayMayResultInDisconnect(const CValidationState& state) { + assert(state.GetDoS() == state.GetDoSForReason()); return (state.GetDoS() > 0); } @@ -975,6 +976,7 @@ static bool TxRelayMayResultInDisconnect(const CValidationState& state) * 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 = "") { + assert(state.GetDoS() == state.GetDoSForReason()); int nDoS = state.GetDoS(); if (nDoS > 0 && !via_compact_block) { LOCK(cs_main); -- cgit v1.2.3 From c8b0d22698385f91215ce8145631e3d5826dc977 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 18 Jan 2019 12:58:41 +1000 Subject: [refactor] Drop redundant nDoS, corruptionPossible, SetCorruptionPossible Co-authored-by: Anthony Towns --- src/net_processing.cpp | 2 -- 1 file changed, 2 deletions(-) (limited to 'src/net_processing.cpp') diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 489ffcdc6..a416093db 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -961,7 +961,6 @@ void Misbehaving(NodeId pnode, int howmuch, const std::string& message) EXCLUSIV static bool TxRelayMayResultInDisconnect(const CValidationState& state) { - assert(state.GetDoS() == state.GetDoSForReason()); return (state.GetDoS() > 0); } @@ -976,7 +975,6 @@ static bool TxRelayMayResultInDisconnect(const CValidationState& state) * 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 = "") { - assert(state.GetDoS() == state.GetDoSForReason()); int nDoS = state.GetDoS(); if (nDoS > 0 && !via_compact_block) { LOCK(cs_main); -- cgit v1.2.3 From 7df16e70e67c753c871797ce947ea09d7cb0e519 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 17 Jan 2019 14:33:16 +1000 Subject: LookupBlockIndex -> CACHED_INVALID Co-authored-by: Anthony Towns --- src/net_processing.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src/net_processing.cpp') diff --git a/src/net_processing.cpp b/src/net_processing.cpp index a416093db..5c5faa130 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1578,7 +1578,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 && LookupBlockIndex(first_invalid_header.GetHash())) { + 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. // -- cgit v1.2.3 From 6e55b292b0ea944897b6dc2f766446fd209af484 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 17 Jan 2019 14:35:01 +1000 Subject: CorruptionPossible -> TX_WITNESS_MUTATED Co-authored-by: Anthony Towns --- src/net_processing.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'src/net_processing.cpp') diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 5c5faa130..99c791dae 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1777,7 +1777,7 @@ void static ProcessOrphanTx(CConnman* connman, std::set& orphan_work_se // Has inputs but not accepted to mempool // Probably non-standard or insufficient fee LogPrint(BCLog::MEMPOOL, " removed orphan tx %s\n", orphanHash.ToString()); - if (!orphanTx.HasWitness() && !orphan_state.CorruptionPossible()) { + if (!orphanTx.HasWitness() && orphan_state.GetReason() != ValidationInvalidReason::TX_WITNESS_MUTATED) { // Do not use rejection cache for witness transactions or // witness-stripped transactions, as they can have been malleated. // See https://github.com/bitcoin/bitcoin/issues/8279 for details. @@ -2494,7 +2494,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr recentRejects->insert(tx.GetHash()); } } else { - if (!tx.HasWitness() && !state.CorruptionPossible()) { + if (!tx.HasWitness() && state.GetReason() != ValidationInvalidReason::TX_WITNESS_MUTATED) { // Do not use rejection cache for witness transactions or // witness-stripped transactions, as they can have been malleated. // See https://github.com/bitcoin/bitcoin/issues/8279 for details. -- cgit v1.2.3 From ef54b486d5333dfc85c56e6b933c81735196a25d Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 8 Nov 2017 11:57:38 -0500 Subject: [refactor] Use Reasons directly instead of DoS codes --- src/net_processing.cpp | 87 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 57 insertions(+), 30 deletions(-) (limited to 'src/net_processing.cpp') diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 99c791dae..0a78ad47e 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -959,27 +959,68 @@ 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); } +/** + * Returns true if the given validation state result may result in a peer + * banning/disconnecting us. We use this to determine which unaccepted + * transactions from a whitelisted peer that we can safely relay. + */ static bool TxRelayMayResultInDisconnect(const CValidationState& state) { - return (state.GetDoS() > 0); + return state.GetReason() == ValidationInvalidReason::CONSENSUS; } /** * 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 + * @param[in] 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. + * + * @return Returns true if the peer was punished (probably disconnected) + * + * Changes here may need to be reflected in TxRelayMayResultInDisconnect(). */ 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; + switch (state.GetReason()) { + case ValidationInvalidReason::NONE: + break; + // The node is providing invalid data: + case ValidationInvalidReason::CONSENSUS: + case ValidationInvalidReason::BLOCK_MUTATED: + if (!via_compact_block) { + LOCK(cs_main); + Misbehaving(nodeid, 100, message); + return true; + } + break; + // Handled elsewhere for now + case ValidationInvalidReason::CACHED_INVALID: + break; + case ValidationInvalidReason::BLOCK_INVALID_HEADER: + case ValidationInvalidReason::BLOCK_CHECKPOINT: + case ValidationInvalidReason::BLOCK_INVALID_PREV: + { + LOCK(cs_main); + Misbehaving(nodeid, 100, message); + } + return true; + // Conflicting (but not necessarily invalid) data or different policy: + case ValidationInvalidReason::BLOCK_MISSING_PREV: + { + // TODO: Handle this much more gracefully (10 DoS points is super arbitrary) + LOCK(cs_main); + Misbehaving(nodeid, 10, message); + } + return true; + case ValidationInvalidReason::RECENT_CONSENSUS_CHANGE: + case ValidationInvalidReason::BLOCK_TIME_FUTURE: + case ValidationInvalidReason::TX_NOT_STANDARD: + case ValidationInvalidReason::TX_MISSING_INPUTS: + case ValidationInvalidReason::TX_WITNESS_MUTATED: + case ValidationInvalidReason::TX_CONFLICT: + case ValidationInvalidReason::TX_MEMPOOL_POLICY: + break; } if (message != "") { LogPrint(BCLog::NET, "peer=%d: %s\n", nodeid, message); @@ -2513,14 +2554,13 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr // to policy, allowing the node to function as a gateway for // nodes hidden behind it. // - // 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 - // case. - if (!state.IsInvalid() || !TxRelayMayResultInDisconnect(state)) { + // Never relay transactions that might result in being + // disconnected (or banned). + if (state.IsInvalid() && TxRelayMayResultInDisconnect(state)) { + LogPrintf("Not relaying invalid transaction %s from whitelisted peer=%d (%s)\n", tx.GetHash().ToString(), pfrom->GetId(), FormatStateMessage(state)); + } else { LogPrintf("Force relaying tx %s from whitelisted peer=%d\n", tx.GetHash().ToString(), pfrom->GetId()); RelayTransaction(tx, connman); - } else { - LogPrintf("Not relaying invalid transaction %s from whitelisted peer=%d (%s)\n", tx.GetHash().ToString(), pfrom->GetId(), FormatStateMessage(state)); } } } @@ -2590,21 +2630,8 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr const CBlockIndex *pindex = nullptr; CValidationState state; if (!ProcessNewBlockHeaders({cmpctblock.header}, state, chainparams, &pindex)) { - if (state.IsInvalid() && received_new_header) { - // In this situation, the block header is known to be invalid. - // If we never created a CBlockIndex entry for it, then the - // header must be bad just by inspection (and is not one that - // looked okay but the block later turned out to be invalid for - // some other reason). - // 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"); + if (state.IsInvalid()) { + MaybePunishNode(pfrom->GetId(), state, /*via_compact_block*/ true, "invalid header via cmpctblock"); return true; } } -- cgit v1.2.3 From 6b34bc6b6f54f85537494cbea3846d5d195a06d9 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Tue, 15 Jan 2019 15:54:04 -0500 Subject: 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(-) (limited to 'src/net_processing.cpp') diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 0a78ad47e..c19befcf8 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) -- cgit v1.2.3 From 54470e767bab37f9b7089782b1be73d5883bb244 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Thu, 21 Feb 2019 13:46:25 -0500 Subject: Assert validation reasons are contextually correct --- src/net_processing.cpp | 3 +++ 1 file changed, 3 insertions(+) (limited to 'src/net_processing.cpp') diff --git a/src/net_processing.cpp b/src/net_processing.cpp index c19befcf8..3319d3211 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -975,6 +975,7 @@ void Misbehaving(NodeId pnode, int howmuch, const std::string& message) EXCLUSIV */ static bool TxRelayMayResultInDisconnect(const CValidationState& state) { + assert(IsTransactionReason(state.GetReason())); return state.GetReason() == ValidationInvalidReason::CONSENSUS; } @@ -1806,6 +1807,7 @@ void static ProcessOrphanTx(CConnman* connman, std::set& orphan_work_se // Has inputs but not accepted to mempool // Probably non-standard or insufficient fee LogPrint(BCLog::MEMPOOL, " removed orphan tx %s\n", orphanHash.ToString()); + assert(IsTransactionReason(orphan_state.GetReason())); if (!orphanTx.HasWitness() && orphan_state.GetReason() != ValidationInvalidReason::TX_WITNESS_MUTATED) { // Do not use rejection cache for witness transactions or // witness-stripped transactions, as they can have been malleated. @@ -2523,6 +2525,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr recentRejects->insert(tx.GetHash()); } } else { + assert(IsTransactionReason(state.GetReason())); if (!tx.HasWitness() && state.GetReason() != ValidationInvalidReason::TX_WITNESS_MUTATED) { // Do not use rejection cache for witness transactions or // witness-stripped transactions, as they can have been malleated. -- cgit v1.2.3 From 0ff1c2a838da9e8dc7f77609adc89124bbea3e2b Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Fri, 8 Mar 2019 09:55:23 -0500 Subject: Separate reason for premature spends (coinbase/locktime) --- src/net_processing.cpp | 1 + 1 file changed, 1 insertion(+) (limited to 'src/net_processing.cpp') diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 3319d3211..71ebd72b8 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1040,6 +1040,7 @@ static bool MaybePunishNode(NodeId nodeid, const CValidationState& state, bool v case ValidationInvalidReason::BLOCK_TIME_FUTURE: case ValidationInvalidReason::TX_NOT_STANDARD: case ValidationInvalidReason::TX_MISSING_INPUTS: + case ValidationInvalidReason::TX_PREMATURE_SPEND: case ValidationInvalidReason::TX_WITNESS_MUTATED: case ValidationInvalidReason::TX_CONFLICT: case ValidationInvalidReason::TX_MEMPOOL_POLICY: -- cgit v1.2.3