Merge #15921: validation: Tidy up ValidationState interface

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

Tree-SHA512: 511de1fb380a18bec1944ea82b513b6192df632ee08bb16344a2df3c40811a88f3872f04df24bc93a41643c96c48f376a04551840fd804a961490d6c702c3d36

src/test/blockfilter_index_tests.cpp

@@ -102,8 +102,8 @@ static bool BuildChain(const CBlockIndex* pindex, const CScript& coinbase_script
         block = std::make_shared<CBlock>(CreateBlock(pindex, no_txns, coinbase_script_pub_key));
         CBlockHeader header = block->GetBlockHeader();
 
-        CValidationState state;
-        if (!ProcessNewBlockHeaders({header}, state, Params(), &pindex, nullptr)) {
+        BlockValidationState state;
+        if (!ProcessNewBlockHeaders({header}, state, Params(), &pindex)) {
             return false;
         }
     }

src/test/fuzz/transaction.cpp

@@ -42,7 +42,7 @@ void test_one_input(const std::vector<uint8_t>& buffer)
         return;
     }
 
-    CValidationState state_with_dupe_check;
+    TxValidationState state_with_dupe_check;
     (void)CheckTransaction(tx, state_with_dupe_check);
 
     const CFeeRate dust_relay_fee{DUST_RELAY_TX_FEE};

src/test/setup_common.cpp

@@ -97,7 +97,7 @@ TestingSetup::TestingSetup(const std::string& chainName) : BasicTestingSetup(cha
         throw std::runtime_error("LoadGenesisBlock failed.");
     }
 
-    CValidationState state;
+    BlockValidationState state;
     if (!ActivateBestChain(state, chainparams)) {
         throw std::runtime_error(strprintf("ActivateBestChain failed. (%s)", FormatStateMessage(state)));
     }

src/test/sighash_tests.cpp

@@ -193,7 +193,7 @@ BOOST_AUTO_TEST_CASE(sighash_from_data)
           CDataStream stream(ParseHex(raw_tx), SER_NETWORK, PROTOCOL_VERSION);
           stream >> tx;
 
-          CValidationState state;
+          TxValidationState state;
           BOOST_CHECK_MESSAGE(CheckTransaction(*tx, state), strTest);
           BOOST_CHECK(state.IsValid());
 

src/test/transaction_tests.cpp

@@ -152,7 +152,7 @@ BOOST_AUTO_TEST_CASE(tx_valid)
             CDataStream stream(ParseHex(transaction), SER_NETWORK, PROTOCOL_VERSION);
             CTransaction tx(deserialize, stream);
 
-            CValidationState state;
+            TxValidationState state;
             BOOST_CHECK_MESSAGE(CheckTransaction(tx, state), strTest);
             BOOST_CHECK(state.IsValid());
 
@@ -239,7 +239,7 @@ BOOST_AUTO_TEST_CASE(tx_invalid)
             CDataStream stream(ParseHex(transaction), SER_NETWORK, PROTOCOL_VERSION );
             CTransaction tx(deserialize, stream);
 
-            CValidationState state;
+            TxValidationState state;
             fValid = CheckTransaction(tx, state) && state.IsValid();
 
             PrecomputedTransactionData txdata(tx);
@@ -274,7 +274,7 @@ BOOST_AUTO_TEST_CASE(basic_transaction_tests)
     CDataStream stream(vch, SER_DISK, CLIENT_VERSION);
     CMutableTransaction tx;
     stream >> tx;
-    CValidationState state;
+    TxValidationState state;
     BOOST_CHECK_MESSAGE(CheckTransaction(CTransaction(tx), state) && state.IsValid(), "Simple deserialized transaction should be valid.");
 
     // Check that duplicate txins fail

src/test/txvalidation_tests.cpp

@@ -30,7 +30,7 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_reject_coinbase, TestChain100Setup)
 
     BOOST_CHECK(CTransaction(coinbaseTx).IsCoinBase());
 
-    CValidationState state;
+    TxValidationState state;
 
     LOCK(cs_main);
 
@@ -39,7 +39,6 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_reject_coinbase, TestChain100Setup)
     BOOST_CHECK_EQUAL(
             false,
             AcceptToMemoryPool(mempool, state, MakeTransactionRef(coinbaseTx),
-                nullptr /* pfMissingInputs */,
                 nullptr /* plTxnReplaced */,
                 true /* bypass_limits */,
                 0 /* nAbsurdFee */));
