Merge bitcoin/bitcoin#31269: validation: Remove RECENT_CONSENSUS_CHAN…

…GE validation result

e80e4c6 validation: Remove RECENT_CONSENSUS_CHANGE validation result (TheCharlatan)

Pull request description:

  The *_RECENT_CONSENSUS_CHANGE variants in the validation result enumerations were always unused. They seem to have been kept around speculatively for a soft fork after segwit, however they were never used for taproot either. This points at them not having a clear purpose. Based on the original pull requests' comments their usage was never entirely clear:
  bitcoin/bitcoin#11639 (comment) bitcoin/bitcoin#15141 (comment)

  Since they are part of the validation interface and need to be exposed by the kernel library keeping them around may also be confusing to future users of the library.

ACKs for top commit:
  sipa:
    ACK e80e4c6
  naumenkogs:
    ACK bitcoin/bitcoin@e80e4c6
  dergoegge:
    ACK e80e4c6
  ajtowns:
    ACK e80e4c6

Tree-SHA512: 0af17c4435bb1b5a4f43600da30545cbbe95a7d642419cabdefabfb82b9335d92262c1c48be7ca2f2a024078ae9447161228b6f951d2f508a51159a31947fb54

src/bitcoin-chainstate.cpp

@@ -253,9 +253,6 @@ int main(int argc, char* argv[])
         case BlockValidationResult::BLOCK_CONSENSUS:
             std::cerr << "invalid by consensus rules (excluding any below reasons)" << std::endl;
             break;
-        case BlockValidationResult::BLOCK_RECENT_CONSENSUS_CHANGE:
-            std::cerr << "Invalid by a change to consensus rules more recent than SegWit." << std::endl;
-            break;
         case BlockValidationResult::BLOCK_CACHED_INVALID:
             std::cerr << "this block was cached as being invalid and we didn't store the reason why" << std::endl;
             break;

src/consensus/validation.h

@@ -23,14 +23,6 @@ static constexpr size_t MINIMUM_WITNESS_COMMITMENT{38};
 enum class TxValidationResult {
     TX_RESULT_UNSET = 0,     //!< 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.
-     */
-    TX_RECENT_CONSENSUS_CHANGE,
     TX_INPUTS_NOT_STANDARD,   //!< inputs (covered by txid) failed policy rules
     TX_NOT_STANDARD,          //!< otherwise didn't meet our local policy rules
     TX_MISSING_INPUTS,        //!< transaction was missing some of its inputs
@@ -65,14 +57,6 @@ enum class TxValidationResult {
 enum class BlockValidationResult {
     BLOCK_RESULT_UNSET = 0,  //!< 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

src/net_processing.cpp

@@ -1792,7 +1792,6 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati
     case BlockValidationResult::BLOCK_MISSING_PREV:
         if (peer) Misbehaving(*peer, message);
         return;
-    case BlockValidationResult::BLOCK_RECENT_CONSENSUS_CHANGE:
     case BlockValidationResult::BLOCK_TIME_FUTURE:
         break;
     }
@@ -1812,7 +1811,6 @@ void PeerManagerImpl::MaybePunishNodeForTx(NodeId nodeid, const TxValidationStat
         if (peer) Misbehaving(*peer, "");
         return;
     // Conflicting (but not necessarily invalid) data or different policy:
-    case TxValidationResult::TX_RECENT_CONSENSUS_CHANGE:
     case TxValidationResult::TX_INPUTS_NOT_STANDARD:
     case TxValidationResult::TX_NOT_STANDARD:
     case TxValidationResult::TX_MISSING_INPUTS:

src/test/fuzz/partially_downloaded_block.cpp

@@ -114,7 +114,6 @@ FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb)
         fuzzed_data_provider.PickValueInArray(
             {BlockValidationResult::BLOCK_RESULT_UNSET,
              BlockValidationResult::BLOCK_CONSENSUS,
-             BlockValidationResult::BLOCK_RECENT_CONSENSUS_CHANGE,
              BlockValidationResult::BLOCK_CACHED_INVALID,
              BlockValidationResult::BLOCK_INVALID_HEADER,
              BlockValidationResult::BLOCK_MUTATED,

src/test/fuzz/txdownloadman.cpp

@@ -32,7 +32,6 @@ COutPoint COINS[NUM_COINS];
 static TxValidationResult TESTED_TX_RESULTS[] = {
     // Skip TX_RESULT_UNSET
     TxValidationResult::TX_CONSENSUS,
-    TxValidationResult::TX_RECENT_CONSENSUS_CHANGE,
     TxValidationResult::TX_INPUTS_NOT_STANDARD,
     TxValidationResult::TX_NOT_STANDARD,
     TxValidationResult::TX_MISSING_INPUTS,

src/test/txdownload_tests.cpp

@@ -58,7 +58,6 @@ struct Behaviors {
 // Txid and Wtxid are assumed to be different here. For a nonsegwit transaction, use the wtxid results.
 static std::map<TxValidationResult, Behaviors> expected_behaviors{
     {TxValidationResult::TX_CONSENSUS,               {/*txid_rejects*/0,/*wtxid_rejects*/1,/*txid_recon*/0,/*wtxid_recon*/0,/*keep*/1,/*txid_inv*/0,/*wtxid_inv*/1}},
-    {TxValidationResult::TX_RECENT_CONSENSUS_CHANGE, {                0,                 1,              0,               0,        1,            0,             1}},
     {TxValidationResult::TX_INPUTS_NOT_STANDARD,     {                1,                 1,              0,               0,        1,            1,             1}},
     {TxValidationResult::TX_NOT_STANDARD,            {                0,                 1,              0,               0,        1,            0,             1}},
     {TxValidationResult::TX_MISSING_INPUTS,          {                0,                 0,              0,               0,        1,            0,             1}},

src/validation.cpp

@@ -2217,15 +2217,9 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
                 // string by reporting the error from the second check.
                 error = check2.GetScriptError();
             }
+
             // MANDATORY flag failures correspond to
-            // TxValidationResult::TX_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).
+            // TxValidationResult::TX_CONSENSUS.
             return state.Invalid(TxValidationResult::TX_CONSENSUS, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(error)));
         }
     }