From f34fa719cf33a51d11f1d2219cbe73ccff6fd697 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 18 Jan 2019 23:14:21 +1000 Subject: [PATCH 01/20] Drop obsolete sigops comment This comment was confusing and incorrect when first added ("invalid rather than merely non-standard" has the opposite meaning of what is actually the case), and was also not updated after segwit with the correct variable names. Delete it since the code reads just fine on its own. Co-authored by: Anthony Towns Suhas Daftuar --- src/validation.cpp | 5 ----- 1 file changed, 5 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index be6257ea2..ad2d327e3 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -721,11 +721,6 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool fSpendsCoinbase, nSigOpsCost, lp); unsigned int nSize = entry.GetTxSize(); - // Check that the transaction doesn't have an excessive number of - // sigops, making it impossible to mine. Since the coinbase transaction - // itself can contain sigops MAX_STANDARD_TX_SIGOPS is less than - // MAX_BLOCK_SIGOPS; we still consider this an invalid rather than - // merely non-standard transaction. if (nSigOpsCost > MAX_STANDARD_TX_SIGOPS_COST) return state.DoS(0, false, REJECT_NONSTANDARD, "bad-txns-too-many-sigops", false, strprintf("%d", nSigOpsCost)); From 00e11e61c0211a62788611cd6a6714a393fdc26c Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 22 Jan 2019 15:20:34 -0500 Subject: [PATCH 02/20] [refactor] rename stateDummy -> orphan_state Co-authored-by: Anthony Towns Suhas Daftuar --- src/net_processing.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) 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. From 8818729013e17c650a25f030b2b80e0997389155 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Mon, 16 Apr 2018 12:52:03 -0400 Subject: [PATCH 03/20] [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/consensus/validation.h | 1 + src/net_processing.cpp | 91 ++++++++++++++++++++++++-------------- 2 files changed, 58 insertions(+), 34 deletions(-) diff --git a/src/consensus/validation.h b/src/consensus/validation.h index f2e2c3585..36142e1e4 100644 --- a/src/consensus/validation.h +++ b/src/consensus/validation.h @@ -84,6 +84,7 @@ public: void SetCorruptionPossible() { corruptionPossible = true; } + int GetDoS(void) const { return nDoS; } unsigned int GetRejectCode() const { return chRejectCode; } std::string GetRejectReason() const { return strRejectReason; } std::string GetDebugMessage() const { return strDebugMessage; } 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; } } From b8b4c80146780f9011abbd1be72343cc965c07b9 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 17 Jan 2019 14:48:25 +1000 Subject: [PATCH 04/20] [refactor] drop IsInvalid(nDoSOut) Co-authored-by: Anthony Towns --- src/consensus/validation.h | 7 ------- src/test/txvalidation_tests.cpp | 4 ---- 2 files changed, 11 deletions(-) diff --git a/src/consensus/validation.h b/src/consensus/validation.h index 36142e1e4..163b17e62 100644 --- a/src/consensus/validation.h +++ b/src/consensus/validation.h @@ -71,13 +71,6 @@ public: bool IsError() const { return mode == MODE_ERROR; } - bool IsInvalid(int &nDoSOut) const { - if (IsInvalid()) { - nDoSOut = nDoS; - return true; - } - return false; - } bool CorruptionPossible() const { return corruptionPossible; } diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp index c2777cd6d..4876c44f1 100644 --- a/src/test/txvalidation_tests.cpp +++ b/src/test/txvalidation_tests.cpp @@ -52,10 +52,6 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_reject_coinbase, TestChain100Setup) // Check that the validation state reflects the unsuccessful attempt. BOOST_CHECK(state.IsInvalid()); BOOST_CHECK_EQUAL(state.GetRejectReason(), "coinbase"); - - int nDoS; - BOOST_CHECK_EQUAL(state.IsInvalid(nDoS), true); - BOOST_CHECK_EQUAL(nDoS, 100); } BOOST_AUTO_TEST_SUITE_END() From 7b999103e21509e1c2dec10f68e48744ffe90f55 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 23 Jan 2019 15:14:16 -0500 Subject: [PATCH 05/20] Clean up banning levels Compared with previous bans, the following changes are made: * Txn with empty vin/vout or null prevouts move from 10 DoS points to 100. * Loose transactions with a dependency loop now result in a ban instead of 10 DoS points. * Many pre-segwit soft-fork errors now result in a ban. Note: Transactions that violate soft-fork script flags since P2SH do not generally result in a ban. Also, banning behavior for invalid blocks is dependent on whether the node is validating with multiple script check threads, due to a long- standing bug. That inconsistency is still present after this commit. * Proof of work failure moves from 50 DoS points to a ban. * Blocks with timestamps under MTP now result in a ban, blocks too far in the future continue to *not* result in a ban. * Inclusion of non-final transactions in a block now results in a ban instead of 10 DoS points. Co-authored-by: Anthony Towns --- src/consensus/tx_check.cpp | 6 +++--- src/consensus/tx_verify.cpp | 4 ++-- src/validation.cpp | 10 +++++----- test/functional/data/invalid_txs.py | 4 ++-- test/functional/feature_block.py | 10 +++++----- 5 files changed, 17 insertions(+), 17 deletions(-) diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp index 61a607ef7..638f6b808 100644 --- a/src/consensus/tx_check.cpp +++ b/src/consensus/tx_check.cpp @@ -11,9 +11,9 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fChe { // Basic checks that don't depend on any context if (tx.vin.empty()) - return state.DoS(10, false, REJECT_INVALID, "bad-txns-vin-empty"); + return state.DoS(100, false, REJECT_INVALID, "bad-txns-vin-empty"); if (tx.vout.empty()) - return state.DoS(10, false, REJECT_INVALID, "bad-txns-vout-empty"); + return state.DoS(100, false, REJECT_INVALID, "bad-txns-vout-empty"); // Size limits (this doesn't take the witness into account, as that hasn't been checked for malleability) if (::GetSerializeSize(tx, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT) return state.DoS(100, false, REJECT_INVALID, "bad-txns-oversize"); @@ -50,7 +50,7 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fChe { for (const auto& txin : tx.vin) if (txin.prevout.IsNull()) - return state.DoS(10, false, REJECT_INVALID, "bad-txns-prevout-null"); + return state.DoS(100, false, REJECT_INVALID, "bad-txns-prevout-null"); } return true; diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp index fbbbcfd04..24b533850 100644 --- a/src/consensus/tx_verify.cpp +++ b/src/consensus/tx_verify.cpp @@ -172,8 +172,8 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, CValidationState& state, c // If prev is coinbase, check that it's matured if (coin.IsCoinBase() && nSpendHeight - coin.nHeight < COINBASE_MATURITY) { - return state.Invalid(false, - REJECT_INVALID, "bad-txns-premature-spend-of-coinbase", + return state.DoS(0, false, + REJECT_INVALID, "bad-txns-premature-spend-of-coinbase", false, strprintf("tried to spend coinbase at depth %d", nSpendHeight - coin.nHeight)); } diff --git a/src/validation.cpp b/src/validation.cpp index ad2d327e3..37082d3b0 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -760,7 +760,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool const uint256 &hashAncestor = ancestorIt->GetTx().GetHash(); if (setConflicts.count(hashAncestor)) { - return state.DoS(10, false, + return state.DoS(100, false, REJECT_INVALID, "bad-txns-spends-conflicting-tx", false, strprintf("%s spends conflicting transaction %s", hash.ToString(), @@ -3047,7 +3047,7 @@ static bool CheckBlockHeader(const CBlockHeader& block, CValidationState& state, { // Check proof of work matches claimed amount if (fCheckPOW && !CheckProofOfWork(block.GetHash(), block.nBits, consensusParams)) - return state.DoS(50, false, REJECT_INVALID, "high-hash", false, "proof of work failed"); + return state.DoS(100, false, REJECT_INVALID, "high-hash", false, "proof of work failed"); return true; } @@ -3214,7 +3214,7 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationSta // Check timestamp against prev if (block.GetBlockTime() <= pindexPrev->GetMedianTimePast()) - return state.Invalid(false, REJECT_INVALID, "time-too-old", "block's timestamp is too early"); + return state.DoS(100, false, REJECT_INVALID, "time-too-old", false, "block's timestamp is too early"); // Check timestamp if (block.GetBlockTime() > nAdjustedTime + MAX_FUTURE_BLOCK_TIME) @@ -3225,7 +3225,7 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationSta if((block.nVersion < 2 && nHeight >= consensusParams.BIP34Height) || (block.nVersion < 3 && nHeight >= consensusParams.BIP66Height) || (block.nVersion < 4 && nHeight >= consensusParams.BIP65Height)) - return state.Invalid(false, REJECT_OBSOLETE, strprintf("bad-version(0x%08x)", block.nVersion), + return state.DoS(100, false, REJECT_OBSOLETE, strprintf("bad-version(0x%08x)", block.nVersion), false, strprintf("rejected nVersion=0x%08x block", block.nVersion)); return true; @@ -3255,7 +3255,7 @@ static bool ContextualCheckBlock(const CBlock& block, CValidationState& state, c // Check that all transactions are finalized for (const auto& tx : block.vtx) { if (!IsFinalTx(*tx, nHeight, nLockTimeCutoff)) { - return state.DoS(10, false, REJECT_INVALID, "bad-txns-nonfinal", false, "non-final transaction"); + return state.DoS(100, false, REJECT_INVALID, "bad-txns-nonfinal", false, "non-final transaction"); } } diff --git a/test/functional/data/invalid_txs.py b/test/functional/data/invalid_txs.py index 02deae92f..f0991a284 100644 --- a/test/functional/data/invalid_txs.py +++ b/test/functional/data/invalid_txs.py @@ -58,7 +58,7 @@ class BadTxTemplate: class OutputMissing(BadTxTemplate): reject_reason = "bad-txns-vout-empty" - expect_disconnect = False + expect_disconnect = True def get_tx(self): tx = CTransaction() @@ -69,7 +69,7 @@ class OutputMissing(BadTxTemplate): class InputMissing(BadTxTemplate): reject_reason = "bad-txns-vin-empty" - expect_disconnect = False + expect_disconnect = True def get_tx(self): tx = CTransaction() diff --git a/test/functional/feature_block.py b/test/functional/feature_block.py index e7a888c32..9e00e2d23 100755 --- a/test/functional/feature_block.py +++ b/test/functional/feature_block.py @@ -630,7 +630,7 @@ class FullBlockTest(BitcoinTestFramework): while b47.sha256 < target: b47.nNonce += 1 b47.rehash() - self.send_blocks([b47], False, force_send=True, reject_reason='high-hash') + self.send_blocks([b47], False, force_send=True, reject_reason='high-hash', reconnect=True) self.log.info("Reject a block with a timestamp >2 hours in the future") self.move_tip(44) @@ -681,7 +681,7 @@ class FullBlockTest(BitcoinTestFramework): b54 = self.next_block(54, spend=out[15]) b54.nTime = b35.nTime - 1 b54.solve() - self.send_blocks([b54], False, force_send=True, reject_reason='time-too-old') + self.send_blocks([b54], False, force_send=True, reject_reason='time-too-old', reconnect=True) # valid timestamp self.move_tip(53) @@ -827,7 +827,7 @@ class FullBlockTest(BitcoinTestFramework): assert tx.vin[0].nSequence < 0xffffffff tx.calc_sha256() b62 = self.update_block(62, [tx]) - self.send_blocks([b62], success=False, reject_reason='bad-txns-nonfinal') + self.send_blocks([b62], success=False, reject_reason='bad-txns-nonfinal', reconnect=True) # Test a non-final coinbase is also rejected # @@ -841,7 +841,7 @@ class FullBlockTest(BitcoinTestFramework): b63.vtx[0].vin[0].nSequence = 0xDEADBEEF b63.vtx[0].rehash() b63 = self.update_block(63, []) - self.send_blocks([b63], success=False, reject_reason='bad-txns-nonfinal') + self.send_blocks([b63], success=False, reject_reason='bad-txns-nonfinal', reconnect=True) # This checks that a block with a bloated VARINT between the block_header and the array of tx such that # the block is > MAX_BLOCK_BASE_SIZE with the bloated varint, but <= MAX_BLOCK_BASE_SIZE without the bloated varint, @@ -1255,7 +1255,7 @@ class FullBlockTest(BitcoinTestFramework): self.log.info("Reject a block with an invalid block header version") b_v1 = self.next_block('b_v1', version=1) - self.send_blocks([b_v1], success=False, force_send=True, reject_reason='bad-version(0x00000001)') + self.send_blocks([b_v1], success=False, force_send=True, reject_reason='bad-version(0x00000001)', reconnect=True) self.move_tip(chain1_tip + 2) b_cb34 = self.next_block('b_cb34', version=4) From 6a7f8777a0b193fae4f976196f3464ffac01bf1b Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Tue, 16 Apr 2019 12:38:14 -0400 Subject: [PATCH 06/20] Ban all peers for all block script failures This eliminates a discrepancy between block validation with multiple script check threads, versus a single script check thread. --- src/validation.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/validation.cpp b/src/validation.cpp index 37082d3b0..0951b3cf9 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1998,9 +1998,13 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl { std::vector vChecks; bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */ - if (!CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : nullptr)) + if (!CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : nullptr)) { + // With parallel script checks, we always set DoS to 100; do + // that here as well for simplicity (for now). + state.DoS(100, false, state.GetRejectCode(), state.GetRejectReason(), state.CorruptionPossible(), state.GetDebugMessage()); return error("ConnectBlock(): CheckInputs on %s failed with %s", tx.GetHash().ToString(), FormatStateMessage(state)); + } control.Add(vChecks); } From 34477ccd39a8d4bfa8ad612f22d5a46291922185 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 16 Jan 2019 13:11:13 +1000 Subject: [PATCH 07/20] [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/consensus/tx_check.cpp | 18 ++--- src/consensus/tx_verify.cpp | 10 +-- src/consensus/validation.h | 85 ++++++++++++++++++-- src/net_processing.cpp | 2 + src/test/txvalidation_tests.cpp | 1 + src/validation.cpp | 131 +++++++++++++++++-------------- test/functional/feature_block.py | 4 +- 7 files changed, 173 insertions(+), 78 deletions(-) diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp index 638f6b808..3aa6d3ae1 100644 --- a/src/consensus/tx_check.cpp +++ b/src/consensus/tx_check.cpp @@ -11,24 +11,24 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fChe { // Basic checks that don't depend on any context if (tx.vin.empty()) - return state.DoS(100, false, REJECT_INVALID, "bad-txns-vin-empty"); + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-vin-empty"); if (tx.vout.empty()) - return state.DoS(100, false, REJECT_INVALID, "bad-txns-vout-empty"); + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-vout-empty"); // Size limits (this doesn't take the witness into account, as that hasn't been checked for malleability) if (::GetSerializeSize(tx, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT) - return state.DoS(100, false, REJECT_INVALID, "bad-txns-oversize"); + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-oversize"); // Check for negative or overflow output values CAmount nValueOut = 0; for (const auto& txout : tx.vout) { if (txout.nValue < 0) - return state.DoS(100, false, REJECT_INVALID, "bad-txns-vout-negative"); + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-vout-negative"); if (txout.nValue > MAX_MONEY) - return state.DoS(100, false, REJECT_INVALID, "bad-txns-vout-toolarge"); + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-vout-toolarge"); nValueOut += txout.nValue; if (!MoneyRange(nValueOut)) - return state.DoS(100, false, REJECT_INVALID, "bad-txns-txouttotal-toolarge"); + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-txouttotal-toolarge"); } // Check for duplicate inputs - note that this check is slow so we skip it in CheckBlock @@ -37,20 +37,20 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fChe for (const auto& txin : tx.vin) { if (!vInOutPoints.insert(txin.prevout).second) - return state.DoS(100, false, REJECT_INVALID, "bad-txns-inputs-duplicate"); + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-inputs-duplicate"); } } if (tx.IsCoinBase()) { if (tx.vin[0].scriptSig.size() < 2 || tx.vin[0].scriptSig.size() > 100) - return state.DoS(100, false, REJECT_INVALID, "bad-cb-length"); + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-cb-length"); } else { for (const auto& txin : tx.vin) if (txin.prevout.IsNull()) - return state.DoS(100, false, REJECT_INVALID, "bad-txns-prevout-null"); + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-prevout-null"); } return true; diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp index 24b533850..62a1676e2 100644 --- a/src/consensus/tx_verify.cpp +++ b/src/consensus/tx_verify.cpp @@ -160,7 +160,7 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, CValidationState& state, c { // are the actual inputs available? if (!inputs.HaveInputs(tx)) { - return state.DoS(100, false, REJECT_INVALID, "bad-txns-inputs-missingorspent", false, + return state.DoS(0, ValidationInvalidReason::TX_MISSING_INPUTS, false, REJECT_INVALID, "bad-txns-inputs-missingorspent", false, strprintf("%s: inputs missing/spent", __func__)); } @@ -172,7 +172,7 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, CValidationState& state, c // If prev is coinbase, check that it's matured if (coin.IsCoinBase() && nSpendHeight - coin.nHeight < COINBASE_MATURITY) { - return state.DoS(0, false, + return state.DoS(0, ValidationInvalidReason::TX_MISSING_INPUTS, false, REJECT_INVALID, "bad-txns-premature-spend-of-coinbase", false, strprintf("tried to spend coinbase at depth %d", nSpendHeight - coin.nHeight)); } @@ -180,20 +180,20 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, CValidationState& state, c // Check for negative or overflow input values nValueIn += coin.out.nValue; if (!MoneyRange(coin.out.nValue) || !MoneyRange(nValueIn)) { - return state.DoS(100, false, REJECT_INVALID, "bad-txns-inputvalues-outofrange"); + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-inputvalues-outofrange"); } } const CAmount value_out = tx.GetValueOut(); if (nValueIn < value_out) { - return state.DoS(100, false, REJECT_INVALID, "bad-txns-in-belowout", false, + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-in-belowout", false, strprintf("value in (%s) < value out (%s)", FormatMoney(nValueIn), FormatMoney(value_out))); } // Tally transaction fees const CAmount txfee_aux = nValueIn - value_out; if (!MoneyRange(txfee_aux)) { - return state.DoS(100, false, REJECT_INVALID, "bad-txns-fee-outofrange"); + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-fee-outofrange"); } txfee = txfee_aux; diff --git a/src/consensus/validation.h b/src/consensus/validation.h index 163b17e62..787b171ee 100644 --- a/src/consensus/validation.h +++ b/src/consensus/validation.h @@ -22,6 +22,50 @@ static const unsigned char REJECT_NONSTANDARD = 0x40; static const unsigned char REJECT_INSUFFICIENTFEE = 0x42; static const unsigned char REJECT_CHECKPOINT = 0x43; +/** A "reason" why something was invalid, suitable for determining whether the + * provider of the object should be banned/ignored/disconnected/etc. + * These are much more granular than the rejection codes, which may be more + * useful for some other use-cases. + */ +enum class ValidationInvalidReason { + // txn and blocks: + NONE, //!< not actually invalid + CONSENSUS, //!< invalid by consensus rules (excluding any below reasons) + /** + * Invalid by a change to consensus rules more recent than SegWit. + * Currently unused as there are no such consensus rule changes, and any download + * sources realistically need to support SegWit in order to provide useful data, + * so differentiating between always-invalid and invalid-by-pre-SegWit-soft-fork + * is uninteresting. + */ + RECENT_CONSENSUS_CHANGE, + // Only blocks (or headers): + CACHED_INVALID, //!< this object was cached as being invalid, but we don't know why + BLOCK_INVALID_HEADER, //!< invalid proof of work or time too old + BLOCK_MUTATED, //!< the block's data didn't match the data committed to by the PoW + BLOCK_MISSING_PREV, //!< We don't have the previous block the checked one is built on + BLOCK_INVALID_PREV, //!< A block this one builds on is invalid + BLOCK_TIME_FUTURE, //!< block timestamp was > 2 hours in the future (or our clock is bad) + BLOCK_CHECKPOINT, //!< the block failed to meet one of our checkpoints + // Only loose txn: + TX_NOT_STANDARD, //!< didn't meet our local policy rules + TX_MISSING_INPUTS, //!< a transaction was missing some of its inputs (or its inputs were spent at < coinbase maturity height) + /** + * Transaction might be missing a witness, have a witness prior to SegWit + * activation, or witness may have been malleated (which includes + * non-standard witnesses). + */ + TX_WITNESS_MUTATED, + /** + * Tx already in mempool or conflicts with a tx in the chain + * (if it conflicts with another tx in mempool, we use MEMPOOL_POLICY as it failed to reach the RBF threshold) + * TODO: Currently this is only used if the transaction already exists in the mempool or on chain, + * TODO: ATMP's fMissingInputs and a valid CValidationState being used to indicate missing inputs + */ + TX_CONFLICT, + TX_MEMPOOL_POLICY, //!< violated mempool's fee/size/descendant/RBF/etc limits +}; + /** Capture information about block/transaction validation */ class CValidationState { private: @@ -30,31 +74,35 @@ private: MODE_INVALID, //!< network rule violation (DoS value may be set) MODE_ERROR, //!< run-time error } mode; + ValidationInvalidReason m_reason; int nDoS; std::string strRejectReason; unsigned int chRejectCode; bool corruptionPossible; std::string strDebugMessage; public: - CValidationState() : mode(MODE_VALID), nDoS(0), chRejectCode(0), corruptionPossible(false) {} - bool DoS(int level, bool ret = false, + CValidationState() : mode(MODE_VALID), m_reason(ValidationInvalidReason::NONE), nDoS(0), chRejectCode(0), corruptionPossible(false) {} + bool DoS(int level, ValidationInvalidReason reasonIn, bool ret = false, unsigned int chRejectCodeIn=0, const std::string &strRejectReasonIn="", bool corruptionIn=false, const std::string &strDebugMessageIn="") { + m_reason = reasonIn; chRejectCode = chRejectCodeIn; strRejectReason = strRejectReasonIn; corruptionPossible = corruptionIn; strDebugMessage = strDebugMessageIn; + nDoS += level; + assert(nDoS == GetDoSForReason()); + assert(corruptionPossible == (m_reason == ValidationInvalidReason::BLOCK_MUTATED || m_reason == ValidationInvalidReason::TX_WITNESS_MUTATED)); if (mode == MODE_ERROR) return ret; - nDoS += level; mode = MODE_INVALID; return ret; } - bool Invalid(bool ret = false, + bool Invalid(ValidationInvalidReason _reason, bool ret = false, unsigned int _chRejectCode=0, const std::string &_strRejectReason="", const std::string &_strDebugMessage="") { - return DoS(0, ret, _chRejectCode, _strRejectReason, false, _strDebugMessage); + return DoS(0, _reason, ret, _chRejectCode, _strRejectReason, false, _strDebugMessage); } bool Error(const std::string& strRejectReasonIn) { if (mode == MODE_VALID) @@ -72,12 +120,39 @@ public: return mode == MODE_ERROR; } bool CorruptionPossible() const { + assert(corruptionPossible == (m_reason == ValidationInvalidReason::BLOCK_MUTATED || m_reason == ValidationInvalidReason::TX_WITNESS_MUTATED)); return corruptionPossible; } void SetCorruptionPossible() { corruptionPossible = true; + assert(corruptionPossible == (m_reason == ValidationInvalidReason::BLOCK_MUTATED || m_reason == ValidationInvalidReason::TX_WITNESS_MUTATED)); } int GetDoS(void) const { return nDoS; } + int GetDoSForReason() const { + switch (m_reason) { + case ValidationInvalidReason::NONE: + return 0; + case ValidationInvalidReason::CONSENSUS: + case ValidationInvalidReason::BLOCK_MUTATED: + case ValidationInvalidReason::BLOCK_INVALID_HEADER: + case ValidationInvalidReason::BLOCK_CHECKPOINT: + case ValidationInvalidReason::BLOCK_INVALID_PREV: + return 100; + case ValidationInvalidReason::BLOCK_MISSING_PREV: + return 10; + case ValidationInvalidReason::CACHED_INVALID: + 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: + return 0; + } + return 0; + } + ValidationInvalidReason GetReason() const { return m_reason; } unsigned int GetRejectCode() const { return chRejectCode; } std::string GetRejectReason() const { return strRejectReason; } std::string GetDebugMessage() const { return strDebugMessage; } 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); diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp index 4876c44f1..aa3012936 100644 --- a/src/test/txvalidation_tests.cpp +++ b/src/test/txvalidation_tests.cpp @@ -52,6 +52,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_reject_coinbase, TestChain100Setup) // Check that the validation state reflects the unsuccessful attempt. BOOST_CHECK(state.IsInvalid()); BOOST_CHECK_EQUAL(state.GetRejectReason(), "coinbase"); + BOOST_CHECK(state.GetReason() == ValidationInvalidReason::CONSENSUS); } BOOST_AUTO_TEST_SUITE_END() diff --git a/src/validation.cpp b/src/validation.cpp index 0951b3cf9..5f6d578c2 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -579,28 +579,28 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool // Coinbase is only valid in a block, not as a loose transaction if (tx.IsCoinBase()) - return state.DoS(100, false, REJECT_INVALID, "coinbase"); + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "coinbase"); // Rather not work on nonstandard transactions (unless -testnet/-regtest) std::string reason; if (fRequireStandard && !IsStandardTx(tx, reason)) - return state.DoS(0, false, REJECT_NONSTANDARD, reason); + return state.DoS(0, ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, reason); // Do not work on transactions that are too small. // A transaction with 1 segwit input and 1 P2WPHK output has non-witness size of 82 bytes. // Transactions smaller than this are not relayed to reduce unnecessary malloc overhead. if (::GetSerializeSize(tx, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) < MIN_STANDARD_TX_NONWITNESS_SIZE) - return state.DoS(0, false, REJECT_NONSTANDARD, "tx-size-small"); + return state.DoS(0, ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, "tx-size-small"); // Only accept nLockTime-using transactions that can be mined in the next // block; we don't want our mempool filled up with transactions that can't // be mined yet. if (!CheckFinalTx(tx, STANDARD_LOCKTIME_VERIFY_FLAGS)) - return state.DoS(0, false, REJECT_NONSTANDARD, "non-final"); + return state.DoS(0, ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, "non-final"); // is it already in the memory pool? if (pool.exists(hash)) { - return state.Invalid(false, REJECT_DUPLICATE, "txn-already-in-mempool"); + return state.Invalid(ValidationInvalidReason::TX_CONFLICT, false, REJECT_DUPLICATE, "txn-already-in-mempool"); } // Check for conflicts with in-memory transactions @@ -636,7 +636,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool } } if (fReplacementOptOut) { - return state.Invalid(false, REJECT_DUPLICATE, "txn-mempool-conflict"); + return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_DUPLICATE, "txn-mempool-conflict"); } setConflicts.insert(ptxConflicting->GetHash()); @@ -662,7 +662,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool for (size_t out = 0; out < tx.vout.size(); out++) { // Optimistically just do efficient check of cache for outputs if (pcoinsTip->HaveCoinInCache(COutPoint(hash, out))) { - return state.Invalid(false, REJECT_DUPLICATE, "txn-already-known"); + return state.Invalid(ValidationInvalidReason::TX_CONFLICT, false, REJECT_DUPLICATE, "txn-already-known"); } } // Otherwise assume this might be an orphan tx for which we just haven't seen parents yet @@ -685,7 +685,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool // Must keep pool.cs for this unless we change CheckSequenceLocks to take a // CoinsViewCache instead of create its own if (!CheckSequenceLocks(pool, tx, STANDARD_LOCKTIME_VERIFY_FLAGS, &lp)) - return state.DoS(0, false, REJECT_NONSTANDARD, "non-BIP68-final"); + return state.DoS(0, ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, "non-BIP68-final"); CAmount nFees = 0; if (!Consensus::CheckTxInputs(tx, state, view, GetSpendHeight(view), nFees)) { @@ -694,11 +694,11 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool // Check for non-standard pay-to-script-hash in inputs if (fRequireStandard && !AreInputsStandard(tx, view)) - return state.Invalid(false, REJECT_NONSTANDARD, "bad-txns-nonstandard-inputs"); + return state.Invalid(ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, "bad-txns-nonstandard-inputs"); // Check for non-standard witness in P2WSH if (tx.HasWitness() && fRequireStandard && !IsWitnessStandard(tx, view)) - return state.DoS(0, false, REJECT_NONSTANDARD, "bad-witness-nonstandard", true); + return state.DoS(0, ValidationInvalidReason::TX_WITNESS_MUTATED, false, REJECT_NONSTANDARD, "bad-witness-nonstandard", true); int64_t nSigOpsCost = GetTransactionSigOpCost(tx, view, STANDARD_SCRIPT_VERIFY_FLAGS); @@ -722,21 +722,21 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool unsigned int nSize = entry.GetTxSize(); if (nSigOpsCost > MAX_STANDARD_TX_SIGOPS_COST) - return state.DoS(0, false, REJECT_NONSTANDARD, "bad-txns-too-many-sigops", false, + return state.DoS(0, ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, "bad-txns-too-many-sigops", false, strprintf("%d", nSigOpsCost)); CAmount mempoolRejectFee = pool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFee(nSize); if (!bypass_limits && mempoolRejectFee > 0 && nModifiedFees < mempoolRejectFee) { - return state.DoS(0, false, REJECT_INSUFFICIENTFEE, "mempool min fee not met", false, strprintf("%d < %d", nModifiedFees, mempoolRejectFee)); + return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "mempool min fee not met", false, strprintf("%d < %d", nModifiedFees, mempoolRejectFee)); } // No transactions are allowed below minRelayTxFee except from disconnected blocks if (!bypass_limits && nModifiedFees < ::minRelayTxFee.GetFee(nSize)) { - return state.DoS(0, false, REJECT_INSUFFICIENTFEE, "min relay fee not met", false, strprintf("%d < %d", nModifiedFees, ::minRelayTxFee.GetFee(nSize))); + return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "min relay fee not met", false, strprintf("%d < %d", nModifiedFees, ::minRelayTxFee.GetFee(nSize))); } if (nAbsurdFee && nFees > nAbsurdFee) - return state.Invalid(false, + return state.Invalid(ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_HIGHFEE, "absurdly-high-fee", strprintf("%d > %d", nFees, nAbsurdFee)); @@ -748,7 +748,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool size_t nLimitDescendantSize = gArgs.GetArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT)*1000; std::string errString; if (!pool.CalculateMemPoolAncestors(entry, setAncestors, nLimitAncestors, nLimitAncestorSize, nLimitDescendants, nLimitDescendantSize, errString)) { - return state.DoS(0, false, REJECT_NONSTANDARD, "too-long-mempool-chain", false, errString); + return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too-long-mempool-chain", false, errString); } // A transaction that spends outputs that would be replaced by it is invalid. Now @@ -760,7 +760,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool const uint256 &hashAncestor = ancestorIt->GetTx().GetHash(); if (setConflicts.count(hashAncestor)) { - return state.DoS(100, false, + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-spends-conflicting-tx", false, strprintf("%s spends conflicting transaction %s", hash.ToString(), @@ -803,7 +803,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool CFeeRate oldFeeRate(mi->GetModifiedFee(), mi->GetTxSize()); if (newFeeRate <= oldFeeRate) { - return state.DoS(0, false, + return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "insufficient fee", false, strprintf("rejecting replacement %s; new feerate %s <= old feerate %s", hash.ToString(), @@ -832,7 +832,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool nConflictingSize += it->GetTxSize(); } } else { - return state.DoS(0, false, + return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too many potential replacements", false, strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n", hash.ToString(), @@ -852,7 +852,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool // it's cheaper to just check if the new input refers to a // tx that's in the mempool. if (pool.exists(tx.vin[j].prevout.hash)) { - return state.DoS(0, false, + return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "replacement-adds-unconfirmed", false, strprintf("replacement %s adds unconfirmed input, idx %d", hash.ToString(), j)); @@ -865,7 +865,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool // transactions would not be paid for. if (nModifiedFees < nConflictingFees) { - return state.DoS(0, false, + return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "insufficient fee", false, strprintf("rejecting replacement %s, less fees than conflicting txs; %s < %s", hash.ToString(), FormatMoney(nModifiedFees), FormatMoney(nConflictingFees))); @@ -876,7 +876,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool CAmount nDeltaFees = nModifiedFees - nConflictingFees; if (nDeltaFees < ::incrementalRelayFee.GetFee(nSize)) { - return state.DoS(0, false, + return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "insufficient fee", false, strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s", hash.ToString(), @@ -898,7 +898,8 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool if (!tx.HasWitness() && CheckInputs(tx, stateDummy, view, true, scriptVerifyFlags & ~(SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_CLEANSTACK), true, false, txdata) && !CheckInputs(tx, stateDummy, view, true, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true, false, txdata)) { // Only the witness is missing, so the transaction itself may be fine. - state.SetCorruptionPossible(); + state.DoS(0, ValidationInvalidReason::TX_WITNESS_MUTATED, false, + state.GetRejectCode(), state.GetRejectReason(), true, state.GetDebugMessage()); } return false; // state filled in by CheckInputs } @@ -956,7 +957,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool if (!bypass_limits) { LimitMempoolSize(pool, gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000, gArgs.GetArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY) * 60 * 60); if (!pool.exists(hash)) - return state.DoS(0, false, REJECT_INSUFFICIENTFEE, "mempool full"); + return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "mempool full"); } } @@ -1357,6 +1358,9 @@ void InitScriptExecutionCache() { * which are matched. This is useful for checking blocks where we will likely never need the cache * entry again. * + * Note that we may set state.reason to NOT_STANDARD for extra soft-fork flags in flags, block-checking + * callers should probably reset it to CONSENSUS in such cases. + * * Non-static (and re-declared) in src/test/txvalidationcache_tests.cpp */ bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsViewCache &inputs, bool fScriptChecks, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector *pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main) @@ -1418,7 +1422,7 @@ bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsVi CScriptCheck check2(coin.out, tx, i, flags & ~STANDARD_NOT_MANDATORY_VERIFY_FLAGS, cacheSigStore, &txdata); if (check2()) - return state.Invalid(false, REJECT_NONSTANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(check.GetScriptError()))); + return state.Invalid(ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(check.GetScriptError()))); } // Failures of other flags indicate a transaction that is // invalid in new blocks, e.g. an invalid P2SH. We DoS ban @@ -1427,7 +1431,7 @@ bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsVi // as to the correct behavior - we may want to continue // peering with non-upgraded nodes even after soft-fork // super-majority signaling has occurred. - return state.DoS(100,false, REJECT_INVALID, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(check.GetScriptError()))); + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(check.GetScriptError()))); } } @@ -1922,7 +1926,7 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl for (const auto& tx : block.vtx) { for (size_t o = 0; o < tx->vout.size(); o++) { if (view.HaveCoin(COutPoint(tx->GetHash(), o))) { - return state.DoS(100, error("ConnectBlock(): tried to overwrite transaction"), + return state.DoS(100, ValidationInvalidReason::CONSENSUS, error("ConnectBlock(): tried to overwrite transaction"), REJECT_INVALID, "bad-txns-BIP30"); } } @@ -1962,11 +1966,17 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl { CAmount txfee = 0; if (!Consensus::CheckTxInputs(tx, state, view, pindex->nHeight, txfee)) { + if (state.GetReason() == ValidationInvalidReason::TX_MISSING_INPUTS) { + // CheckTxInputs may return MISSING_INPUTS but we can't return that, as + // it's not defined for a block, so we reset the reason flag to CONSENSUS here. + state.DoS(100, ValidationInvalidReason::CONSENSUS, false, + state.GetRejectCode(), state.GetRejectReason(), state.CorruptionPossible(), state.GetDebugMessage()); + } return error("%s: Consensus::CheckTxInputs: %s, %s", __func__, tx.GetHash().ToString(), FormatStateMessage(state)); } nFees += txfee; if (!MoneyRange(nFees)) { - return state.DoS(100, error("%s: accumulated fee in the block out of range.", __func__), + return state.DoS(100, ValidationInvalidReason::CONSENSUS, error("%s: accumulated fee in the block out of range.", __func__), REJECT_INVALID, "bad-txns-accumulated-fee-outofrange"); } @@ -1979,7 +1989,7 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl } if (!SequenceLocks(tx, nLockTimeFlags, &prevheights, *pindex)) { - return state.DoS(100, error("%s: contains a non-BIP68-final transaction", __func__), + return state.DoS(100, ValidationInvalidReason::CONSENSUS, error("%s: contains a non-BIP68-final transaction", __func__), REJECT_INVALID, "bad-txns-nonfinal"); } } @@ -1990,7 +2000,7 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl // * witness (when witness enabled in flags and excludes coinbase) nSigOpsCost += GetTransactionSigOpCost(tx, view, flags); if (nSigOpsCost > MAX_BLOCK_SIGOPS_COST) - return state.DoS(100, error("ConnectBlock(): too many sigops"), + return state.DoS(100, ValidationInvalidReason::CONSENSUS, error("ConnectBlock(): too many sigops"), REJECT_INVALID, "bad-blk-sigops"); txdata.emplace_back(tx); @@ -1999,9 +2009,16 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl std::vector vChecks; bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */ if (!CheckInputs(tx, state, view, fScriptChecks, flags, fCacheResults, fCacheResults, txdata[i], nScriptCheckThreads ? &vChecks : nullptr)) { - // With parallel script checks, we always set DoS to 100; do - // that here as well for simplicity (for now). - state.DoS(100, false, state.GetRejectCode(), state.GetRejectReason(), state.CorruptionPossible(), state.GetDebugMessage()); + if (state.GetReason() == ValidationInvalidReason::TX_NOT_STANDARD) { + // CheckInputs may return NOT_STANDARD for extra flags we passed, + // but we can't return that, as it's not defined for a block, so + // we reset the reason flag to CONSENSUS here. + // In the event of a future soft-fork, we may need to + // consider whether rewriting to CONSENSUS or + // RECENT_CONSENSUS_CHANGE would be more appropriate. + state.DoS(100, ValidationInvalidReason::CONSENSUS, false, + state.GetRejectCode(), state.GetRejectReason(), state.CorruptionPossible(), state.GetDebugMessage()); + } return error("ConnectBlock(): CheckInputs on %s failed with %s", tx.GetHash().ToString(), FormatStateMessage(state)); } @@ -2019,13 +2036,13 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl CAmount blockReward = nFees + GetBlockSubsidy(pindex->nHeight, chainparams.GetConsensus()); if (block.vtx[0]->GetValueOut() > blockReward) - return state.DoS(100, + return state.DoS(100, ValidationInvalidReason::CONSENSUS, error("ConnectBlock(): coinbase pays too much (actual=%d vs limit=%d)", block.vtx[0]->GetValueOut(), blockReward), REJECT_INVALID, "bad-cb-amount"); if (!control.Wait()) - return state.DoS(100, error("%s: CheckQueue failed", __func__), REJECT_INVALID, "block-validation-failed"); + return state.DoS(100, ValidationInvalidReason::CONSENSUS, error("%s: CheckQueue failed", __func__), REJECT_INVALID, "block-validation-failed"); int64_t nTime4 = GetTimeMicros(); nTimeVerify += nTime4 - nTime2; LogPrint(BCLog::BENCH, " - Verify %u txins: %.2fms (%.3fms/txin) [%.2fs (%.2fms/blk)]\n", nInputs - 1, MILLI * (nTime4 - nTime2), nInputs <= 1 ? 0 : MILLI * (nTime4 - nTime2) / (nInputs-1), nTimeVerify * MICRO, nTimeVerify * MILLI / nBlocksTotal); @@ -3051,7 +3068,7 @@ static bool CheckBlockHeader(const CBlockHeader& block, CValidationState& state, { // Check proof of work matches claimed amount if (fCheckPOW && !CheckProofOfWork(block.GetHash(), block.nBits, consensusParams)) - return state.DoS(100, false, REJECT_INVALID, "high-hash", false, "proof of work failed"); + return state.DoS(100, ValidationInvalidReason::BLOCK_INVALID_HEADER, false, REJECT_INVALID, "high-hash", false, "proof of work failed"); return true; } @@ -3073,13 +3090,13 @@ bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::P bool mutated; uint256 hashMerkleRoot2 = BlockMerkleRoot(block, &mutated); if (block.hashMerkleRoot != hashMerkleRoot2) - return state.DoS(100, false, REJECT_INVALID, "bad-txnmrklroot", true, "hashMerkleRoot mismatch"); + return state.DoS(100, ValidationInvalidReason::BLOCK_MUTATED, false, REJECT_INVALID, "bad-txnmrklroot", true, "hashMerkleRoot mismatch"); // Check for merkle tree malleability (CVE-2012-2459): repeating sequences // of transactions in a block without affecting the merkle root of a block, // while still invalidating it. if (mutated) - return state.DoS(100, false, REJECT_INVALID, "bad-txns-duplicate", true, "duplicate transaction"); + return state.DoS(100, ValidationInvalidReason::BLOCK_MUTATED, false, REJECT_INVALID, "bad-txns-duplicate", true, "duplicate transaction"); } // All potential-corruption validation must be done before we do any @@ -3090,19 +3107,19 @@ bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::P // Size limits if (block.vtx.empty() || block.vtx.size() * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT || ::GetSerializeSize(block, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT) - return state.DoS(100, false, REJECT_INVALID, "bad-blk-length", false, "size limits failed"); + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-blk-length", false, "size limits failed"); // First transaction must be coinbase, the rest must not be if (block.vtx.empty() || !block.vtx[0]->IsCoinBase()) - return state.DoS(100, false, REJECT_INVALID, "bad-cb-missing", false, "first tx is not coinbase"); + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-cb-missing", false, "first tx is not coinbase"); for (unsigned int i = 1; i < block.vtx.size(); i++) if (block.vtx[i]->IsCoinBase()) - return state.DoS(100, false, REJECT_INVALID, "bad-cb-multiple", false, "more than one coinbase"); + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-cb-multiple", false, "more than one coinbase"); // Check transactions for (const auto& tx : block.vtx) if (!CheckTransaction(*tx, state, true)) - return state.Invalid(false, state.GetRejectCode(), state.GetRejectReason(), + return state.Invalid(state.GetReason(), false, state.GetRejectCode(), state.GetRejectReason(), strprintf("Transaction check failed (tx hash %s) %s", tx->GetHash().ToString(), state.GetDebugMessage())); unsigned int nSigOps = 0; @@ -3111,7 +3128,7 @@ bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::P nSigOps += GetLegacySigOpCount(*tx); } if (nSigOps * WITNESS_SCALE_FACTOR > MAX_BLOCK_SIGOPS_COST) - return state.DoS(100, false, REJECT_INVALID, "bad-blk-sigops", false, "out-of-bounds SigOpCount"); + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-blk-sigops", false, "out-of-bounds SigOpCount"); if (fCheckPOW && fCheckMerkleRoot) block.fChecked = true; @@ -3204,7 +3221,7 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationSta // Check proof of work const Consensus::Params& consensusParams = params.GetConsensus(); if (block.nBits != GetNextWorkRequired(pindexPrev, &block, consensusParams)) - return state.DoS(100, false, REJECT_INVALID, "bad-diffbits", false, "incorrect proof of work"); + return state.DoS(100, ValidationInvalidReason::BLOCK_INVALID_HEADER, false, REJECT_INVALID, "bad-diffbits", false, "incorrect proof of work"); // Check against checkpoints if (fCheckpointsEnabled) { @@ -3213,23 +3230,23 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationSta // MapBlockIndex. CBlockIndex* pcheckpoint = Checkpoints::GetLastCheckpoint(params.Checkpoints()); if (pcheckpoint && nHeight < pcheckpoint->nHeight) - return state.DoS(100, error("%s: forked chain older than last checkpoint (height %d)", __func__, nHeight), REJECT_CHECKPOINT, "bad-fork-prior-to-checkpoint"); + return state.DoS(100, ValidationInvalidReason::BLOCK_CHECKPOINT, error("%s: forked chain older than last checkpoint (height %d)", __func__, nHeight), REJECT_CHECKPOINT, "bad-fork-prior-to-checkpoint"); } // Check timestamp against prev if (block.GetBlockTime() <= pindexPrev->GetMedianTimePast()) - return state.DoS(100, false, REJECT_INVALID, "time-too-old", false, "block's timestamp is too early"); + return state.DoS(100, ValidationInvalidReason::BLOCK_INVALID_HEADER, false, REJECT_INVALID, "time-too-old", false, "block's timestamp is too early"); // Check timestamp if (block.GetBlockTime() > nAdjustedTime + MAX_FUTURE_BLOCK_TIME) - return state.Invalid(false, REJECT_INVALID, "time-too-new", "block timestamp too far in the future"); + return state.Invalid(ValidationInvalidReason::BLOCK_TIME_FUTURE, false, REJECT_INVALID, "time-too-new", "block timestamp too far in the future"); // Reject outdated version blocks when 95% (75% on testnet) of the network has upgraded: // check for version 2, 3 and 4 upgrades if((block.nVersion < 2 && nHeight >= consensusParams.BIP34Height) || (block.nVersion < 3 && nHeight >= consensusParams.BIP66Height) || (block.nVersion < 4 && nHeight >= consensusParams.BIP65Height)) - return state.DoS(100, false, REJECT_OBSOLETE, strprintf("bad-version(0x%08x)", block.nVersion), false, + return state.DoS(100, ValidationInvalidReason::BLOCK_INVALID_HEADER, false, REJECT_OBSOLETE, strprintf("bad-version(0x%08x)", block.nVersion), false, strprintf("rejected nVersion=0x%08x block", block.nVersion)); return true; @@ -3259,7 +3276,7 @@ static bool ContextualCheckBlock(const CBlock& block, CValidationState& state, c // Check that all transactions are finalized for (const auto& tx : block.vtx) { if (!IsFinalTx(*tx, nHeight, nLockTimeCutoff)) { - return state.DoS(100, false, REJECT_INVALID, "bad-txns-nonfinal", false, "non-final transaction"); + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-nonfinal", false, "non-final transaction"); } } @@ -3269,7 +3286,7 @@ static bool ContextualCheckBlock(const CBlock& block, CValidationState& state, c CScript expect = CScript() << nHeight; if (block.vtx[0]->vin[0].scriptSig.size() < expect.size() || !std::equal(expect.begin(), expect.end(), block.vtx[0]->vin[0].scriptSig.begin())) { - return state.DoS(100, false, REJECT_INVALID, "bad-cb-height", false, "block height mismatch in coinbase"); + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-cb-height", false, "block height mismatch in coinbase"); } } @@ -3291,11 +3308,11 @@ static bool ContextualCheckBlock(const CBlock& block, CValidationState& state, c // already does not permit it, it is impossible to trigger in the // witness tree. if (block.vtx[0]->vin[0].scriptWitness.stack.size() != 1 || block.vtx[0]->vin[0].scriptWitness.stack[0].size() != 32) { - return state.DoS(100, false, REJECT_INVALID, "bad-witness-nonce-size", true, strprintf("%s : invalid witness reserved value size", __func__)); + return state.DoS(100, ValidationInvalidReason::BLOCK_MUTATED, false, REJECT_INVALID, "bad-witness-nonce-size", true, strprintf("%s : invalid witness reserved value size", __func__)); } CHash256().Write(hashWitness.begin(), 32).Write(&block.vtx[0]->vin[0].scriptWitness.stack[0][0], 32).Finalize(hashWitness.begin()); if (memcmp(hashWitness.begin(), &block.vtx[0]->vout[commitpos].scriptPubKey[6], 32)) { - return state.DoS(100, false, REJECT_INVALID, "bad-witness-merkle-match", true, strprintf("%s : witness merkle commitment mismatch", __func__)); + return state.DoS(100, ValidationInvalidReason::BLOCK_MUTATED, false, REJECT_INVALID, "bad-witness-merkle-match", true, strprintf("%s : witness merkle commitment mismatch", __func__)); } fHaveWitness = true; } @@ -3305,7 +3322,7 @@ static bool ContextualCheckBlock(const CBlock& block, CValidationState& state, c if (!fHaveWitness) { for (const auto& tx : block.vtx) { if (tx->HasWitness()) { - return state.DoS(100, false, REJECT_INVALID, "unexpected-witness", true, strprintf("%s : unexpected witness data found", __func__)); + return state.DoS(100, ValidationInvalidReason::BLOCK_MUTATED, false, REJECT_INVALID, "unexpected-witness", true, strprintf("%s : unexpected witness data found", __func__)); } } } @@ -3317,7 +3334,7 @@ static bool ContextualCheckBlock(const CBlock& block, CValidationState& state, c // the block hash, so we couldn't mark the block as permanently // failed). if (GetBlockWeight(block) > MAX_BLOCK_WEIGHT) { - return state.DoS(100, false, REJECT_INVALID, "bad-blk-weight", false, strprintf("%s : weight limit failed", __func__)); + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-blk-weight", false, strprintf("%s : weight limit failed", __func__)); } return true; @@ -3337,7 +3354,7 @@ bool CChainState::AcceptBlockHeader(const CBlockHeader& block, CValidationState& if (ppindex) *ppindex = pindex; if (pindex->nStatus & BLOCK_FAILED_MASK) - return state.Invalid(error("%s: block %s is marked invalid", __func__, hash.ToString()), 0, "duplicate"); + return state.Invalid(ValidationInvalidReason::CACHED_INVALID, error("%s: block %s is marked invalid", __func__, hash.ToString()), 0, "duplicate"); return true; } @@ -3348,10 +3365,10 @@ bool CChainState::AcceptBlockHeader(const CBlockHeader& block, CValidationState& CBlockIndex* pindexPrev = nullptr; BlockMap::iterator mi = mapBlockIndex.find(block.hashPrevBlock); if (mi == mapBlockIndex.end()) - return state.DoS(10, error("%s: prev block not found", __func__), 0, "prev-blk-not-found"); + return state.DoS(10, ValidationInvalidReason::BLOCK_MISSING_PREV, error("%s: prev block not found", __func__), 0, "prev-blk-not-found"); pindexPrev = (*mi).second; if (pindexPrev->nStatus & BLOCK_FAILED_MASK) - return state.DoS(100, error("%s: prev block invalid", __func__), REJECT_INVALID, "bad-prevblk"); + return state.DoS(100, ValidationInvalidReason::BLOCK_INVALID_PREV, error("%s: prev block invalid", __func__), REJECT_INVALID, "bad-prevblk"); if (!ContextualCheckBlockHeader(block, state, chainparams, pindexPrev, GetAdjustedTime())) return error("%s: Consensus::ContextualCheckBlockHeader: %s, %s", __func__, hash.ToString(), FormatStateMessage(state)); @@ -3388,7 +3405,7 @@ bool CChainState::AcceptBlockHeader(const CBlockHeader& block, CValidationState& setDirtyBlockIndex.insert(invalid_walk); invalid_walk = invalid_walk->pprev; } - return state.DoS(100, error("%s: prev block invalid", __func__), REJECT_INVALID, "bad-prevblk"); + return state.DoS(100, ValidationInvalidReason::BLOCK_INVALID_PREV, error("%s: prev block invalid", __func__), REJECT_INVALID, "bad-prevblk"); } } } diff --git a/test/functional/feature_block.py b/test/functional/feature_block.py index 9e00e2d23..9a9424b82 100755 --- a/test/functional/feature_block.py +++ b/test/functional/feature_block.py @@ -295,7 +295,7 @@ class FullBlockTest(BitcoinTestFramework): self.log.info("Reject a block spending an immature coinbase.") self.move_tip(15) b20 = self.next_block(20, spend=out[7]) - self.send_blocks([b20], success=False, reject_reason='bad-txns-premature-spend-of-coinbase') + self.send_blocks([b20], success=False, reject_reason='bad-txns-premature-spend-of-coinbase', reconnect=True) # Attempt to spend a coinbase at depth too low (on a fork this time) # genesis -> b1 (0) -> b2 (1) -> b5 (2) -> b6 (3) @@ -308,7 +308,7 @@ class FullBlockTest(BitcoinTestFramework): self.send_blocks([b21], False) b22 = self.next_block(22, spend=out[5]) - self.send_blocks([b22], success=False, reject_reason='bad-txns-premature-spend-of-coinbase') + self.send_blocks([b22], success=False, reject_reason='bad-txns-premature-spend-of-coinbase', reconnect=True) # Create a block on either side of MAX_BLOCK_BASE_SIZE and make sure its accepted/rejected # genesis -> b1 (0) -> b2 (1) -> b5 (2) -> b6 (3) From c8b0d22698385f91215ce8145631e3d5826dc977 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 18 Jan 2019 12:58:41 +1000 Subject: [PATCH 08/20] [refactor] Drop redundant nDoS, corruptionPossible, SetCorruptionPossible Co-authored-by: Anthony Towns --- src/consensus/validation.h | 21 +++++---------------- src/net_processing.cpp | 2 -- 2 files changed, 5 insertions(+), 18 deletions(-) diff --git a/src/consensus/validation.h b/src/consensus/validation.h index 787b171ee..a18739135 100644 --- a/src/consensus/validation.h +++ b/src/consensus/validation.h @@ -75,25 +75,20 @@ private: MODE_ERROR, //!< run-time error } mode; ValidationInvalidReason m_reason; - int nDoS; std::string strRejectReason; unsigned int chRejectCode; - bool corruptionPossible; std::string strDebugMessage; public: - CValidationState() : mode(MODE_VALID), m_reason(ValidationInvalidReason::NONE), nDoS(0), chRejectCode(0), corruptionPossible(false) {} + CValidationState() : mode(MODE_VALID), m_reason(ValidationInvalidReason::NONE), chRejectCode(0) {} bool DoS(int level, ValidationInvalidReason reasonIn, bool ret = false, unsigned int chRejectCodeIn=0, const std::string &strRejectReasonIn="", - bool corruptionIn=false, + bool corruptionPossibleIn=false, const std::string &strDebugMessageIn="") { m_reason = reasonIn; chRejectCode = chRejectCodeIn; strRejectReason = strRejectReasonIn; - corruptionPossible = corruptionIn; strDebugMessage = strDebugMessageIn; - nDoS += level; - assert(nDoS == GetDoSForReason()); - assert(corruptionPossible == (m_reason == ValidationInvalidReason::BLOCK_MUTATED || m_reason == ValidationInvalidReason::TX_WITNESS_MUTATED)); + assert(corruptionPossibleIn == CorruptionPossible()); if (mode == MODE_ERROR) return ret; mode = MODE_INVALID; @@ -120,15 +115,9 @@ public: return mode == MODE_ERROR; } bool CorruptionPossible() const { - assert(corruptionPossible == (m_reason == ValidationInvalidReason::BLOCK_MUTATED || m_reason == ValidationInvalidReason::TX_WITNESS_MUTATED)); - return corruptionPossible; + return m_reason == ValidationInvalidReason::BLOCK_MUTATED || m_reason == ValidationInvalidReason::TX_WITNESS_MUTATED; } - void SetCorruptionPossible() { - corruptionPossible = true; - assert(corruptionPossible == (m_reason == ValidationInvalidReason::BLOCK_MUTATED || m_reason == ValidationInvalidReason::TX_WITNESS_MUTATED)); - } - int GetDoS(void) const { return nDoS; } - int GetDoSForReason() const { + int GetDoS() const { switch (m_reason) { case ValidationInvalidReason::NONE: return 0; 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); From 7df16e70e67c753c871797ce947ea09d7cb0e519 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 17 Jan 2019 14:33:16 +1000 Subject: [PATCH 09/20] LookupBlockIndex -> CACHED_INVALID Co-authored-by: Anthony Towns --- src/net_processing.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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. // From 6e55b292b0ea944897b6dc2f766446fd209af484 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 17 Jan 2019 14:35:01 +1000 Subject: [PATCH 10/20] CorruptionPossible -> TX_WITNESS_MUTATED Co-authored-by: Anthony Towns --- src/net_processing.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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. From 9ab2a0412e96e87956fe61257387683635213035 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 18 Jan 2019 12:56:06 +1000 Subject: [PATCH 11/20] CorruptionPossible -> BLOCK_MUTATED Co-authored-by: Anthony Towns --- src/blockencodings.cpp | 2 +- src/validation.cpp | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp index 10f51931f..f0fcf675e 100644 --- a/src/blockencodings.cpp +++ b/src/blockencodings.cpp @@ -203,7 +203,7 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector< // but that is expensive, and CheckBlock caches a block's // "checked-status" (in the CBlock?). CBlock should be able to // check its own merkle root and cache that check. - if (state.CorruptionPossible()) + if (state.GetReason() == ValidationInvalidReason::BLOCK_MUTATED) return READ_STATUS_FAILED; // Possible Short ID collision return READ_STATUS_CHECKBLOCK_FAILED; } diff --git a/src/validation.cpp b/src/validation.cpp index 5f6d578c2..6aa2941e5 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1290,7 +1290,7 @@ void static InvalidChainFound(CBlockIndex* pindexNew) EXCLUSIVE_LOCKS_REQUIRED(c } void CChainState::InvalidBlockFound(CBlockIndex *pindex, const CValidationState &state) { - if (!state.CorruptionPossible()) { + if (state.GetReason() != ValidationInvalidReason::BLOCK_MUTATED) { pindex->nStatus |= BLOCK_FAILED_VALID; m_failed_blocks.insert(pindex); setDirtyBlockIndex.insert(pindex); @@ -1791,7 +1791,7 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl // re-enforce that rule here (at least until we make it impossible for // GetAdjustedTime() to go backward). if (!CheckBlock(block, state, chainparams.GetConsensus(), !fJustCheck, !fJustCheck)) { - if (state.CorruptionPossible()) { + if (state.GetReason() == ValidationInvalidReason::BLOCK_MUTATED) { // We don't write down blocks to disk if they may have been // corrupted, so this should be impossible unless we're having hardware // problems. @@ -2570,7 +2570,7 @@ bool CChainState::ActivateBestChainStep(CValidationState& state, const CChainPar if (!ConnectTip(state, chainparams, pindexConnect, pindexConnect == pindexMostWork ? pblock : std::shared_ptr(), connectTrace, disconnectpool)) { if (state.IsInvalid()) { // The block violates a consensus rule. - if (!state.CorruptionPossible()) { + if (state.GetReason() != ValidationInvalidReason::BLOCK_MUTATED) { InvalidChainFound(vpindexToConnect.front()); } state = CValidationState(); @@ -3509,7 +3509,7 @@ bool CChainState::AcceptBlock(const std::shared_ptr& pblock, CVali if (!CheckBlock(block, state, chainparams.GetConsensus()) || !ContextualCheckBlock(block, state, chainparams.GetConsensus(), pindex->pprev)) { - if (state.IsInvalid() && !state.CorruptionPossible()) { + if (state.IsInvalid() && state.GetReason() != ValidationInvalidReason::BLOCK_MUTATED) { pindex->nStatus |= BLOCK_FAILED_VALID; setDirtyBlockIndex.insert(pindex); } From ef54b486d5333dfc85c56e6b933c81735196a25d Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 8 Nov 2017 11:57:38 -0500 Subject: [PATCH 12/20] [refactor] Use Reasons directly instead of DoS codes --- src/net_processing.cpp | 87 +++++++++++++++++++++++++++--------------- 1 file changed, 57 insertions(+), 30 deletions(-) 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; } } From 6b34bc6b6f54f85537494cbea3846d5d195a06d9 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Tue, 15 Jan 2019 15:54:04 -0500 Subject: [PATCH 13/20] 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 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) From 5e78c5734bb0c9aae7b0a7019a745b2d7059b3d9 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sun, 20 Jan 2019 13:21:55 +1000 Subject: [PATCH 14/20] Allow use of state.Invalid() for all reasons Co-authored-by: Anthony Towns --- src/consensus/validation.h | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/consensus/validation.h b/src/consensus/validation.h index a18739135..9583e6105 100644 --- a/src/consensus/validation.h +++ b/src/consensus/validation.h @@ -84,21 +84,23 @@ public: unsigned int chRejectCodeIn=0, const std::string &strRejectReasonIn="", bool corruptionPossibleIn=false, const std::string &strDebugMessageIn="") { + ret = Invalid(reasonIn, ret, chRejectCodeIn, strRejectReasonIn, strDebugMessageIn); + assert(level == GetDoS()); + assert(corruptionPossibleIn == CorruptionPossible()); + return ret; + } + bool Invalid(ValidationInvalidReason reasonIn, bool ret = false, + unsigned int chRejectCodeIn=0, const std::string &strRejectReasonIn="", + const std::string &strDebugMessageIn="") { m_reason = reasonIn; chRejectCode = chRejectCodeIn; strRejectReason = strRejectReasonIn; strDebugMessage = strDebugMessageIn; - assert(corruptionPossibleIn == CorruptionPossible()); if (mode == MODE_ERROR) return ret; mode = MODE_INVALID; return ret; } - bool Invalid(ValidationInvalidReason _reason, bool ret = false, - unsigned int _chRejectCode=0, const std::string &_strRejectReason="", - const std::string &_strDebugMessage="") { - return DoS(0, _reason, ret, _chRejectCode, _strRejectReason, false, _strDebugMessage); - } bool Error(const std::string& strRejectReasonIn) { if (mode == MODE_VALID) strRejectReason = strRejectReasonIn; From 7721ad64f40a0c67edefaaf7353264d78df8803e Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 8 Nov 2017 14:50:18 -0500 Subject: [PATCH 15/20] [refactor] Prep for scripted-diff by removing some \ns which annoy sed. --- src/consensus/tx_verify.cpp | 3 +-- src/validation.cpp | 22 ++++++++-------------- 2 files changed, 9 insertions(+), 16 deletions(-) diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp index 62a1676e2..60af773b1 100644 --- a/src/consensus/tx_verify.cpp +++ b/src/consensus/tx_verify.cpp @@ -172,8 +172,7 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, CValidationState& state, c // If prev is coinbase, check that it's matured if (coin.IsCoinBase() && nSpendHeight - coin.nHeight < COINBASE_MATURITY) { - return state.DoS(0, ValidationInvalidReason::TX_MISSING_INPUTS, false, - REJECT_INVALID, "bad-txns-premature-spend-of-coinbase", false, + return state.DoS(0, ValidationInvalidReason::TX_MISSING_INPUTS, false, REJECT_INVALID, "bad-txns-premature-spend-of-coinbase", false, strprintf("tried to spend coinbase at depth %d", nSpendHeight - coin.nHeight)); } diff --git a/src/validation.cpp b/src/validation.cpp index 6aa2941e5..d7595627e 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -760,8 +760,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool const uint256 &hashAncestor = ancestorIt->GetTx().GetHash(); if (setConflicts.count(hashAncestor)) { - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, - REJECT_INVALID, "bad-txns-spends-conflicting-tx", false, + return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-spends-conflicting-tx", false, strprintf("%s spends conflicting transaction %s", hash.ToString(), hashAncestor.ToString())); @@ -803,8 +802,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool CFeeRate oldFeeRate(mi->GetModifiedFee(), mi->GetTxSize()); if (newFeeRate <= oldFeeRate) { - return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, - REJECT_INSUFFICIENTFEE, "insufficient fee", false, + return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "insufficient fee", false, strprintf("rejecting replacement %s; new feerate %s <= old feerate %s", hash.ToString(), newFeeRate.ToString(), @@ -832,8 +830,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool nConflictingSize += it->GetTxSize(); } } else { - return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, - REJECT_NONSTANDARD, "too many potential replacements", false, + return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too many potential replacements", false, strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n", hash.ToString(), nConflictingCount, @@ -852,8 +849,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool // it's cheaper to just check if the new input refers to a // tx that's in the mempool. if (pool.exists(tx.vin[j].prevout.hash)) { - return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, - REJECT_NONSTANDARD, "replacement-adds-unconfirmed", false, + return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "replacement-adds-unconfirmed", false, strprintf("replacement %s adds unconfirmed input, idx %d", hash.ToString(), j)); } @@ -865,8 +861,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool // transactions would not be paid for. if (nModifiedFees < nConflictingFees) { - return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, - REJECT_INSUFFICIENTFEE, "insufficient fee", false, + return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "insufficient fee", false, strprintf("rejecting replacement %s, less fees than conflicting txs; %s < %s", hash.ToString(), FormatMoney(nModifiedFees), FormatMoney(nConflictingFees))); } @@ -876,8 +871,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool CAmount nDeltaFees = nModifiedFees - nConflictingFees; if (nDeltaFees < ::incrementalRelayFee.GetFee(nSize)) { - return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, - REJECT_INSUFFICIENTFEE, "insufficient fee", false, + return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "insufficient fee", false, strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s", hash.ToString(), FormatMoney(nDeltaFees), @@ -899,7 +893,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool !CheckInputs(tx, stateDummy, view, true, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true, false, txdata)) { // Only the witness is missing, so the transaction itself may be fine. state.DoS(0, ValidationInvalidReason::TX_WITNESS_MUTATED, false, - state.GetRejectCode(), state.GetRejectReason(), true, state.GetDebugMessage()); + state.GetRejectCode(), state.GetRejectReason(), true, state.GetDebugMessage()); } return false; // state filled in by CheckInputs } @@ -1970,7 +1964,7 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl // CheckTxInputs may return MISSING_INPUTS but we can't return that, as // it's not defined for a block, so we reset the reason flag to CONSENSUS here. state.DoS(100, ValidationInvalidReason::CONSENSUS, false, - state.GetRejectCode(), state.GetRejectReason(), state.CorruptionPossible(), state.GetDebugMessage()); + state.GetRejectCode(), state.GetRejectReason(), state.CorruptionPossible(), state.GetDebugMessage()); } return error("%s: Consensus::CheckTxInputs: %s, %s", __func__, tx.GetHash().ToString(), FormatStateMessage(state)); } From aa502b88d10c2c3ac56d9163555849b96dc4df1e Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Thu, 11 Apr 2019 14:11:48 -0400 Subject: [PATCH 16/20] scripted-diff: Remove DoS calls to CValidationState -BEGIN VERIFY SCRIPT- sed -i 's/\.DoS(\(.*\), REJECT_\(.*\), \(true\|false\)/.DoS(\1, REJECT_\2/' src/validation.cpp src/consensus/tx_verify.cpp src/consensus/tx_check.cpp sed -i 's/state.GetRejectCode(), state.GetRejectReason(), [^,]\+, state.GetDebugMessage())/state.GetRejectCode(), state.GetRejectReason(), state.GetDebugMessage())/' src/validation.cpp sed -i 's/\.DoS([^,]*, /.Invalid\(/' src/validation.cpp src/consensus/tx_verify.cpp src/consensus/tx_check.cpp -END VERIFY SCRIPT- Co-authored-by: Suhas Daftuar --- src/consensus/tx_check.cpp | 18 +++---- src/consensus/tx_verify.cpp | 10 ++-- src/validation.cpp | 100 ++++++++++++++++++------------------ 3 files changed, 64 insertions(+), 64 deletions(-) diff --git a/src/consensus/tx_check.cpp b/src/consensus/tx_check.cpp index 3aa6d3ae1..23ed3ecb5 100644 --- a/src/consensus/tx_check.cpp +++ b/src/consensus/tx_check.cpp @@ -11,24 +11,24 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fChe { // Basic checks that don't depend on any context if (tx.vin.empty()) - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-vin-empty"); + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-vin-empty"); if (tx.vout.empty()) - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-vout-empty"); + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-vout-empty"); // Size limits (this doesn't take the witness into account, as that hasn't been checked for malleability) if (::GetSerializeSize(tx, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT) - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-oversize"); + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-oversize"); // Check for negative or overflow output values CAmount nValueOut = 0; for (const auto& txout : tx.vout) { if (txout.nValue < 0) - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-vout-negative"); + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-vout-negative"); if (txout.nValue > MAX_MONEY) - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-vout-toolarge"); + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-vout-toolarge"); nValueOut += txout.nValue; if (!MoneyRange(nValueOut)) - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-txouttotal-toolarge"); + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-txouttotal-toolarge"); } // Check for duplicate inputs - note that this check is slow so we skip it in CheckBlock @@ -37,20 +37,20 @@ bool CheckTransaction(const CTransaction& tx, CValidationState &state, bool fChe for (const auto& txin : tx.vin) { if (!vInOutPoints.insert(txin.prevout).second) - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-inputs-duplicate"); + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-inputs-duplicate"); } } if (tx.IsCoinBase()) { if (tx.vin[0].scriptSig.size() < 2 || tx.vin[0].scriptSig.size() > 100) - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-cb-length"); + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-cb-length"); } else { for (const auto& txin : tx.vin) if (txin.prevout.IsNull()) - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-prevout-null"); + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-prevout-null"); } return true; diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp index 60af773b1..c13a38cd6 100644 --- a/src/consensus/tx_verify.cpp +++ b/src/consensus/tx_verify.cpp @@ -160,7 +160,7 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, CValidationState& state, c { // are the actual inputs available? if (!inputs.HaveInputs(tx)) { - return state.DoS(0, ValidationInvalidReason::TX_MISSING_INPUTS, false, REJECT_INVALID, "bad-txns-inputs-missingorspent", false, + return state.Invalid(ValidationInvalidReason::TX_MISSING_INPUTS, false, REJECT_INVALID, "bad-txns-inputs-missingorspent", strprintf("%s: inputs missing/spent", __func__)); } @@ -172,27 +172,27 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, CValidationState& state, c // If prev is coinbase, check that it's matured if (coin.IsCoinBase() && nSpendHeight - coin.nHeight < COINBASE_MATURITY) { - return state.DoS(0, ValidationInvalidReason::TX_MISSING_INPUTS, false, REJECT_INVALID, "bad-txns-premature-spend-of-coinbase", false, + return state.Invalid(ValidationInvalidReason::TX_MISSING_INPUTS, false, REJECT_INVALID, "bad-txns-premature-spend-of-coinbase", strprintf("tried to spend coinbase at depth %d", nSpendHeight - coin.nHeight)); } // Check for negative or overflow input values nValueIn += coin.out.nValue; if (!MoneyRange(coin.out.nValue) || !MoneyRange(nValueIn)) { - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-inputvalues-outofrange"); + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-inputvalues-outofrange"); } } const CAmount value_out = tx.GetValueOut(); if (nValueIn < value_out) { - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-in-belowout", false, + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-in-belowout", strprintf("value in (%s) < value out (%s)", FormatMoney(nValueIn), FormatMoney(value_out))); } // Tally transaction fees const CAmount txfee_aux = nValueIn - value_out; if (!MoneyRange(txfee_aux)) { - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-fee-outofrange"); + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-fee-outofrange"); } txfee = txfee_aux; diff --git a/src/validation.cpp b/src/validation.cpp index d7595627e..4184d5a81 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -579,24 +579,24 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool // Coinbase is only valid in a block, not as a loose transaction if (tx.IsCoinBase()) - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "coinbase"); + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "coinbase"); // Rather not work on nonstandard transactions (unless -testnet/-regtest) std::string reason; if (fRequireStandard && !IsStandardTx(tx, reason)) - return state.DoS(0, ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, reason); + return state.Invalid(ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, reason); // Do not work on transactions that are too small. // A transaction with 1 segwit input and 1 P2WPHK output has non-witness size of 82 bytes. // Transactions smaller than this are not relayed to reduce unnecessary malloc overhead. if (::GetSerializeSize(tx, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) < MIN_STANDARD_TX_NONWITNESS_SIZE) - return state.DoS(0, ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, "tx-size-small"); + return state.Invalid(ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, "tx-size-small"); // Only accept nLockTime-using transactions that can be mined in the next // block; we don't want our mempool filled up with transactions that can't // be mined yet. if (!CheckFinalTx(tx, STANDARD_LOCKTIME_VERIFY_FLAGS)) - return state.DoS(0, ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, "non-final"); + return state.Invalid(ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, "non-final"); // is it already in the memory pool? if (pool.exists(hash)) { @@ -685,7 +685,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool // Must keep pool.cs for this unless we change CheckSequenceLocks to take a // CoinsViewCache instead of create its own if (!CheckSequenceLocks(pool, tx, STANDARD_LOCKTIME_VERIFY_FLAGS, &lp)) - return state.DoS(0, ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, "non-BIP68-final"); + return state.Invalid(ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, "non-BIP68-final"); CAmount nFees = 0; if (!Consensus::CheckTxInputs(tx, state, view, GetSpendHeight(view), nFees)) { @@ -698,7 +698,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool // Check for non-standard witness in P2WSH if (tx.HasWitness() && fRequireStandard && !IsWitnessStandard(tx, view)) - return state.DoS(0, ValidationInvalidReason::TX_WITNESS_MUTATED, false, REJECT_NONSTANDARD, "bad-witness-nonstandard", true); + return state.Invalid(ValidationInvalidReason::TX_WITNESS_MUTATED, false, REJECT_NONSTANDARD, "bad-witness-nonstandard"); int64_t nSigOpsCost = GetTransactionSigOpCost(tx, view, STANDARD_SCRIPT_VERIFY_FLAGS); @@ -722,17 +722,17 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool unsigned int nSize = entry.GetTxSize(); if (nSigOpsCost > MAX_STANDARD_TX_SIGOPS_COST) - return state.DoS(0, ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, "bad-txns-too-many-sigops", false, + return state.Invalid(ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, "bad-txns-too-many-sigops", strprintf("%d", nSigOpsCost)); CAmount mempoolRejectFee = pool.GetMinFee(gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000).GetFee(nSize); if (!bypass_limits && mempoolRejectFee > 0 && nModifiedFees < mempoolRejectFee) { - return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "mempool min fee not met", false, strprintf("%d < %d", nModifiedFees, mempoolRejectFee)); + return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "mempool min fee not met", strprintf("%d < %d", nModifiedFees, mempoolRejectFee)); } // No transactions are allowed below minRelayTxFee except from disconnected blocks if (!bypass_limits && nModifiedFees < ::minRelayTxFee.GetFee(nSize)) { - return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "min relay fee not met", false, strprintf("%d < %d", nModifiedFees, ::minRelayTxFee.GetFee(nSize))); + return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "min relay fee not met", strprintf("%d < %d", nModifiedFees, ::minRelayTxFee.GetFee(nSize))); } if (nAbsurdFee && nFees > nAbsurdFee) @@ -748,7 +748,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool size_t nLimitDescendantSize = gArgs.GetArg("-limitdescendantsize", DEFAULT_DESCENDANT_SIZE_LIMIT)*1000; std::string errString; if (!pool.CalculateMemPoolAncestors(entry, setAncestors, nLimitAncestors, nLimitAncestorSize, nLimitDescendants, nLimitDescendantSize, errString)) { - return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too-long-mempool-chain", false, errString); + return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too-long-mempool-chain", errString); } // A transaction that spends outputs that would be replaced by it is invalid. Now @@ -760,7 +760,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool const uint256 &hashAncestor = ancestorIt->GetTx().GetHash(); if (setConflicts.count(hashAncestor)) { - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-spends-conflicting-tx", false, + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-spends-conflicting-tx", strprintf("%s spends conflicting transaction %s", hash.ToString(), hashAncestor.ToString())); @@ -802,7 +802,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool CFeeRate oldFeeRate(mi->GetModifiedFee(), mi->GetTxSize()); if (newFeeRate <= oldFeeRate) { - return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "insufficient fee", false, + return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "insufficient fee", strprintf("rejecting replacement %s; new feerate %s <= old feerate %s", hash.ToString(), newFeeRate.ToString(), @@ -830,7 +830,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool nConflictingSize += it->GetTxSize(); } } else { - return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too many potential replacements", false, + return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too many potential replacements", strprintf("rejecting replacement %s; too many potential replacements (%d > %d)\n", hash.ToString(), nConflictingCount, @@ -849,7 +849,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool // it's cheaper to just check if the new input refers to a // tx that's in the mempool. if (pool.exists(tx.vin[j].prevout.hash)) { - return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "replacement-adds-unconfirmed", false, + return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "replacement-adds-unconfirmed", strprintf("replacement %s adds unconfirmed input, idx %d", hash.ToString(), j)); } @@ -861,7 +861,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool // transactions would not be paid for. if (nModifiedFees < nConflictingFees) { - return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "insufficient fee", false, + return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "insufficient fee", strprintf("rejecting replacement %s, less fees than conflicting txs; %s < %s", hash.ToString(), FormatMoney(nModifiedFees), FormatMoney(nConflictingFees))); } @@ -871,7 +871,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool CAmount nDeltaFees = nModifiedFees - nConflictingFees; if (nDeltaFees < ::incrementalRelayFee.GetFee(nSize)) { - return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "insufficient fee", false, + return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "insufficient fee", strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s", hash.ToString(), FormatMoney(nDeltaFees), @@ -892,8 +892,8 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool if (!tx.HasWitness() && CheckInputs(tx, stateDummy, view, true, scriptVerifyFlags & ~(SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_CLEANSTACK), true, false, txdata) && !CheckInputs(tx, stateDummy, view, true, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true, false, txdata)) { // Only the witness is missing, so the transaction itself may be fine. - state.DoS(0, ValidationInvalidReason::TX_WITNESS_MUTATED, false, - state.GetRejectCode(), state.GetRejectReason(), true, state.GetDebugMessage()); + state.Invalid(ValidationInvalidReason::TX_WITNESS_MUTATED, false, + state.GetRejectCode(), state.GetRejectReason(), state.GetDebugMessage()); } return false; // state filled in by CheckInputs } @@ -951,7 +951,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool if (!bypass_limits) { LimitMempoolSize(pool, gArgs.GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000, gArgs.GetArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY) * 60 * 60); if (!pool.exists(hash)) - return state.DoS(0, ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "mempool full"); + return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_INSUFFICIENTFEE, "mempool full"); } } @@ -1425,7 +1425,7 @@ bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsVi // as to the correct behavior - we may want to continue // peering with non-upgraded nodes even after soft-fork // super-majority signaling has occurred. - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(check.GetScriptError()))); + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(check.GetScriptError()))); } } @@ -1920,7 +1920,7 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl for (const auto& tx : block.vtx) { for (size_t o = 0; o < tx->vout.size(); o++) { if (view.HaveCoin(COutPoint(tx->GetHash(), o))) { - return state.DoS(100, ValidationInvalidReason::CONSENSUS, error("ConnectBlock(): tried to overwrite transaction"), + return state.Invalid(ValidationInvalidReason::CONSENSUS, error("ConnectBlock(): tried to overwrite transaction"), REJECT_INVALID, "bad-txns-BIP30"); } } @@ -1963,14 +1963,14 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl if (state.GetReason() == ValidationInvalidReason::TX_MISSING_INPUTS) { // CheckTxInputs may return MISSING_INPUTS but we can't return that, as // it's not defined for a block, so we reset the reason flag to CONSENSUS here. - state.DoS(100, ValidationInvalidReason::CONSENSUS, false, - state.GetRejectCode(), state.GetRejectReason(), state.CorruptionPossible(), state.GetDebugMessage()); + state.Invalid(ValidationInvalidReason::CONSENSUS, false, + state.GetRejectCode(), state.GetRejectReason(), state.GetDebugMessage()); } return error("%s: Consensus::CheckTxInputs: %s, %s", __func__, tx.GetHash().ToString(), FormatStateMessage(state)); } nFees += txfee; if (!MoneyRange(nFees)) { - return state.DoS(100, ValidationInvalidReason::CONSENSUS, error("%s: accumulated fee in the block out of range.", __func__), + return state.Invalid(ValidationInvalidReason::CONSENSUS, error("%s: accumulated fee in the block out of range.", __func__), REJECT_INVALID, "bad-txns-accumulated-fee-outofrange"); } @@ -1983,7 +1983,7 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl } if (!SequenceLocks(tx, nLockTimeFlags, &prevheights, *pindex)) { - return state.DoS(100, ValidationInvalidReason::CONSENSUS, error("%s: contains a non-BIP68-final transaction", __func__), + return state.Invalid(ValidationInvalidReason::CONSENSUS, error("%s: contains a non-BIP68-final transaction", __func__), REJECT_INVALID, "bad-txns-nonfinal"); } } @@ -1994,7 +1994,7 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl // * witness (when witness enabled in flags and excludes coinbase) nSigOpsCost += GetTransactionSigOpCost(tx, view, flags); if (nSigOpsCost > MAX_BLOCK_SIGOPS_COST) - return state.DoS(100, ValidationInvalidReason::CONSENSUS, error("ConnectBlock(): too many sigops"), + return state.Invalid(ValidationInvalidReason::CONSENSUS, error("ConnectBlock(): too many sigops"), REJECT_INVALID, "bad-blk-sigops"); txdata.emplace_back(tx); @@ -2010,8 +2010,8 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl // In the event of a future soft-fork, we may need to // consider whether rewriting to CONSENSUS or // RECENT_CONSENSUS_CHANGE would be more appropriate. - state.DoS(100, ValidationInvalidReason::CONSENSUS, false, - state.GetRejectCode(), state.GetRejectReason(), state.CorruptionPossible(), state.GetDebugMessage()); + state.Invalid(ValidationInvalidReason::CONSENSUS, false, + state.GetRejectCode(), state.GetRejectReason(), state.GetDebugMessage()); } return error("ConnectBlock(): CheckInputs on %s failed with %s", tx.GetHash().ToString(), FormatStateMessage(state)); @@ -2030,13 +2030,13 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl CAmount blockReward = nFees + GetBlockSubsidy(pindex->nHeight, chainparams.GetConsensus()); if (block.vtx[0]->GetValueOut() > blockReward) - return state.DoS(100, ValidationInvalidReason::CONSENSUS, + return state.Invalid(ValidationInvalidReason::CONSENSUS, error("ConnectBlock(): coinbase pays too much (actual=%d vs limit=%d)", block.vtx[0]->GetValueOut(), blockReward), REJECT_INVALID, "bad-cb-amount"); if (!control.Wait()) - return state.DoS(100, ValidationInvalidReason::CONSENSUS, error("%s: CheckQueue failed", __func__), REJECT_INVALID, "block-validation-failed"); + return state.Invalid(ValidationInvalidReason::CONSENSUS, error("%s: CheckQueue failed", __func__), REJECT_INVALID, "block-validation-failed"); int64_t nTime4 = GetTimeMicros(); nTimeVerify += nTime4 - nTime2; LogPrint(BCLog::BENCH, " - Verify %u txins: %.2fms (%.3fms/txin) [%.2fs (%.2fms/blk)]\n", nInputs - 1, MILLI * (nTime4 - nTime2), nInputs <= 1 ? 0 : MILLI * (nTime4 - nTime2) / (nInputs-1), nTimeVerify * MICRO, nTimeVerify * MILLI / nBlocksTotal); @@ -3062,7 +3062,7 @@ static bool CheckBlockHeader(const CBlockHeader& block, CValidationState& state, { // Check proof of work matches claimed amount if (fCheckPOW && !CheckProofOfWork(block.GetHash(), block.nBits, consensusParams)) - return state.DoS(100, ValidationInvalidReason::BLOCK_INVALID_HEADER, false, REJECT_INVALID, "high-hash", false, "proof of work failed"); + return state.Invalid(ValidationInvalidReason::BLOCK_INVALID_HEADER, false, REJECT_INVALID, "high-hash", "proof of work failed"); return true; } @@ -3084,13 +3084,13 @@ bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::P bool mutated; uint256 hashMerkleRoot2 = BlockMerkleRoot(block, &mutated); if (block.hashMerkleRoot != hashMerkleRoot2) - return state.DoS(100, ValidationInvalidReason::BLOCK_MUTATED, false, REJECT_INVALID, "bad-txnmrklroot", true, "hashMerkleRoot mismatch"); + return state.Invalid(ValidationInvalidReason::BLOCK_MUTATED, false, REJECT_INVALID, "bad-txnmrklroot", "hashMerkleRoot mismatch"); // Check for merkle tree malleability (CVE-2012-2459): repeating sequences // of transactions in a block without affecting the merkle root of a block, // while still invalidating it. if (mutated) - return state.DoS(100, ValidationInvalidReason::BLOCK_MUTATED, false, REJECT_INVALID, "bad-txns-duplicate", true, "duplicate transaction"); + return state.Invalid(ValidationInvalidReason::BLOCK_MUTATED, false, REJECT_INVALID, "bad-txns-duplicate", "duplicate transaction"); } // All potential-corruption validation must be done before we do any @@ -3101,14 +3101,14 @@ bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::P // Size limits if (block.vtx.empty() || block.vtx.size() * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT || ::GetSerializeSize(block, PROTOCOL_VERSION | SERIALIZE_TRANSACTION_NO_WITNESS) * WITNESS_SCALE_FACTOR > MAX_BLOCK_WEIGHT) - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-blk-length", false, "size limits failed"); + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-blk-length", "size limits failed"); // First transaction must be coinbase, the rest must not be if (block.vtx.empty() || !block.vtx[0]->IsCoinBase()) - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-cb-missing", false, "first tx is not coinbase"); + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-cb-missing", "first tx is not coinbase"); for (unsigned int i = 1; i < block.vtx.size(); i++) if (block.vtx[i]->IsCoinBase()) - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-cb-multiple", false, "more than one coinbase"); + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-cb-multiple", "more than one coinbase"); // Check transactions for (const auto& tx : block.vtx) @@ -3122,7 +3122,7 @@ bool CheckBlock(const CBlock& block, CValidationState& state, const Consensus::P nSigOps += GetLegacySigOpCount(*tx); } if (nSigOps * WITNESS_SCALE_FACTOR > MAX_BLOCK_SIGOPS_COST) - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-blk-sigops", false, "out-of-bounds SigOpCount"); + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-blk-sigops", "out-of-bounds SigOpCount"); if (fCheckPOW && fCheckMerkleRoot) block.fChecked = true; @@ -3215,7 +3215,7 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationSta // Check proof of work const Consensus::Params& consensusParams = params.GetConsensus(); if (block.nBits != GetNextWorkRequired(pindexPrev, &block, consensusParams)) - return state.DoS(100, ValidationInvalidReason::BLOCK_INVALID_HEADER, false, REJECT_INVALID, "bad-diffbits", false, "incorrect proof of work"); + return state.Invalid(ValidationInvalidReason::BLOCK_INVALID_HEADER, false, REJECT_INVALID, "bad-diffbits", "incorrect proof of work"); // Check against checkpoints if (fCheckpointsEnabled) { @@ -3224,12 +3224,12 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationSta // MapBlockIndex. CBlockIndex* pcheckpoint = Checkpoints::GetLastCheckpoint(params.Checkpoints()); if (pcheckpoint && nHeight < pcheckpoint->nHeight) - return state.DoS(100, ValidationInvalidReason::BLOCK_CHECKPOINT, error("%s: forked chain older than last checkpoint (height %d)", __func__, nHeight), REJECT_CHECKPOINT, "bad-fork-prior-to-checkpoint"); + return state.Invalid(ValidationInvalidReason::BLOCK_CHECKPOINT, error("%s: forked chain older than last checkpoint (height %d)", __func__, nHeight), REJECT_CHECKPOINT, "bad-fork-prior-to-checkpoint"); } // Check timestamp against prev if (block.GetBlockTime() <= pindexPrev->GetMedianTimePast()) - return state.DoS(100, ValidationInvalidReason::BLOCK_INVALID_HEADER, false, REJECT_INVALID, "time-too-old", false, "block's timestamp is too early"); + return state.Invalid(ValidationInvalidReason::BLOCK_INVALID_HEADER, false, REJECT_INVALID, "time-too-old", "block's timestamp is too early"); // Check timestamp if (block.GetBlockTime() > nAdjustedTime + MAX_FUTURE_BLOCK_TIME) @@ -3240,7 +3240,7 @@ static bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationSta if((block.nVersion < 2 && nHeight >= consensusParams.BIP34Height) || (block.nVersion < 3 && nHeight >= consensusParams.BIP66Height) || (block.nVersion < 4 && nHeight >= consensusParams.BIP65Height)) - return state.DoS(100, ValidationInvalidReason::BLOCK_INVALID_HEADER, false, REJECT_OBSOLETE, strprintf("bad-version(0x%08x)", block.nVersion), false, + return state.Invalid(ValidationInvalidReason::BLOCK_INVALID_HEADER, false, REJECT_OBSOLETE, strprintf("bad-version(0x%08x)", block.nVersion), strprintf("rejected nVersion=0x%08x block", block.nVersion)); return true; @@ -3270,7 +3270,7 @@ static bool ContextualCheckBlock(const CBlock& block, CValidationState& state, c // Check that all transactions are finalized for (const auto& tx : block.vtx) { if (!IsFinalTx(*tx, nHeight, nLockTimeCutoff)) { - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-nonfinal", false, "non-final transaction"); + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-nonfinal", "non-final transaction"); } } @@ -3280,7 +3280,7 @@ static bool ContextualCheckBlock(const CBlock& block, CValidationState& state, c CScript expect = CScript() << nHeight; if (block.vtx[0]->vin[0].scriptSig.size() < expect.size() || !std::equal(expect.begin(), expect.end(), block.vtx[0]->vin[0].scriptSig.begin())) { - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-cb-height", false, "block height mismatch in coinbase"); + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-cb-height", "block height mismatch in coinbase"); } } @@ -3302,11 +3302,11 @@ static bool ContextualCheckBlock(const CBlock& block, CValidationState& state, c // already does not permit it, it is impossible to trigger in the // witness tree. if (block.vtx[0]->vin[0].scriptWitness.stack.size() != 1 || block.vtx[0]->vin[0].scriptWitness.stack[0].size() != 32) { - return state.DoS(100, ValidationInvalidReason::BLOCK_MUTATED, false, REJECT_INVALID, "bad-witness-nonce-size", true, strprintf("%s : invalid witness reserved value size", __func__)); + return state.Invalid(ValidationInvalidReason::BLOCK_MUTATED, false, REJECT_INVALID, "bad-witness-nonce-size", strprintf("%s : invalid witness reserved value size", __func__)); } CHash256().Write(hashWitness.begin(), 32).Write(&block.vtx[0]->vin[0].scriptWitness.stack[0][0], 32).Finalize(hashWitness.begin()); if (memcmp(hashWitness.begin(), &block.vtx[0]->vout[commitpos].scriptPubKey[6], 32)) { - return state.DoS(100, ValidationInvalidReason::BLOCK_MUTATED, false, REJECT_INVALID, "bad-witness-merkle-match", true, strprintf("%s : witness merkle commitment mismatch", __func__)); + return state.Invalid(ValidationInvalidReason::BLOCK_MUTATED, false, REJECT_INVALID, "bad-witness-merkle-match", strprintf("%s : witness merkle commitment mismatch", __func__)); } fHaveWitness = true; } @@ -3316,7 +3316,7 @@ static bool ContextualCheckBlock(const CBlock& block, CValidationState& state, c if (!fHaveWitness) { for (const auto& tx : block.vtx) { if (tx->HasWitness()) { - return state.DoS(100, ValidationInvalidReason::BLOCK_MUTATED, false, REJECT_INVALID, "unexpected-witness", true, strprintf("%s : unexpected witness data found", __func__)); + return state.Invalid(ValidationInvalidReason::BLOCK_MUTATED, false, REJECT_INVALID, "unexpected-witness", strprintf("%s : unexpected witness data found", __func__)); } } } @@ -3328,7 +3328,7 @@ static bool ContextualCheckBlock(const CBlock& block, CValidationState& state, c // the block hash, so we couldn't mark the block as permanently // failed). if (GetBlockWeight(block) > MAX_BLOCK_WEIGHT) { - return state.DoS(100, ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-blk-weight", false, strprintf("%s : weight limit failed", __func__)); + return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-blk-weight", strprintf("%s : weight limit failed", __func__)); } return true; @@ -3359,10 +3359,10 @@ bool CChainState::AcceptBlockHeader(const CBlockHeader& block, CValidationState& CBlockIndex* pindexPrev = nullptr; BlockMap::iterator mi = mapBlockIndex.find(block.hashPrevBlock); if (mi == mapBlockIndex.end()) - return state.DoS(10, ValidationInvalidReason::BLOCK_MISSING_PREV, error("%s: prev block not found", __func__), 0, "prev-blk-not-found"); + return state.Invalid(ValidationInvalidReason::BLOCK_MISSING_PREV, error("%s: prev block not found", __func__), 0, "prev-blk-not-found"); pindexPrev = (*mi).second; if (pindexPrev->nStatus & BLOCK_FAILED_MASK) - return state.DoS(100, ValidationInvalidReason::BLOCK_INVALID_PREV, error("%s: prev block invalid", __func__), REJECT_INVALID, "bad-prevblk"); + return state.Invalid(ValidationInvalidReason::BLOCK_INVALID_PREV, error("%s: prev block invalid", __func__), REJECT_INVALID, "bad-prevblk"); if (!ContextualCheckBlockHeader(block, state, chainparams, pindexPrev, GetAdjustedTime())) return error("%s: Consensus::ContextualCheckBlockHeader: %s, %s", __func__, hash.ToString(), FormatStateMessage(state)); @@ -3399,7 +3399,7 @@ bool CChainState::AcceptBlockHeader(const CBlockHeader& block, CValidationState& setDirtyBlockIndex.insert(invalid_walk); invalid_walk = invalid_walk->pprev; } - return state.DoS(100, ValidationInvalidReason::BLOCK_INVALID_PREV, error("%s: prev block invalid", __func__), REJECT_INVALID, "bad-prevblk"); + return state.Invalid(ValidationInvalidReason::BLOCK_INVALID_PREV, error("%s: prev block invalid", __func__), REJECT_INVALID, "bad-prevblk"); } } } From 12dbdd7a41bac73e51ed8f7b290b7671196bf9ea Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Fri, 18 Jan 2019 12:59:02 +1000 Subject: [PATCH 17/20] [refactor] Drop unused state.DoS(), state.GetDoS(), state.CorruptionPossible() Co-authored-by: Anthony Towns --- src/consensus/validation.h | 40 ++------------------------------------ 1 file changed, 2 insertions(+), 38 deletions(-) diff --git a/src/consensus/validation.h b/src/consensus/validation.h index 9583e6105..ed1b04761 100644 --- a/src/consensus/validation.h +++ b/src/consensus/validation.h @@ -80,18 +80,9 @@ private: std::string strDebugMessage; public: CValidationState() : mode(MODE_VALID), m_reason(ValidationInvalidReason::NONE), chRejectCode(0) {} - bool DoS(int level, ValidationInvalidReason reasonIn, bool ret = false, - unsigned int chRejectCodeIn=0, const std::string &strRejectReasonIn="", - bool corruptionPossibleIn=false, - const std::string &strDebugMessageIn="") { - ret = Invalid(reasonIn, ret, chRejectCodeIn, strRejectReasonIn, strDebugMessageIn); - assert(level == GetDoS()); - assert(corruptionPossibleIn == CorruptionPossible()); - return ret; - } bool Invalid(ValidationInvalidReason reasonIn, bool ret = false, - unsigned int chRejectCodeIn=0, const std::string &strRejectReasonIn="", - const std::string &strDebugMessageIn="") { + unsigned int chRejectCodeIn=0, const std::string &strRejectReasonIn="", + const std::string &strDebugMessageIn="") { m_reason = reasonIn; chRejectCode = chRejectCodeIn; strRejectReason = strRejectReasonIn; @@ -116,33 +107,6 @@ public: bool IsError() const { return mode == MODE_ERROR; } - bool CorruptionPossible() const { - return m_reason == ValidationInvalidReason::BLOCK_MUTATED || m_reason == ValidationInvalidReason::TX_WITNESS_MUTATED; - } - int GetDoS() const { - switch (m_reason) { - case ValidationInvalidReason::NONE: - return 0; - case ValidationInvalidReason::CONSENSUS: - case ValidationInvalidReason::BLOCK_MUTATED: - case ValidationInvalidReason::BLOCK_INVALID_HEADER: - case ValidationInvalidReason::BLOCK_CHECKPOINT: - case ValidationInvalidReason::BLOCK_INVALID_PREV: - return 100; - case ValidationInvalidReason::BLOCK_MISSING_PREV: - return 10; - case ValidationInvalidReason::CACHED_INVALID: - 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: - return 0; - } - return 0; - } ValidationInvalidReason GetReason() const { return m_reason; } unsigned int GetRejectCode() const { return chRejectCode; } std::string GetRejectReason() const { return strRejectReason; } From 2120c31521aa51aa1984ee33250b8320506d3a0f Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 17 Apr 2018 10:46:30 -0400 Subject: [PATCH 18/20] [refactor] Update some comments in validation.cpp as we arent doing DoS there --- src/validation.cpp | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 4184d5a81..a9cf42501 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1410,21 +1410,25 @@ bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsVi // Check whether the failure was caused by a // non-mandatory script verification check, such as // non-standard DER encodings or non-null dummy - // arguments; if so, don't trigger DoS protection to - // avoid splitting the network between upgraded and - // non-upgraded nodes. + // arguments; if so, ensure we return NOT_STANDARD + // instead of CONSENSUS to avoid downstream users + // splitting the network between upgraded and + // non-upgraded nodes by banning CONSENSUS-failing + // data providers. CScriptCheck check2(coin.out, tx, i, flags & ~STANDARD_NOT_MANDATORY_VERIFY_FLAGS, cacheSigStore, &txdata); if (check2()) return state.Invalid(ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(check.GetScriptError()))); } - // Failures of other flags indicate a transaction that is - // invalid in new blocks, e.g. an invalid P2SH. We DoS ban - // such nodes as they are not following the protocol. That - // said during an upgrade careful thought should be taken - // as to the correct behavior - we may want to continue - // peering with non-upgraded nodes even after soft-fork - // super-majority signaling has occurred. + // MANDATORY flag failures correspond to + // ValidationInvalidReason::CONSENSUS. Because CONSENSUS + // failures are the most serious case of validation + // failures, we may need to consider using + // RECENT_CONSENSUS_CHANGE for any script failure that + // could be due to non-upgraded nodes which we may want to + // support, to avoid splitting the network (but this + // depends on the details of how net_processing handles + // such errors). return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(check.GetScriptError()))); } } From 54470e767bab37f9b7089782b1be73d5883bb244 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Thu, 21 Feb 2019 13:46:25 -0500 Subject: [PATCH 19/20] Assert validation reasons are contextually correct --- src/consensus/validation.h | 26 ++++++++++++++++++++++++++ src/net_processing.cpp | 3 +++ src/validation.cpp | 3 +++ 3 files changed, 32 insertions(+) diff --git a/src/consensus/validation.h b/src/consensus/validation.h index ed1b04761..3079f3b08 100644 --- a/src/consensus/validation.h +++ b/src/consensus/validation.h @@ -66,6 +66,32 @@ enum class ValidationInvalidReason { TX_MEMPOOL_POLICY, //!< violated mempool's fee/size/descendant/RBF/etc limits }; +inline bool IsTransactionReason(ValidationInvalidReason r) +{ + return r == ValidationInvalidReason::NONE || + r == ValidationInvalidReason::CONSENSUS || + r == ValidationInvalidReason::RECENT_CONSENSUS_CHANGE || + r == ValidationInvalidReason::TX_NOT_STANDARD || + r == ValidationInvalidReason::TX_MISSING_INPUTS || + r == ValidationInvalidReason::TX_WITNESS_MUTATED || + r == ValidationInvalidReason::TX_CONFLICT || + r == ValidationInvalidReason::TX_MEMPOOL_POLICY; +} + +inline bool IsBlockReason(ValidationInvalidReason r) +{ + return r == ValidationInvalidReason::NONE || + r == ValidationInvalidReason::CONSENSUS || + r == ValidationInvalidReason::RECENT_CONSENSUS_CHANGE || + r == ValidationInvalidReason::CACHED_INVALID || + r == ValidationInvalidReason::BLOCK_INVALID_HEADER || + r == ValidationInvalidReason::BLOCK_MUTATED || + r == ValidationInvalidReason::BLOCK_MISSING_PREV || + r == ValidationInvalidReason::BLOCK_INVALID_PREV || + r == ValidationInvalidReason::BLOCK_TIME_FUTURE || + r == ValidationInvalidReason::BLOCK_CHECKPOINT; +} + /** Capture information about block/transaction validation */ class CValidationState { private: 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. diff --git a/src/validation.cpp b/src/validation.cpp index a9cf42501..c29bff9d3 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -895,6 +895,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool state.Invalid(ValidationInvalidReason::TX_WITNESS_MUTATED, false, state.GetRejectCode(), state.GetRejectReason(), state.GetDebugMessage()); } + assert(IsTransactionReason(state.GetReason())); return false; // state filled in by CheckInputs } @@ -1970,6 +1971,7 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl state.Invalid(ValidationInvalidReason::CONSENSUS, false, state.GetRejectCode(), state.GetRejectReason(), state.GetDebugMessage()); } + assert(IsBlockReason(state.GetReason())); return error("%s: Consensus::CheckTxInputs: %s, %s", __func__, tx.GetHash().ToString(), FormatStateMessage(state)); } nFees += txfee; @@ -3507,6 +3509,7 @@ bool CChainState::AcceptBlock(const std::shared_ptr& pblock, CVali if (!CheckBlock(block, state, chainparams.GetConsensus()) || !ContextualCheckBlock(block, state, chainparams.GetConsensus(), pindex->pprev)) { + assert(IsBlockReason(state.GetReason())); if (state.IsInvalid() && state.GetReason() != ValidationInvalidReason::BLOCK_MUTATED) { pindex->nStatus |= BLOCK_FAILED_VALID; setDirtyBlockIndex.insert(pindex); From 0ff1c2a838da9e8dc7f77609adc89124bbea3e2b Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Fri, 8 Mar 2019 09:55:23 -0500 Subject: [PATCH 20/20] Separate reason for premature spends (coinbase/locktime) --- src/consensus/tx_verify.cpp | 2 +- src/consensus/validation.h | 4 +++- src/net_processing.cpp | 1 + src/validation.cpp | 13 +++++++------ 4 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp index c13a38cd6..4b93cae84 100644 --- a/src/consensus/tx_verify.cpp +++ b/src/consensus/tx_verify.cpp @@ -172,7 +172,7 @@ bool Consensus::CheckTxInputs(const CTransaction& tx, CValidationState& state, c // If prev is coinbase, check that it's matured if (coin.IsCoinBase() && nSpendHeight - coin.nHeight < COINBASE_MATURITY) { - return state.Invalid(ValidationInvalidReason::TX_MISSING_INPUTS, false, REJECT_INVALID, "bad-txns-premature-spend-of-coinbase", + return state.Invalid(ValidationInvalidReason::TX_PREMATURE_SPEND, false, REJECT_INVALID, "bad-txns-premature-spend-of-coinbase", strprintf("tried to spend coinbase at depth %d", nSpendHeight - coin.nHeight)); } diff --git a/src/consensus/validation.h b/src/consensus/validation.h index 3079f3b08..2e23f4b3a 100644 --- a/src/consensus/validation.h +++ b/src/consensus/validation.h @@ -49,7 +49,8 @@ enum class ValidationInvalidReason { BLOCK_CHECKPOINT, //!< the block failed to meet one of our checkpoints // Only loose txn: TX_NOT_STANDARD, //!< didn't meet our local policy rules - TX_MISSING_INPUTS, //!< a transaction was missing some of its inputs (or its inputs were spent at < coinbase maturity height) + TX_MISSING_INPUTS, //!< a transaction was missing some of its inputs + TX_PREMATURE_SPEND, //!< transaction spends a coinbase too early, or violates locktime/sequence locks /** * Transaction might be missing a witness, have a witness prior to SegWit * activation, or witness may have been malleated (which includes @@ -72,6 +73,7 @@ inline bool IsTransactionReason(ValidationInvalidReason r) r == ValidationInvalidReason::CONSENSUS || r == ValidationInvalidReason::RECENT_CONSENSUS_CHANGE || r == ValidationInvalidReason::TX_NOT_STANDARD || + r == ValidationInvalidReason::TX_PREMATURE_SPEND || r == ValidationInvalidReason::TX_MISSING_INPUTS || r == ValidationInvalidReason::TX_WITNESS_MUTATED || r == ValidationInvalidReason::TX_CONFLICT || 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: diff --git a/src/validation.cpp b/src/validation.cpp index c29bff9d3..c394afd82 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -596,7 +596,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool // block; we don't want our mempool filled up with transactions that can't // be mined yet. if (!CheckFinalTx(tx, STANDARD_LOCKTIME_VERIFY_FLAGS)) - return state.Invalid(ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, "non-final"); + return state.Invalid(ValidationInvalidReason::TX_PREMATURE_SPEND, false, REJECT_NONSTANDARD, "non-final"); // is it already in the memory pool? if (pool.exists(hash)) { @@ -685,7 +685,7 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool // Must keep pool.cs for this unless we change CheckSequenceLocks to take a // CoinsViewCache instead of create its own if (!CheckSequenceLocks(pool, tx, STANDARD_LOCKTIME_VERIFY_FLAGS, &lp)) - return state.Invalid(ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, "non-BIP68-final"); + return state.Invalid(ValidationInvalidReason::TX_PREMATURE_SPEND, false, REJECT_NONSTANDARD, "non-BIP68-final"); CAmount nFees = 0; if (!Consensus::CheckTxInputs(tx, state, view, GetSpendHeight(view), nFees)) { @@ -1965,13 +1965,14 @@ bool CChainState::ConnectBlock(const CBlock& block, CValidationState& state, CBl { CAmount txfee = 0; if (!Consensus::CheckTxInputs(tx, state, view, pindex->nHeight, txfee)) { - if (state.GetReason() == ValidationInvalidReason::TX_MISSING_INPUTS) { - // CheckTxInputs may return MISSING_INPUTS but we can't return that, as - // it's not defined for a block, so we reset the reason flag to CONSENSUS here. + if (!IsBlockReason(state.GetReason())) { + // CheckTxInputs may return MISSING_INPUTS or + // PREMATURE_SPEND but we can't return that, as it's not + // defined for a block, so we reset the reason flag to + // CONSENSUS here. state.Invalid(ValidationInvalidReason::CONSENSUS, false, state.GetRejectCode(), state.GetRejectReason(), state.GetDebugMessage()); } - assert(IsBlockReason(state.GetReason())); return error("%s: Consensus::CheckTxInputs: %s, %s", __func__, tx.GetHash().ToString(), FormatStateMessage(state)); } nFees += txfee;