Merge #15921: validation: Tidy up ValidationState interface

3004d5a [validation] Remove fMissingInputs from AcceptToMemoryPool() (John Newbery)
c428622 [validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders() (John Newbery)
7204c64 [validation] Remove useless ret parameter from Invalid() (John Newbery)
1a37de4 [validation] Remove error() calls from Invalid() calls (John Newbery)
067981e [validation] Tidy Up ValidationResult class (John Newbery)
a27a295 [validation] Add CValidationState subclasses (John Newbery)

Pull request description:

  Carries out some remaining tidy-ups remaining after PR 15141:

  - split ValidationState into TxValidationState and BlockValidationState (commit from ajtowns)
  - various minor code style tidy-ups to the ValidationState class
  - remove the useless `ret` parameter from `ValidationState::Invalid()`
  - remove the now unused `first_invalid` parameter from `ProcessNewBlockHeaders()`
  - remove the `fMissingInputs` parameter from `AcceptToMemoryPool()`, and deal with missing inputs the same way as other errors by using the `TxValidationState` object.

  Tip for reviewers (thanks ryanofsky!): The first commit ("[validation] Add CValidationState subclasses" ) is huge and can be easier to start reviewing if you revert the rote, mechanical changes:

  Substitute the commit hash of commit "[validation] Add CValidationState subclasses" for <CommitHash> in the commands below.

  ```sh
  git checkout <CommitHash>
  git grep -l ValidationState | xargs sed -i 's/BlockValidationState\|TxValidationState/CValidationState/g'
  git grep -l ValidationResult | xargs sed -i 's/BlockValidationResult\|TxValidationResult/ValidationInvalidReason/g'
  git grep -l MaybePunish | xargs sed -i 's/MaybePunishNode\(ForBlock\|ForTx\)/MaybePunishNode/g'
  git diff HEAD^
  ```

  After that it's possible to easily see the mechanical changes with:

  ```sh
  git log -p -n1 -U0 --word-diff-regex=. <CommitHash>
  ```

ACKs for top commit:
  laanwj:
    ACK 3004d5a
  amitiuttarwar:
    code review ACK 3004d5a. Also built & ran tests locally.
  fjahr:
    Code review ACK 3004d5a . Only nit style change and pure virtual destructor added since my last review.
  ryanofsky:
    Code review ACK 3004d5a. Just whitespace change and pure virtual destructor added since last review.

Tree-SHA512: 511de1fb380a18bec1944ea82b513b6192df632ee08bb16344a2df3c40811a88f3872f04df24bc93a41643c96c48f376a04551840fd804a961490d6c702c3d36

Author: laanwj

src/bench/block_assemble.cpp

@@ -38,8 +38,8 @@ static void AssembleBlock(benchmark::State& state)
         LOCK(::cs_main); // Required for ::AcceptToMemoryPool.
 
         for (const auto& txr : txs) {
-            CValidationState state;
-            bool ret{::AcceptToMemoryPool(::mempool, state, txr, nullptr /* pfMissingInputs */, nullptr /* plTxnReplaced */, false /* bypass_limits */, /* nAbsurdFee */ 0)};
+            TxValidationState state;
+            bool ret{::AcceptToMemoryPool(::mempool, state, txr, nullptr /* plTxnReplaced */, false /* bypass_limits */, /* nAbsurdFee */ 0)};
             assert(ret);
         }
     }

src/bench/checkblock.cpp

@@ -42,7 +42,7 @@ static void DeserializeAndCheckBlockTest(benchmark::State& state)
         bool rewound = stream.Rewind(benchmark::data::block413567.size());
         assert(rewound);
 
-        CValidationState validationState;
+        BlockValidationState validationState;
         bool checked = CheckBlock(block, validationState, chainParams->GetConsensus());
         assert(checked);
     }

src/bench/duplicate_inputs.cpp

