From e80e4c6ff91e27d7d40f099a2d7942c29085234c Mon Sep 17 00:00:00 2001 From: TheCharlatan Date: Mon, 11 Nov 2024 10:24:30 +0100 Subject: [PATCH] validation: Remove RECENT_CONSENSUS_CHANGE validation result 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: https://github.com/bitcoin/bitcoin/pull/11639#issuecomment-370234133 https://github.com/bitcoin/bitcoin/pull/15141#discussion_r271039747 Since they are part of the validation interface and need to exposed by the kernel library keeping them around may also be confusing to future users of the library. --- src/bitcoin-chainstate.cpp | 3 --- src/consensus/validation.h | 16 ---------------- src/net_processing.cpp | 2 -- src/test/fuzz/partially_downloaded_block.cpp | 1 - src/test/fuzz/txdownloadman.cpp | 1 - src/test/txdownload_tests.cpp | 1 - src/validation.cpp | 10 ++-------- 7 files changed, 2 insertions(+), 32 deletions(-) diff --git a/src/bitcoin-chainstate.cpp b/src/bitcoin-chainstate.cpp index 9cbafa233dd..4bb463acf53 100644 --- a/src/bitcoin-chainstate.cpp +++ b/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; diff --git a/src/consensus/validation.h b/src/consensus/validation.h index 1556c7888f9..dffa9f2dd6e 100644 --- a/src/consensus/validation.h +++ b/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 diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 170352f7299..482e0f675c1 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1790,7 +1790,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; } @@ -1810,7 +1809,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: diff --git a/src/test/fuzz/partially_downloaded_block.cpp b/src/test/fuzz/partially_downloaded_block.cpp index 77952cab9e5..913d056ff6d 100644 --- a/src/test/fuzz/partially_downloaded_block.cpp +++ b/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, diff --git a/src/test/fuzz/txdownloadman.cpp b/src/test/fuzz/txdownloadman.cpp index a313caa1117..8a4d7d55c26 100644 --- a/src/test/fuzz/txdownloadman.cpp +++ b/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, diff --git a/src/test/txdownload_tests.cpp b/src/test/txdownload_tests.cpp index e401fbf59c4..41f6bd83c5f 100644 --- a/src/test/txdownload_tests.cpp +++ b/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 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}}, diff --git a/src/validation.cpp b/src/validation.cpp index f9288fcc0d7..c6de748f57f 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2201,15 +2201,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))); } }