Merge bitcoin#15141: Rewrite DoS interface between validation and net…

…_processing

0ff1c2a Separate reason for premature spends (coinbase/locktime) (Suhas Daftuar)
54470e7 Assert validation reasons are contextually correct (Suhas Daftuar)
2120c31 [refactor] Update some comments in validation.cpp as we arent doing DoS there (Matt Corallo)
12dbdd7 [refactor] Drop unused state.DoS(), state.GetDoS(), state.CorruptionPossible() (Matt Corallo)
aa502b8 scripted-diff: Remove DoS calls to CValidationState (Matt Corallo)
7721ad6 [refactor] Prep for scripted-diff by removing some \ns which annoy sed. (Matt Corallo)
5e78c57 Allow use of state.Invalid() for all reasons (Matt Corallo)
6b34bc6 Fix handling of invalid headers (Suhas Daftuar)
ef54b48 [refactor] Use Reasons directly instead of DoS codes (Matt Corallo)
9ab2a04 CorruptionPossible -> BLOCK_MUTATED (Matt Corallo)
6e55b29 CorruptionPossible -> TX_WITNESS_MUTATED (Matt Corallo)
7df16e7 LookupBlockIndex -> CACHED_INVALID (Matt Corallo)
c8b0d22 [refactor] Drop redundant nDoS, corruptionPossible, SetCorruptionPossible (Matt Corallo)
34477cc [refactor] Add useful-for-dos "reason" field to CValidationState (Matt Corallo)
6a7f877 Ban all peers for all block script failures (Suhas Daftuar)
7b99910 Clean up banning levels (Matt Corallo)
b8b4c80 [refactor] drop IsInvalid(nDoSOut) (Matt Corallo)
8818729 [refactor] Refactor misbehavior ban decisions to MaybePunishNode() (Matt Corallo)
00e11e6 [refactor] rename stateDummy -> orphan_state (Matt Corallo)
f34fa71 Drop obsolete sigops comment (Matt Corallo)

Pull request description:

  This is a rebase of bitcoin#11639 with some fixes for the last few comments which were not yet addressed.

  The original PR text, with some strikethroughs of text that is no longer correct:

  > This cleans up an old main-carryover - it made sense that main could decide what DoS scores to assign things because the DoS scores were handled in a different part of main, but now validation is telling net_processing what DoS scores to assign to different things, which is utter nonsense. Instead, we replace CValidationState's nDoS and CorruptionPossible with a general ValidationInvalidReason, which net_processing can handle as it sees fit. I keep the behavior changes here to a minimum, but in the future we can utilize these changes for other smarter behavior, such as disconnecting/preferring to rotate outbound peers based on them providing things which are invalid due to SOFT_FORK because we shouldn't ban for such cases.
  >
  > This is somewhat complementary with, though obviously conflicts heavily with bitcoin#11523, which added enums in place of DoS scores, as well as a few other cleanups (which are still relevant).
  >
  > 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.
  > ~~BIP68-violation no longer results in a ban as it is SOFT_FORK.~~
  > ~~Non-SegWit SigOp violation no longer results in a ban as it
  > considers P2SH sigops and is thus SOFT_FORK.~~
  > ~~Any script violation in a block no longer results in a ban as
  > it may be the result of a SOFT_FORK. This should likely be
  > fixed in the future by differentiating between them.~~
  > 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.

  Note: The change to ban all peers for consensus violations is actually NOT the change I'd like to make -- I'd prefer to only ban outbound peers in those situations.  The current behavior is a bit of a mess, however, and so in the interests of advancing this PR I tried to keep the changes to a minimum.  I plan to revisit the behavior in a followup PR.

  EDIT: One reviewer suggested I add some additional context for this PR:

  > The goal of this work was to make net_processing aware of the actual reasons for validation failures, rather than just deal with opaque numbers instructing it to do something.
  >
  > In the future, I'd like to make it so that we use more context to decide how to punish a peer. One example is to differentiate inbound and outbound peer misbehaviors. Another potential example is if we'd treat RECENT_CONSENSUS_CHANGE failures differently (ie after the next consensus change is implemented), and perhaps again we'd want to treat some peers differently than others.

ACKs for commit 0ff1c2:
  jnewbery:
    utACK 0ff1c2a
  ryanofsky:
    utACK 0ff1c2a. Only change is dropping the first commit (f3883a3), and dropping the temporary `assert(level == GetDoS())` that was in 35ee77f (now c8b0d22)

Tree-SHA512: e915a411100876398af5463d0a885920e44d473467bb6af991ef2e8f2681db6c1209bb60f848bd154be72d460f039b5653df20a6840352c5f7ea5486d9f777a3

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;
     }

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(10, 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(10, 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, 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, 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, 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, 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, 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, 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(10, false, REJECT_INVALID, "bad-txns-prevout-null");
+                return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-prevout-null");
     }
 
     return true;

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.Invalid(ValidationInvalidReason::TX_MISSING_INPUTS, false, REJECT_INVALID, "bad-txns-inputs-missingorspent",
                          strprintf("%s: inputs missing/spent", __func__));
     }
 
@@ -172,28 +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.Invalid(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));
         }
 
         // 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.Invalid(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.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, false, REJECT_INVALID, "bad-txns-fee-outofrange");
+        return state.Invalid(ValidationInvalidReason::CONSENSUS, false, REJECT_INVALID, "bad-txns-fee-outofrange");
     }
 
     txfee = txfee_aux;

src/consensus/validation.h

@@ -22,6 +22,78 @@ 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
+    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
+     * 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
+};
+
+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_PREMATURE_SPEND ||
+           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:
@@ -30,32 +102,24 @@ class CValidationState {
         MODE_INVALID, //!< network rule violation (DoS value may be set)
         MODE_ERROR,   //!< run-time error
     } mode;
-    int nDoS;
+    ValidationInvalidReason m_reason;
     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,
-             unsigned int chRejectCodeIn=0, const std::string &strRejectReasonIn="",
-             bool corruptionIn=false,
-             const std::string &strDebugMessageIn="") {
+    CValidationState() : mode(MODE_VALID), m_reason(ValidationInvalidReason::NONE), chRejectCode(0) {}
+    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;
-        corruptionPossible = corruptionIn;
         strDebugMessage = strDebugMessageIn;
         if (mode == MODE_ERROR)
             return ret;
-        nDoS += level;
         mode = MODE_INVALID;
         return ret;
     }
-    bool Invalid(bool ret = false,
-                 unsigned int _chRejectCode=0, const std::string &_strRejectReason="",
-                 const std::string &_strDebugMessage="") {
-        return DoS(0, ret, _chRejectCode, _strRejectReason, false, _strDebugMessage);
-    }
     bool Error(const std::string& strRejectReasonIn) {
         if (mode == MODE_VALID)
             strRejectReason = strRejectReasonIn;
@@ -71,19 +135,7 @@ class CValidationState {
     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;
-    }
-    void SetCorruptionPossible() {
-        corruptionPossible = true;
-    }
+    ValidationInvalidReason GetReason() const { return m_reason; }
     unsigned int GetRejectCode() const { return chRejectCode; }
     std::string GetRejectReason() const { return strRejectReason; }
     std::string GetDebugMessage() const { return strDebugMessage; }