@@ -54,7 +54,7 @@ static void DuplicateInputs(benchmark::State& state)
     block.hashMerkleRoot = BlockMerkleRoot(block);
 
     while (state.KeepRunning()) {
-        CValidationState cvstate{};
+        BlockValidationState cvstate{};
         assert(!CheckBlock(block, cvstate, chainparams.GetConsensus(), false, false));
         assert(cvstate.GetRejectReason() == "bad-txns-inputs-duplicate");
     }

src/blockencodings.cpp

@@ -197,13 +197,13 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
     if (vtx_missing.size() != tx_missing_offset)
         return READ_STATUS_INVALID;
 
-    CValidationState state;
+    BlockValidationState state;
     if (!CheckBlock(block, state, Params().GetConsensus())) {
         // TODO: We really want to just check merkle tree manually here,
         // 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.GetReason() == ValidationInvalidReason::BLOCK_MUTATED)
+        if (state.GetResult() == BlockValidationResult::BLOCK_MUTATED)
             return READ_STATUS_FAILED; // Possible Short ID collision
         return READ_STATUS_CHECKBLOCK_FAILED;
     }

src/consensus/tx_check.cpp

@@ -7,28 +7,28 @@
 #include <primitives/transaction.h>
 #include <consensus/validation.h>
 
