Merge #15921: validation: Tidy up ValidationState interface

Summary:
3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf [validation] Remove fMissingInputs from AcceptToMemoryPool() (John Newbery)
c428622a5bb1e37b2e6ab2c52791ac05d9271238 [validation] Remove unused first_invalid parameter from ProcessNewBlockHeaders() (John Newbery)
7204c6434b944f6ad51b3c895837729d3aa56eea [validation] Remove useless ret parameter from Invalid() (John Newbery)
1a37de4b3174d19a6d8691ae07e92b32fdfaef11 [validation] Remove error() calls from Invalid() calls (John Newbery)
067981e49246822421a7bcc720491427e1dba8a3 [validation] Tidy Up ValidationResult class (John Newbery)
a27a2957ed9afbe5a96caa5f0f4cbec730d27460 [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 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf
  amitiuttarwar:
    code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf. Also built & ran tests locally.
  fjahr:
    Code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf . Only nit style change and pure virtual destructor added since my last review.
  ryanofsky:
    Code review ACK 3004d5a12d09d94bfc4dee2a8e8f2291996a4aaf. Just whitespace change and pure virtual destructor added since last review.

Tree-SHA512: 511de1fb380a18bec1944ea82b513b6192df632ee08bb16344a2df3c40811a88f3872f04df24bc93a41643c96c48f376a04551840fd804a961490d6c702c3d36

Backport of Core [[bitcoin/bitcoin#15921 | PR15921]] and [[bitcoin/bitcoin#17624 | PR17624]] (small fix to 15921)

Followup to [[bitcoin/bitcoin#15141 | PR15141]]

Some differences reviewers will encounter:
1. RejectCodes (such as REJECT_INVALID) do not appear in the original PR due to an out-of-order backport, but we currently still support them, so it was retained.
2. Some files (notably tests) contain refactors not present in the original PR. This is due to a few out-of-order backports but otherwise harmless.

Depends on D6923, D6929, D6930

Test Plan: `ninja check check-functional-extended`

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Subscribers: deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D6860

src/bench/block_assemble.cpp

@@ -42,7 +42,7 @@ static void AssembleBlock(benchmark::State &state) {
         LOCK(::cs_main);
 
         for (const auto &txr : txs) {
-            CValidationState vstate;
+            TxValidationState vstate;
             bool ret{::AcceptToMemoryPool(config, ::g_mempool, vstate, txr,
                                           false /* bypass_limits */,
                                           /* nAbsurdFee */ Amount::zero())};

src/bench/checkblock.cpp

@@ -46,7 +46,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, params, options);
         assert(checked);
     }

src/bench/duplicate_inputs.cpp

@@ -57,7 +57,7 @@ static void DuplicateInputs(benchmark::State &state) {
     block.hashMerkleRoot = BlockMerkleRoot(block);
 
     while (state.KeepRunning()) {
-        CValidationState cvstate{};
+        BlockValidationState cvstate{};
         assert(!CheckBlock(block, cvstate, chainparams.GetConsensus(),
                            BlockValidationOptions(GetConfig())
                                .withCheckPoW(false)

src/blockencodings.cpp

@@ -240,14 +240,14 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(
         return READ_STATUS_INVALID;
     }
 
-    CValidationState state;
+    BlockValidationState state;
     if (!CheckBlock(block, state, config->GetChainParams().GetConsensus(),
                     BlockValidationOptions(*config))) {
         // 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) {
             // Possible Short ID collision.
             return READ_STATUS_FAILED;
         }

src/consensus/tx_check.cpp

@@ -11,40 +11,40 @@
 #include <unordered_set>
 
 static bool CheckTransactionCommon(const CTransaction &tx,
-                                   CValidationState &state) {
+                                   TxValidationState &state) {
     // Basic checks that don't depend on any context
     if (tx.vin.empty()) {
-        return state.Invalid(ValidationInvalidReason::CONSENSUS, REJECT_INVALID,
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, REJECT_INVALID,
                              "bad-txns-vin-empty");
     }
 
     if (tx.vout.empty()) {
-        return state.Invalid(ValidationInvalidReason::CONSENSUS, REJECT_INVALID,
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, REJECT_INVALID,
                              "bad-txns-vout-empty");
     }
 
     // Size limit
     if (::GetSerializeSize(tx, PROTOCOL_VERSION) > MAX_TX_SIZE) {
-        return state.Invalid(ValidationInvalidReason::CONSENSUS, REJECT_INVALID,
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, REJECT_INVALID,
                              "bad-txns-oversize");
     }
 
     // Check for negative or overflow output values
     Amount nValueOut = Amount::zero();
     for (const auto &txout : tx.vout) {
         if (txout.nValue < Amount::zero()) {
-            return state.Invalid(ValidationInvalidReason::CONSENSUS,
+            return state.Invalid(TxValidationResult::TX_CONSENSUS,
                                  REJECT_INVALID, "bad-txns-vout-negative");
         }
 
         if (txout.nValue > MAX_MONEY) {
-            return state.Invalid(ValidationInvalidReason::CONSENSUS,
+            return state.Invalid(TxValidationResult::TX_CONSENSUS,
                                  REJECT_INVALID, "bad-txns-vout-toolarge");
         }
 
         nValueOut += txout.nValue;
         if (!MoneyRange(nValueOut)) {
-            return state.Invalid(ValidationInvalidReason::CONSENSUS,
+            return state.Invalid(TxValidationResult::TX_CONSENSUS,
                                  REJECT_INVALID,
                                  "bad-txns-txouttotal-toolarge");
         }
@@ -53,9 +53,9 @@ static bool CheckTransactionCommon(const CTransaction &tx,
     return true;
 }
 
-bool CheckCoinbase(const CTransaction &tx, CValidationState &state) {
+bool CheckCoinbase(const CTransaction &tx, TxValidationState &state) {
     if (!tx.IsCoinBase()) {
-        return state.Invalid(ValidationInvalidReason::CONSENSUS, REJECT_INVALID,
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, REJECT_INVALID,
                              "bad-cb-missing", "first tx is not coinbase");
     }
 
@@ -66,16 +66,16 @@ bool CheckCoinbase(const CTransaction &tx, CValidationState &state) {
 
     if (tx.vin[0].scriptSig.size() < 2 ||
         tx.vin[0].scriptSig.size() > MAX_COINBASE_SCRIPTSIG_SIZE) {
-        return state.Invalid(ValidationInvalidReason::CONSENSUS, REJECT_INVALID,
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, REJECT_INVALID,
                              "bad-cb-length");
     }
 
     return true;
 }
 
-bool CheckRegularTransaction(const CTransaction &tx, CValidationState &state) {
+bool CheckRegularTransaction(const CTransaction &tx, TxValidationState &state) {
     if (tx.IsCoinBase()) {
-        return state.Invalid(ValidationInvalidReason::CONSENSUS, REJECT_INVALID,
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, REJECT_INVALID,
                              "bad-tx-coinbase");
     }
 
@@ -87,7 +87,7 @@ bool CheckRegularTransaction(const CTransaction &tx, CValidationState &state) {
     std::unordered_set<COutPoint, SaltedOutpointHasher> vInOutPoints;
     for (const auto &txin : tx.vin) {
         if (txin.prevout.IsNull()) {
-            return state.Invalid(ValidationInvalidReason::CONSENSUS,
+            return state.Invalid(TxValidationResult::TX_CONSENSUS,
                                  REJECT_INVALID, "bad-txns-prevout-null");
         }
 
@@ -98,7 +98,7 @@ bool CheckRegularTransaction(const CTransaction &tx, CValidationState &state) {
         // will result in either a crash or an inflation bug, depending on the
         // implementation of the underlying coins database.
         if (!vInOutPoints.insert(txin.prevout).second) {
-            return state.Invalid(ValidationInvalidReason::CONSENSUS,
+            return state.Invalid(TxValidationResult::TX_CONSENSUS,
                                  REJECT_INVALID, "bad-txns-inputs-duplicate");
         }
     }

src/consensus/tx_check.h

@@ -13,14 +13,14 @@
  */
 
 class CTransaction;
-class CValidationState;
+class TxValidationState;
 
 /**
  * Context-independent validity checks for coinbase and non-coinbase
  * transactions.
  */
 
-bool CheckRegularTransaction(const CTransaction &tx, CValidationState &state);
-bool CheckCoinbase(const CTransaction &tx, CValidationState &state);
+bool CheckRegularTransaction(const CTransaction &tx, TxValidationState &state);
+bool CheckCoinbase(const CTransaction &tx, TxValidationState &state);
 
 #endif // BITCOIN_CONSENSUS_TX_CHECK_H

src/consensus/tx_verify.cpp

@@ -38,20 +38,21 @@ static bool IsFinalTx(const CTransaction &tx, int nBlockHeight,
 }
 
 bool ContextualCheckTransaction(const Consensus::Params &params,
-                                const CTransaction &tx, CValidationState &state,
-                                int nHeight, int64_t nLockTimeCutoff,
+                                const CTransaction &tx,
+                                TxValidationState &state, int nHeight,
+                                int64_t nLockTimeCutoff,
                                 int64_t nMedianTimePast) {
     if (!IsFinalTx(tx, nHeight, nLockTimeCutoff)) {
         // While this is only one transaction, we use txns in the error to
         // ensure continuity with other clients.
-        return state.Invalid(ValidationInvalidReason::CONSENSUS, REJECT_INVALID,
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, REJECT_INVALID,
                              "bad-txns-nonfinal", "non-final transaction");
     }
 
     if (IsMagneticAnomalyEnabled(params, nHeight)) {
         // Size limit
         if (::GetSerializeSize(tx, PROTOCOL_VERSION) < MIN_TX_SIZE) {
-            return state.Invalid(ValidationInvalidReason::CONSENSUS,
+            return state.Invalid(TxValidationResult::TX_CONSENSUS,
                                  REJECT_INVALID, "bad-txns-undersize");
         }
     }
@@ -154,12 +155,12 @@ bool SequenceLocks(const CTransaction &tx, int flags,
 }
 
 namespace Consensus {
-bool CheckTxInputs(const CTransaction &tx, CValidationState &state,
+bool CheckTxInputs(const CTransaction &tx, TxValidationState &state,
                    const CCoinsViewCache &inputs, int nSpendHeight,
                    Amount &txfee) {
     // are the actual inputs available?
     if (!inputs.HaveInputs(tx)) {
-        return state.Invalid(ValidationInvalidReason::TX_MISSING_INPUTS,
+        return state.Invalid(TxValidationResult::TX_MISSING_INPUTS,
                              REJECT_INVALID, "bad-txns-inputs-missingorspent",
                              strprintf("%s: inputs missing/spent", __func__));
     }
@@ -174,7 +175,7 @@ bool CheckTxInputs(const CTransaction &tx, CValidationState &state,
         if (coin.IsCoinBase() &&
             nSpendHeight - coin.GetHeight() < COINBASE_MATURITY) {
             return state.Invalid(
-                ValidationInvalidReason::TX_PREMATURE_SPEND, REJECT_INVALID,
+                TxValidationResult::TX_PREMATURE_SPEND, REJECT_INVALID,
                 "bad-txns-premature-spend-of-coinbase",
                 strprintf("tried to spend coinbase at depth %d",
                           nSpendHeight - coin.GetHeight()));
@@ -183,15 +184,15 @@ bool CheckTxInputs(const CTransaction &tx, CValidationState &state,
         // Check for negative or overflow input values
         nValueIn += coin.GetTxOut().nValue;
         if (!MoneyRange(coin.GetTxOut().nValue) || !MoneyRange(nValueIn)) {
-            return state.Invalid(ValidationInvalidReason::CONSENSUS,
+            return state.Invalid(TxValidationResult::TX_CONSENSUS,
                                  REJECT_INVALID,
                                  "bad-txns-inputvalues-outofrange");
         }
     }
 
     const Amount value_out = tx.GetValueOut();
     if (nValueIn < value_out) {
-        return state.Invalid(ValidationInvalidReason::CONSENSUS, REJECT_INVALID,
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, REJECT_INVALID,
                              "bad-txns-in-belowout",
                              strprintf("value in (%s) < value out (%s)",
                                        FormatMoney(nValueIn),
@@ -201,7 +202,7 @@ bool CheckTxInputs(const CTransaction &tx, CValidationState &state,
     // Tally transaction fees
     const Amount txfee_aux = nValueIn - value_out;
     if (!MoneyRange(txfee_aux)) {
-        return state.Invalid(ValidationInvalidReason::CONSENSUS, REJECT_INVALID,
+        return state.Invalid(TxValidationResult::TX_CONSENSUS, REJECT_INVALID,
                              "bad-txns-fee-outofrange");
     }
 

src/consensus/tx_verify.h

@@ -12,7 +12,7 @@ struct Amount;
 class CBlockIndex;
 class CCoinsViewCache;
 class CTransaction;
-class CValidationState;
+class TxValidationState;
 
 namespace Consensus {
 struct Params;
@@ -24,7 +24,7 @@ struct Params;
  * @param[out] txfee Set to the transaction fee if successful.
  * Preconditions: tx.IsCoinBase() is false.
  */
-bool CheckTxInputs(const CTransaction &tx, CValidationState &state,
+bool CheckTxInputs(const CTransaction &tx, TxValidationState &state,
                    const CCoinsViewCache &inputs, int nSpendHeight,
                    Amount &txfee);
 
@@ -37,8 +37,9 @@ bool CheckTxInputs(const CTransaction &tx, CValidationState &state,
  * activation/deactivation and CLTV.
  */
 bool ContextualCheckTransaction(const Consensus::Params &params,
-                                const CTransaction &tx, CValidationState &state,
-                                int nHeight, int64_t nLockTimeCutoff,
+                                const CTransaction &tx,
+                                TxValidationState &state, int nHeight,
+                                int64_t nLockTimeCutoff,
                                 int64_t nMedianTimePast);
 
 /**