@@ -50,7 +49,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_CHECK(state.GetResult() == TxValidationResult::TX_CONSENSUS);
 }
 
 BOOST_AUTO_TEST_SUITE_END()

src/test/txvalidationcache_tests.cpp

@@ -13,7 +13,7 @@
 
 #include <boost/test/unit_test.hpp>
 
-bool CheckInputs(const CTransaction& tx, CValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck> *pvChecks);
+bool CheckInputs(const CTransaction& tx, TxValidationState &state, const CCoinsViewCache &inputs, unsigned int flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, std::vector<CScriptCheck> *pvChecks);
 
 BOOST_AUTO_TEST_SUITE(tx_validationcache_tests)
 
@@ -22,8 +22,8 @@ ToMemPool(const CMutableTransaction& tx)
 {
     LOCK(cs_main);
 
-    CValidationState state;
-    return AcceptToMemoryPool(mempool, state, MakeTransactionRef(tx), nullptr /* pfMissingInputs */,
+    TxValidationState state;
+    return AcceptToMemoryPool(mempool, state, MakeTransactionRef(tx),
                               nullptr /* plTxnReplaced */, true /* bypass_limits */, 0 /* nAbsurdFee */);
 }
 
@@ -114,7 +114,7 @@ static void ValidateCheckInputsForAllFlags(const CTransaction &tx, uint32_t fail
     // If we add many more flags, this loop can get too expensive, but we can
     // rewrite in the future to randomly pick a set of flags to evaluate.
     for (uint32_t test_flags=0; test_flags < (1U << 16); test_flags += 1) {
-        CValidationState state;
+        TxValidationState state;
         // Filter out incompatible flag choices
         if ((test_flags & SCRIPT_VERIFY_CLEANSTACK)) {
             // CLEANSTACK requires P2SH and WITNESS, see VerifyScript() in
@@ -201,7 +201,7 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
     {
         LOCK(cs_main);
 
-        CValidationState state;
+        TxValidationState state;
         PrecomputedTransactionData ptd_spend_tx(spend_tx);
 
         BOOST_CHECK(!CheckInputs(CTransaction(spend_tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, nullptr));
@@ -270,7 +270,7 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
 
         // Make it valid, and check again
         invalid_with_cltv_tx.vin[0].scriptSig = CScript() << vchSig << 100;
-        CValidationState state;
+        TxValidationState state;
         PrecomputedTransactionData txdata(invalid_with_cltv_tx);
         BOOST_CHECK(CheckInputs(CTransaction(invalid_with_cltv_tx), state, ::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY, true, true, txdata, nullptr));
     }
@@ -298,7 +298,7 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
 
         // Make it valid, and check again
         invalid_with_csv_tx.vin[0].scriptSig = CScript() << vchSig << 100;
-        CValidationState state;
+        TxValidationState state;
         PrecomputedTransactionData txdata(invalid_with_csv_tx);
         BOOST_CHECK(CheckInputs(CTransaction(invalid_with_csv_tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_CHECKSEQUENCEVERIFY, true, true, txdata, nullptr));
     }
@@ -359,7 +359,7 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, TestChain100Setup)
         // Invalidate vin[1]
         tx.vin[1].scriptWitness.SetNull();
 
-        CValidationState state;
+        TxValidationState state;
         PrecomputedTransactionData txdata(tx);
         // This transaction is now invalid under segwit, because of the second input.
         BOOST_CHECK(!CheckInputs(CTransaction(tx), state, &::ChainstateActive().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, true, txdata, nullptr));

src/test/validation_block_tests.cpp

@@ -151,7 +151,7 @@ BOOST_AUTO_TEST_CASE(processnewblock_signals_ordering)
     }
 
     bool ignored;
-    CValidationState state;
+    BlockValidationState state;
     std::vector<CBlockHeader> headers;
     std::transform(blocks.begin(), blocks.end(), std::back_inserter(headers), [](std::shared_ptr<const CBlock> b) { return b->GetBlockHeader(); });
 
@@ -278,14 +278,13 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg)
         // Add the txs to the tx pool
         {
             LOCK(cs_main);
-            CValidationState state;
+            TxValidationState state;
             std::list<CTransactionRef> plTxnReplaced;
             for (const auto& tx : txs) {
                 BOOST_REQUIRE(AcceptToMemoryPool(
                     ::mempool,
                     state,
                     tx,
-                    /* pfMissingInputs */ &ignored,
                     &plTxnReplaced,
                     /* bypass_limits */ false,
                     /* nAbsurdFee */ 0));