-bool CheckTransaction(const CTransaction& tx, CValidationState& state)
+bool CheckTransaction(const CTransaction& tx, TxValidationState& state)
 {
     // Basic checks that don't depend on any context
     if (tx.vin.empty())
-        return state.Invalid(ValidationInvalidReason::CONSENSUS, false, "bad-txns-vin-empty");
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-vin-empty");
     if (tx.vout.empty())
-        return state.Invalid(ValidationInvalidReason::CONSENSUS, false, "bad-txns-vout-empty");
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, "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.Invalid(ValidationInvalidReason::CONSENSUS, false, "bad-txns-oversize");
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-oversize");
 
     // Check for negative or overflow output values (see CVE-2010-5139)
     CAmount nValueOut = 0;
     for (const auto& txout : tx.vout)
     {
         if (txout.nValue < 0)
-            return state.Invalid(ValidationInvalidReason::CONSENSUS, false, "bad-txns-vout-negative");
+            return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-vout-negative");
         if (txout.nValue > MAX_MONEY)
-            return state.Invalid(ValidationInvalidReason::CONSENSUS, false, "bad-txns-vout-toolarge");
+            return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-vout-toolarge");
         nValueOut += txout.nValue;
         if (!MoneyRange(nValueOut))
-            return state.Invalid(ValidationInvalidReason::CONSENSUS, false, "bad-txns-txouttotal-toolarge");
+            return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-txouttotal-toolarge");
     }
 
     // Check for duplicate inputs (see CVE-2018-17144)
@@ -39,19 +39,19 @@ bool CheckTransaction(const CTransaction& tx, CValidationState& state)
     std::set<COutPoint> vInOutPoints;
     for (const auto& txin : tx.vin) {
         if (!vInOutPoints.insert(txin.prevout).second)
-            return state.Invalid(ValidationInvalidReason::CONSENSUS, false, "bad-txns-inputs-duplicate");
+            return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputs-duplicate");
     }
 
     if (tx.IsCoinBase())
     {
         if (tx.vin[0].scriptSig.size() < 2 || tx.vin[0].scriptSig.size() > 100)
-            return state.Invalid(ValidationInvalidReason::CONSENSUS, false, "bad-cb-length");
+            return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-cb-length");
     }
     else
     {
         for (const auto& txin : tx.vin)
             if (txin.prevout.IsNull())
-                return state.Invalid(ValidationInvalidReason::CONSENSUS, false, "bad-txns-prevout-null");
+                return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-prevout-null");
     }
 
     return true;

src/consensus/tx_check.h

@@ -13,8 +13,8 @@
  */
 
 class CTransaction;
-class CValidationState;
+class TxValidationState;
 
-bool CheckTransaction(const CTransaction& tx, CValidationState& state);
+bool CheckTransaction(const CTransaction& tx, TxValidationState& state);
 
 #endif // BITCOIN_CONSENSUS_TX_CHECK_H

src/consensus/tx_verify.cpp

@@ -156,11 +156,11 @@ int64_t GetTransactionSigOpCost(const CTransaction& tx, const CCoinsViewCache& i
     return nSigOps;
 }
 
-bool Consensus::CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, CAmount& txfee)
+bool Consensus::CheckTxInputs(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, CAmount& txfee)
 {
     // are the actual inputs available?
     if (!inputs.HaveInputs(tx)) {
-        return state.Invalid(ValidationInvalidReason::TX_MISSING_INPUTS, false, "bad-txns-inputs-missingorspent",
+        return state.Invalid(TxValidationResult::TX_MISSING_INPUTS, "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.Invalid(ValidationInvalidReason::TX_PREMATURE_SPEND, false, "bad-txns-premature-spend-of-coinbase",
+            return state.Invalid(TxValidationResult::TX_PREMATURE_SPEND, "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.Invalid(ValidationInvalidReason::CONSENSUS, false, "bad-txns-inputvalues-outofrange");
+            return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-inputvalues-outofrange");
         }
     }
 
     const CAmount value_out = tx.GetValueOut();
     if (nValueIn < value_out) {
-        return state.Invalid(ValidationInvalidReason::CONSENSUS, false, "bad-txns-in-belowout",
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, "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.Invalid(ValidationInvalidReason::CONSENSUS, false, "bad-txns-fee-outofrange");
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-fee-outofrange");
     }
 
     txfee = txfee_aux;

src/consensus/tx_verify.h

@@ -13,7 +13,7 @@
 class CBlockIndex;
 class CCoinsViewCache;
 class CTransaction;
-class CValidationState;
+class TxValidationState;
 
 /** Transaction validation functions */
 
@@ -24,7 +24,7 @@ namespace Consensus {
  * @param[out] txfee Set to the transaction fee if successful.
  * Preconditions: tx.IsCoinBase() is false.
  */
-bool CheckTxInputs(const CTransaction& tx, CValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, CAmount& txfee);
+bool CheckTxInputs(const CTransaction& tx, TxValidationState& state, const CCoinsViewCache& inputs, int nSpendHeight, CAmount& txfee);
 } // namespace Consensus
 
 /** Auxiliary functions for transaction validation (ideally should not be exposed) */

src/consensus/validation.h

@@ -12,32 +12,22 @@
 #include <primitives/transaction.h>
 #include <primitives/block.h>
 
-/** A "reason" why something was invalid, suitable for determining whether the
-  * provider of the object should be banned/ignored/disconnected/etc.
+/** A "reason" why a transaction was invalid, suitable for determining whether the
+  * provider of the transaction should be banned/ignored/disconnected/etc.
   */
-enum class ValidationInvalidReason {
-    // txn and blocks:
-    NONE,                    //!< not actually invalid
-    CONSENSUS,               //!< invalid by consensus rules (excluding any below reasons)
+enum class TxValidationResult {
+    TX_RESULT_UNSET,         //!< initial value. Tx has not yet been rejected
+    TX_CONSENSUS,            //!< invalid by consensus rules
     /**
      * 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_RECENT_CONSENSUS_CHANGE,
     TX_NOT_STANDARD,          //!< didn't meet our local policy rules
-    TX_MISSING_INPUTS,        //!< a transaction was missing some of its inputs
+    TX_MISSING_INPUTS,        //!< 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
@@ -48,82 +38,107 @@ enum class ValidationInvalidReason {
     /**
      * 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
+     * Currently this is only used if the transaction already exists in the mempool or on chain.
      */
     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;
-}
+/** A "reason" why a block was invalid, suitable for determining whether the
+  * provider of the block 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 BlockValidationResult {
+    BLOCK_RESULT_UNSET,      //!< initial value. Block has not yet been rejected
+    BLOCK_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.
+     */
+    BLOCK_RECENT_CONSENSUS_CHANGE,
+    BLOCK_CACHED_INVALID,    //!< this block was cached as being invalid and we didn't store the reason 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
+};
 
-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 {
+
+/** Base class for capturing information about block/transaction validation. This is subclassed
+ *  by TxValidationState and BlockValidationState for validation information on transactions
+ *  and blocks respectively. */
+class ValidationState {
 private:
     enum mode_state {
         MODE_VALID,   //!< everything ok
         MODE_INVALID, //!< network rule violation (DoS value may be set)
         MODE_ERROR,   //!< run-time error
-    } mode;
-    ValidationInvalidReason m_reason;
-    std::string strRejectReason;
-    std::string strDebugMessage;
-public:
-    CValidationState() : mode(MODE_VALID), m_reason(ValidationInvalidReason::NONE) {}
-    bool Invalid(ValidationInvalidReason reasonIn, bool ret = false,
-            const std::string &strRejectReasonIn="",
-            const std::string &strDebugMessageIn="") {
-        m_reason = reasonIn;
-        strRejectReason = strRejectReasonIn;
-        strDebugMessage = strDebugMessageIn;
-        if (mode == MODE_ERROR)
-            return ret;
-        mode = MODE_INVALID;
-        return ret;
+    } m_mode;
+    std::string m_reject_reason;
+    std::string m_debug_message;
+protected:
+    void Invalid(const std::string &reject_reason="",
+                 const std::string &debug_message="")
+    {
+        m_reject_reason = reject_reason;
+        m_debug_message = debug_message;
+        if (m_mode != MODE_ERROR) m_mode = MODE_INVALID;
     }
-    bool Error(const std::string& strRejectReasonIn) {
-        if (mode == MODE_VALID)
-            strRejectReason = strRejectReasonIn;
-        mode = MODE_ERROR;
+public:
+    // ValidationState is abstract. Have a pure virtual destructor.
+    virtual ~ValidationState() = 0;
+
+    ValidationState() : m_mode(MODE_VALID) {}
+    bool Error(const std::string& reject_reason)
+    {
+        if (m_mode == MODE_VALID)
+            m_reject_reason = reject_reason;
+        m_mode = MODE_ERROR;
         return false;
     }
-    bool IsValid() const {
-        return mode == MODE_VALID;
-    }
-    bool IsInvalid() const {
-        return mode == MODE_INVALID;
+    bool IsValid() const { return m_mode == MODE_VALID; }
+    bool IsInvalid() const { return m_mode == MODE_INVALID; }
+    bool IsError() const { return m_mode == MODE_ERROR; }
+    std::string GetRejectReason() const { return m_reject_reason; }
+    std::string GetDebugMessage() const { return m_debug_message; }
+};
+
+inline ValidationState::~ValidationState() {};
+
+class TxValidationState : public ValidationState {
+private:
+    TxValidationResult m_result;
+public:
+    bool Invalid(TxValidationResult result,
+                 const std::string &reject_reason="",
+                 const std::string &debug_message="")
+    {
+        m_result = result;
+        ValidationState::Invalid(reject_reason, debug_message);
+        return false;
     }
-    bool IsError() const {
-        return mode == MODE_ERROR;
+    TxValidationResult GetResult() const { return m_result; }
+};
+
+class BlockValidationState : public ValidationState {
+private:
+    BlockValidationResult m_result;
+public:
+    bool Invalid(BlockValidationResult result,
+                 const std::string &reject_reason="",
+                 const std::string &debug_message="") {
+        m_result = result;
+        ValidationState::Invalid(reject_reason, debug_message);
+        return false;
     }
-    ValidationInvalidReason GetReason() const { return m_reason; }
-    std::string GetRejectReason() const { return strRejectReason; }
-    std::string GetDebugMessage() const { return strDebugMessage; }
+    BlockValidationResult GetResult() const { return m_result; }
 };
 
 // These implement the weight = (stripped_size * 4) + witness_size formula,

src/init.cpp

@@ -710,7 +710,7 @@ static void ThreadImport(std::vector<fs::path> vImportFiles)
     }
 
     // scan for better chains in the block chain database, that are not yet connected in the active best chain
-    CValidationState state;
+    BlockValidationState state;
     if (!ActivateBestChain(state, chainparams)) {
         LogPrintf("Failed to connect best block (%s)\n", FormatStateMessage(state));
         StartShutdown();