policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only)

ajtowns commented at 9:08 am on April 10, 2024: contributor

This PR changes -acceptnonstdtxn=1 so that it also skips the non-mandatory script checks, allowing txs that (eg) use OP_NOPx or OP_SUCCESS into the mempool. This remains only available on test nets.

DrahtBot commented at 9:08 am on April 10, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29843.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	benthecarman, glozow, fjahr, 1440000bytes, ismaelsadeeq

If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#34875 (refactor: separate deferred script check collection from CheckInputScripts by l0rinc)
#32575 (refactor: Remove special treatment for single threaded script checking by fjahr)
#32317 (kernel: Separate UTXO set access from validation functions by sedited)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

CheckInputScripts(tx, state, &active_coins_tip, test_flags, /*check_for_block=*/false, true, add_to_cache, txdata, validation_cache, nullptr) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(tx, state, &active_coins_tip, test_flags, /*check_for_block=*/false, true, add_to_cache, txdata, validation_cache, &scriptchecks) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(tx, state, &active_coins_tip, test_flags, /*check_for_block=*/false, true, add_to_cache, txdata, validation_cache, &scriptchecks) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(CTransaction(spend_tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, /*check_for_block=*/false, true, true, ptd_spend_tx, m_node.chainman->m_validation_cache, nullptr) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(CTransaction(spend_tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, /*check_for_block=*/false, true, true, ptd_spend_tx, m_node.chainman->m_validation_cache, &scriptchecks) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(CTransaction(invalid_with_cltv_tx), state, m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY, /*check_for_block=*/false, true, true, txdata, m_node.chainman->m_validation_cache, nullptr) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(CTransaction(invalid_with_csv_tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKSEQUENCEVERIFY, /*check_for_block=*/false, true, true, txdata, m_node.chainman->m_validation_cache, nullptr) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(CTransaction(tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, /*check_for_block=*/false, true, true, txdata, m_node.chainman->m_validation_cache, nullptr) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(CTransaction(tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, /*check_for_block=*/false, true, true, txdata, m_node.chainman->m_validation_cache, &scriptchecks) in src/test/txvalidationcache_tests.cpp
CheckInputScripts(tx, state, m_view, scriptVerifyFlags, /*check_for_block=*/false, true, false, ws.m_precomputed_txdata, GetValidationCache()) in src/validation.cpp
test_transaction_acceptance(self.nodes[1], self.std_node, tx2, True, False, 'mempool-script-verify-flag-failed (Using non-compressed keys in segwit)') in test/functional/p2p_segwit.py
test_transaction_acceptance(self.nodes[1], self.std_node, tx3, True, False, 'mempool-script-verify-flag-failed (Using non-compressed keys in segwit)') in test/functional/p2p_segwit.py
test_transaction_acceptance(self.nodes[1], self.std_node, tx4, True, False, 'mempool-script-verify-flag-failed (Using non-compressed keys in segwit)') in test/functional/p2p_segwit.py

2026-04-07 12:54:55

DrahtBot added the label TX fees and policy on Apr 10, 2024

ajtowns commented at 9:15 am on April 10, 2024: contributor

Previous discussion in #28334.

If you enable this feature as a non-mining node, you risk having txs pinned in your mempool that conflict or double-spend standard txs in the normal mempool, which may cause problems with applications or layer-2 protocols.

If you enable this feature as a mining node, you have a small risk of ending up with invalid txs remaining in your mempool if a non-standard tx is accepted in your mempool and remains in there during a soft-fork activation that renders the tx invalid. At that point if that tx is included in blocks you generate, they will be invalid. (EDIT: Restarting your node after activation occurs will dump the mempool to disk and reload it under the new consensus rules, which will correct such problems, however)

For these reasons this feature continues to only be available on test nets, not mainnet.

Having this be available in mainline bitcoin may make it easier to observe test transactions using proposed new soft-fork features (eg OP_CAT or SIGHASH_APO is enabled via the inquisition client which considers those txs standard, but the mainline client will consider them non-standard and has no option to accept them). discussion link

EDIT 2025/03/25: There are more or less three reasons why things are marked non-standard:

some are upgrade hooks (future witness versions, future taproot versions, future tapscript pubkey types, annex usage, NOP and OP_SUCCESS opcodes, tx versions greater than 3, currently unused scriptPubKey types)
others are DoS mitigations (p2sh sigop limit, dangerous scriptPubKey types, …)
others are just to make things nicer for users/miners (tx weight/sigop limit to make packing blocks easier, various things to reduce malleability, limit wasteful utxos, etc…)

Prior to this PR, -acceptnonstdtxn enables txs that include features from each of these categories (eg for upgrades, txs with annex data via IsWitnessStandard being disabled, higher tx versions via IsStandardTx being disabled; for DoS, excessive p2sh sigops via IsStandardTx being disabled; for user friendliness, creation of ephemeral dust due to ephemeral checks being disabled). This PR likewise enables features from each category (NOPS, OP_SUCCESS, tapscript pubkey type, new taproot versions are upgrade features; CONST_SCRIPTCODE is a DoS mitigation; LOW_S, MINIMALIF, CLEANSTACK etc are user friendliness).

Separating these into different categories that can be controlled separately might be worthwhile, but doesn’t seem necessary for this PR.

ajtowns requested review from instagibbs on Apr 10, 2024

1440000bytes commented at 7:02 pm on April 12, 2024: none

Concept ACK

benthecarman commented at 7:36 pm on April 12, 2024: contributor

Concept ACK this would be nice for testing soft forks on test networks

luke-jr commented at 1:44 am on April 21, 2024: member

Seems like this goes beyond the expected behaviour. Maybe make it =2 at least?

glozow commented at 11:20 am on April 22, 2024: member

concept ACK fwiw

instagibbs commented at 1:53 pm on April 22, 2024: member

Seems like this goes beyond the expected behaviour. Maybe make it =2 at least?

Since these are test-network-only changes, are we required to preserve current behavior?

twosatsmaxi commented at 3:54 pm on April 25, 2024: none

Following.

DrahtBot added the label CI failed on May 15, 2024

DrahtBot commented at 0:51 am on May 15, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/23651574970

ajtowns force-pushed on May 16, 2024

ajtowns commented at 3:54 pm on May 16, 2024: contributor

Bumped past #29086

DrahtBot removed the label CI failed on May 16, 2024

luke-jr commented at 1:43 am on June 8, 2024: member

Since these are test-network-only changes, are we required to preserve current behavior?

Whether you are required to or not, it is the correct thing to do. This option originated in Knots where it has always been available on mainnet, and expected to not enable upgradable opcodes/etc.

achow101 requested review from glozow on Oct 15, 2024

DrahtBot added the label CI failed on Mar 17, 2025

DrahtBot commented at 7:46 pm on March 17, 2025: contributor

Needs rebase, if still relevant.

ajtowns force-pushed on Mar 24, 2025

DrahtBot removed the label CI failed on Mar 24, 2025

fjahr commented at 4:06 pm on March 24, 2025: contributor

Concept ACK

DrahtBot added the label Needs rebase on Aug 12, 2025

ajtowns force-pushed on Aug 13, 2025

DrahtBot added the label CI failed on Aug 13, 2025

DrahtBot removed the label Needs rebase on Aug 13, 2025

ajtowns marked this as a draft on Aug 14, 2025

ajtowns force-pushed on Aug 28, 2025

DrahtBot removed the label CI failed on Aug 28, 2025

ajtowns marked this as ready for review on Aug 28, 2025

DrahtBot added the label Needs rebase on Oct 7, 2025

ajtowns force-pushed on Nov 18, 2025

DrahtBot removed the label Needs rebase on Nov 18, 2025

sedited commented at 12:07 pm on March 19, 2026: contributor

@instagibbs, @ismaelsadeeq can you take a look here?

ismaelsadeeq commented at 2:24 pm on March 29, 2026: member

Concept ACK for the advantages stated.

I think this also makes it clear that a transaction input script must violate the mandatory script verify flags to be a consensus failure; other flags failure are just standardness.

I think the second commit adds some brittleness to the CheckInputScripts function; it now needs to know whether we are in block validation or individual transaction validation. I think that should not be the case; the function should be as generic as possible, and each caller should know that context and hence use it as necessary. So CheckInputScripts should just return a validation state indicating whether consensus or standardness is violated; callers should add the detail of whether it is block validation or mempool acceptance validation that triggered the failure.

This makes the testing more straightforward, I think, and is better than the current smoke test.

See diff below.

I think after this PR, in non-block input script validation path, the result enum after a consensus input script failure is TX_NOT_STANDARD, which is incorrect; it should be TX_CONSENSUS.

The diff below should fix this as well.

  0diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp
  1index c8cfbadfa9..9edad1d020 100644
  2--- a/src/test/txvalidationcache_tests.cpp
  3+++ b/src/test/txvalidationcache_tests.cpp
  4@@ -20,8 +20,8 @@ struct Dersig100Setup : public TestChain100Setup {
  5         : TestChain100Setup{ChainType::REGTEST, {.extra_args = {"-testactivationheight=dersig@102"}}} {}
  6 };
  7
  8-bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
  9-                       const CCoinsViewCache& inputs, script_verify_flags flags, bool check_for_block,
 10+TxValidationState CheckInputScripts(const CTransaction& tx,
 11+                       const CCoinsViewCache& inputs, script_verify_flags flags,
 12                        bool cacheSigStore,
 13                        bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
 14                        ValidationCache& validation_cache,
 15@@ -110,6 +110,21 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, Dersig100Setup)
 16     BOOST_CHECK_EQUAL(m_node.mempool->size(), 0U);
 17 }
 18
 19+void ValidTxValidationState(const TxValidationState& ret)
 20+{
 21+    BOOST_CHECK_EQUAL(ret.GetResult(), TxValidationResult::TX_RESULT_UNSET);
 22+    BOOST_CHECK(ret.GetRejectReason().empty());
 23+    BOOST_CHECK(ret.GetDebugMessage().empty());
 24+}
 25+
 26+void InvalidTxValidationState(const TxValidationState& ret, TxValidationResult result, const std::string& reject_reason, const std::string& debug_message)
 27+{
 28+    BOOST_CHECK(!ret.IsValid());
 29+    BOOST_CHECK_EQUAL(ret.GetResult(), result);
 30+    BOOST_CHECK_EQUAL(ret.GetRejectReason(), reject_reason);
 31+    BOOST_CHECK_EQUAL(ret.GetDebugMessage(), debug_message);
 32+}
 33+
 34 // Run CheckInputScripts (using CoinsTip()) on the given transaction, for all script
 35 // flags.  Test that CheckInputScripts passes for all flags that don't overlap with
 36 // the failing_flags argument, but otherwise fails.
 37@@ -128,8 +143,6 @@ static void ValidateCheckInputsForAllFlags(const CTransaction &tx, script_verify
 38     FastRandomContext insecure_rand(true);
 39
 40     for (int count = 0; count < 10000; ++count) {
 41-        TxValidationState state;
 42-
 43         // Randomly selects flag combinations
 44         script_verify_flags test_flags = script_verify_flags::from_int(insecure_rand.randrange(MAX_SCRIPT_VERIFY_FLAGS));
 45
 46@@ -143,23 +156,35 @@ static void ValidateCheckInputsForAllFlags(const CTransaction &tx, script_verify
 47             // WITNESS requires P2SH
 48             test_flags |= SCRIPT_VERIFY_P2SH;
 49         }
 50-        bool ret = CheckInputScripts(tx, state, &active_coins_tip, test_flags, /*check_for_block=*/false, true, add_to_cache, txdata, validation_cache, nullptr);
 51+        auto ret = CheckInputScripts(tx, &active_coins_tip, test_flags, true, add_to_cache, txdata, validation_cache, nullptr);
 52         // CheckInputScripts should succeed iff test_flags doesn't intersect with
 53         // failing_flags
 54         bool expected_return_value = !(test_flags & failing_flags);
 55-        BOOST_CHECK_EQUAL(ret, expected_return_value);
 56+        BOOST_CHECK_EQUAL(ret.IsValid(), expected_return_value);
 57+        if (!ret.IsValid()) {
 58+            if (test_flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) {
 59+                BOOST_CHECK_EQUAL(ret.GetResult(), TxValidationResult::TX_NOT_STANDARD);
 60+            } else {
 61+                BOOST_CHECK_EQUAL(ret.GetResult(), TxValidationResult::TX_CONSENSUS);
 62+            }
 63+            BOOST_CHECK(!ret.GetRejectReason().empty());
 64+            BOOST_CHECK(!ret.GetDebugMessage().empty());
 65+        } else {
 66+           ValidTxValidationState(ret);
 67+        }
 68
 69         // Test the caching
 70-        if (ret && add_to_cache) {
 71+        if (ret.IsValid() && add_to_cache) {
 72             // Check that we get a cache hit if the tx was valid
 73             std::vector<CScriptCheck> scriptchecks;
 74-            BOOST_CHECK(CheckInputScripts(tx, state, &active_coins_tip, test_flags, /*check_for_block=*/false, true, add_to_cache, txdata, validation_cache, &scriptchecks));
 75+            BOOST_CHECK(CheckInputScripts(tx, &active_coins_tip, test_flags, true, add_to_cache, txdata, validation_cache, &scriptchecks).IsValid());
 76             BOOST_CHECK(scriptchecks.empty());
 77         } else {
 78             // Check that we get script executions to check, if the transaction
 79             // was invalid, or we didn't add to cache.
 80             std::vector<CScriptCheck> scriptchecks;
 81-            BOOST_CHECK(CheckInputScripts(tx, state, &active_coins_tip, test_flags, /*check_for_block=*/false, true, add_to_cache, txdata, validation_cache, &scriptchecks));
 82+            auto cached_ret{CheckInputScripts(tx, &active_coins_tip, test_flags, true, add_to_cache, txdata, validation_cache, &scriptchecks)};
 83+            ValidTxValidationState(cached_ret);
 84             BOOST_CHECK_EQUAL(scriptchecks.size(), tx.vin.size());
 85         }
 86     }
 87@@ -214,16 +239,16 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup)
 88     {
 89         LOCK(cs_main);
 90
 91-        TxValidationState state;
 92         PrecomputedTransactionData ptd_spend_tx;
 93-
 94-        BOOST_CHECK(!CheckInputScripts(CTransaction(spend_tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, /*check_for_block=*/false, true, true, ptd_spend_tx, m_node.chainman->m_validation_cache, nullptr));
 95+        auto ret = CheckInputScripts(CTransaction(spend_tx), &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, m_node.chainman->m_validation_cache, nullptr);
 96+        InvalidTxValidationState(ret, TxValidationResult::TX_CONSENSUS, "Non-canonical DER signature", strprintf("input 0 of %s (wtxid %s), spending %s:0", spend_tx.GetHash().ToString(), CTransaction(spend_tx).GetWitnessHash().ToString(), m_coinbase_txns[0]->GetHash().ToString()));
 97
 98         // If we call again asking for scriptchecks (as happens in
 99         // ConnectBlock), we should add a script check object for this -- we're
100         // not caching invalidity (if that changes, delete this test case).
101         std::vector<CScriptCheck> scriptchecks;
102-        BOOST_CHECK(CheckInputScripts(CTransaction(spend_tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, /*check_for_block=*/false, true, true, ptd_spend_tx, m_node.chainman->m_validation_cache, &scriptchecks));
103+        auto non_cached_ret{CheckInputScripts(CTransaction(spend_tx), &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true, true, ptd_spend_tx, m_node.chainman->m_validation_cache, &scriptchecks)};
104+        ValidTxValidationState(non_cached_ret);
105         BOOST_CHECK_EQUAL(scriptchecks.size(), 1U);
106
107         // Test that CheckInputScripts returns true iff DERSIG-enforcing flags are
108@@ -283,9 +308,9 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup)
109
110         // Make it valid, and check again
111         invalid_with_cltv_tx.vin[0].scriptSig = CScript() << vchSig << 100;
112-        TxValidationState state;
113         PrecomputedTransactionData txdata;
114-        BOOST_CHECK(CheckInputScripts(CTransaction(invalid_with_cltv_tx), state, m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY, /*check_for_block=*/false, true, true, txdata, m_node.chainman->m_validation_cache, nullptr));
115+        auto valid_ret{CheckInputScripts(CTransaction(invalid_with_cltv_tx), m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY, true, true, txdata, m_node.chainman->m_validation_cache, nullptr)};
116+        ValidTxValidationState(valid_ret);
117     }
118
119     // TEST CHECKSEQUENCEVERIFY
120@@ -311,9 +336,9 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup)
121
122         // Make it valid, and check again
123         invalid_with_csv_tx.vin[0].scriptSig = CScript() << vchSig << 100;
124-        TxValidationState state;
125         PrecomputedTransactionData txdata;
126-        BOOST_CHECK(CheckInputScripts(CTransaction(invalid_with_csv_tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKSEQUENCEVERIFY, /*check_for_block=*/false, true, true, txdata, m_node.chainman->m_validation_cache, nullptr));
127+        auto valid_ret{CheckInputScripts(CTransaction(invalid_with_csv_tx), &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKSEQUENCEVERIFY, true, true, txdata, m_node.chainman->m_validation_cache, nullptr)};
128+        ValidTxValidationState(valid_ret);
129     }
130
131     // TODO: add tests for remaining script flags
132@@ -372,15 +397,16 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup)
133         // Invalidate vin[1]
134         tx.vin[1].scriptWitness.SetNull();
135
136-        TxValidationState state;
137         PrecomputedTransactionData txdata;
138         // This transaction is now invalid under segwit, because of the second input.
139-        BOOST_CHECK(!CheckInputScripts(CTransaction(tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, /*check_for_block=*/false, true, true, txdata, m_node.chainman->m_validation_cache, nullptr));
140+        auto ret = CheckInputScripts(CTransaction(tx), &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, true, txdata, m_node.chainman->m_validation_cache, nullptr);
141+        InvalidTxValidationState(ret, TxValidationResult::TX_CONSENSUS, "Witness program hash mismatch", strprintf("input 1 of %s (wtxid %s), spending %s:1", tx.GetHash().ToString(), CTransaction(tx).GetWitnessHash().ToString(), spend_tx.GetHash().ToString()));
142
143         std::vector<CScriptCheck> scriptchecks;
144         // Make sure this transaction was not cached (ie because the first
145         // input was valid)
146-        BOOST_CHECK(CheckInputScripts(CTransaction(tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, /*check_for_block=*/false, true, true, txdata, m_node.chainman->m_validation_cache, &scriptchecks));
147+        auto valid_ret{CheckInputScripts(CTransaction(tx), &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, true, txdata, m_node.chainman->m_validation_cache, &scriptchecks)};
148+        ValidTxValidationState(valid_ret);
149         // Should get 2 script checks back -- caching is on a whole-transaction basis.
150         BOOST_CHECK_EQUAL(scriptchecks.size(), 2U);
151     }
152diff --git a/src/validation.cpp b/src/validation.cpp
153index 5d95edf4db..6c3644f9a5 100644
154--- a/src/validation.cpp
155+++ b/src/validation.cpp
156@@ -138,8 +138,8 @@ const CBlockIndex* Chainstate::FindForkInGlobalIndex(const CBlockLocator& locato
157     return m_chain.Genesis();
158 }
159
160-bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
161-                       const CCoinsViewCache& inputs, script_verify_flags flags, bool check_for_block,
162+TxValidationState CheckInputScripts(const CTransaction& tx,
163+                       const CCoinsViewCache& inputs, script_verify_flags flags,
164                        bool cacheSigStore,
165                        bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
166                        ValidationCache& validation_cache,
167@@ -429,7 +429,11 @@ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, TxValidationS
168     }
169
170     // Call CheckInputScripts() to cache signature and script validity against current tip consensus rules.
171-    return CheckInputScripts(tx, state, view, flags, /*check_for_block=*/false, /* cacheSigStore= */ true, /* cacheFullScriptStore= */ true, txdata, validation_cache);
172+    auto check_state = CheckInputScripts(tx, view, flags, /* cacheSigStore= */ true, /* cacheFullScriptStore= */ true, txdata, validation_cache);
173+    if (!check_state.IsValid()) {
174+        return state.Invalid(check_state.GetResult(), strprintf("mempool-script-verify-flag-failed (%s)", check_state.GetRejectReason()), check_state.GetDebugMessage());
175+    }
176+    return true;
177 }
178
179 namespace {
180@@ -1253,13 +1257,15 @@ bool MemPoolAccept::PolicyScriptChecks(const ATMPArgs& args, Workspace& ws)
181
182     // Check input scripts and signatures.
183     // This is done last to help prevent CPU exhaustion denial-of-service attacks.
184-    if (!CheckInputScripts(tx, state, m_view, scriptVerifyFlags, /*check_for_block=*/false, true, false, ws.m_precomputed_txdata, GetValidationCache())) {
185+    TxValidationState check_state = CheckInputScripts(tx, m_view, scriptVerifyFlags, true, false, ws.m_precomputed_txdata, GetValidationCache());
186+    if (!check_state.IsValid()) {
187+        state.Invalid(check_state.GetResult(), strprintf("mempool-script-verify-flag-failed (%s)", check_state.GetRejectReason()), check_state.GetDebugMessage());
188         // Detect a failure due to a missing witness so that p2p code can handle rejection caching appropriately.
189         if (!tx.HasWitness() && SpendsNonAnchorWitnessProg(tx, m_view)) {
190             state.Invalid(TxValidationResult::TX_WITNESS_STRIPPED,
191                     state.GetRejectReason(), state.GetDebugMessage());
192         }
193-        return false; // state filled in by CheckInputScripts
194+        return false;
195     }
196
197     return true;
198@@ -2139,18 +2145,15 @@ ValidationCache::ValidationCache(const size_t script_execution_cache_bytes, cons
199  *
200  * Non-static (and redeclared) in src/test/txvalidationcache_tests.cpp
201  */
202-bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
203-                       const CCoinsViewCache& inputs, script_verify_flags flags, bool check_for_block,
204+TxValidationState CheckInputScripts(const CTransaction& tx,
205+                       const CCoinsViewCache& inputs, script_verify_flags flags,
206                        bool cacheSigStore,
207                        bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
208                        ValidationCache& validation_cache,
209                        std::vector<CScriptCheck>* pvChecks)
210 {
211-    if (tx.IsCoinBase()) return true;
212-
213-    if (flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) {
214-        assert(!check_for_block);
215-    }
216+    TxValidationState state;
217+    if (tx.IsCoinBase()) return state;
218
219     if (pvChecks) {
220         pvChecks->reserve(tx.vin.size());
221@@ -2166,7 +2169,7 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
222     hasher.Write(UCharCast(tx.GetWitnessHash().begin()), 32).Write((unsigned char*)&flags, sizeof(flags)).Finalize(hashCacheEntry.begin());
223     AssertLockHeld(cs_main); //TODO: Remove this requirement by making CuckooCache not require external locks
224     if (validation_cache.m_script_execution_cache.contains(hashCacheEntry, !cacheFullScriptStore)) {
225-        return true;
226+        return state;
227     }
228
229     if (!txdata.m_spent_outputs_ready) {
230@@ -2202,11 +2205,9 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
231             // non-standard DER encodings or non-null dummy
232             // arguments) or due to new consensus rules introduced in
233             // soft forks.
234-            if (!check_for_block) {
235-                return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("mempool-script-verify-flag-failed (%s)", ScriptErrorString(result->first)), result->second);
236-            } else {
237-                return state.Invalid(TxValidationResult::TX_CONSENSUS, strprintf("block-script-verify-flag-failed (%s)", ScriptErrorString(result->first)), result->second);
238-            }
239+            auto validation_result = flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS ? TxValidationResult::TX_NOT_STANDARD : TxValidationResult::TX_CONSENSUS;
240+            state.Invalid(validation_result, ScriptErrorString(result->first), result->second);
241+            return state;
242         }
243     }
244
245@@ -2216,7 +2217,7 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
246         validation_cache.m_script_execution_cache.insert(hashCacheEntry);
247     }
248
249-    return true;
250+    return state;
251 }
252
253 bool FatalError(Notifications& notifications, BlockValidationState& state, const bilingual_str& message)
254@@ -2663,21 +2664,20 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
255         if (!tx.IsCoinBase() && fScriptChecks)
256         {
257             bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */
258-            bool tx_ok;
259             TxValidationState tx_state;
260             // If CheckInputScripts is called with a pointer to a checks vector, the resulting checks are appended to it. In that case
261             // they need to be added to control which runs them asynchronously. Otherwise, CheckInputScripts runs the checks before returning.
262             if (control) {
263                 std::vector<CScriptCheck> vChecks;
264-                tx_ok = CheckInputScripts(tx, tx_state, view, flags, /*check_for_block=*/true, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache, &vChecks);
265-                if (tx_ok) control->Add(std::move(vChecks));
266+                tx_state = CheckInputScripts(tx, view, flags, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache, &vChecks);
267+                if (tx_state.IsValid()) control->Add(std::move(vChecks));
268             } else {
269-                tx_ok = CheckInputScripts(tx, tx_state, view, flags, /*check_for_block=*/true, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache);
270+                tx_state = CheckInputScripts(tx, view, flags, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache);
271             }
272-            if (!tx_ok) {
273+            if (!tx_state.IsValid()) {
274                 // Any transaction validation failure in ConnectBlock is a block consensus failure
275                 state.Invalid(BlockValidationResult::BLOCK_CONSENSUS,
276-                              tx_state.GetRejectReason(), tx_state.GetDebugMessage());
277+                              strprintf("block-script-verify-flag-failed (%s)", tx_state.GetRejectReason()), tx_state.GetDebugMessage());
278                 break;
279             }
280         }

ajtowns commented at 5:34 am on March 30, 2026: contributor

0+            auto validation_result = flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS ? TxValidationResult::TX_NOT_STANDARD : TxValidationResult::TX_CONSENSUS;

This form of automated check isn’t compatible with soft-forks: we don’t update MANDATORY_SCRIPT_VERIFY_FLAGS until a soft-fork has been active for some time, but do update STANDARD_SCRIPT_VERIFY_FLAGS so we don’t have invalid txs per the soft-fork included in the mempool. If/when the soft-fork does activate it will be included in the block flags, and at that point all block checks will be incorrectly reported as standardness failures rather consensus failures. Putting all potential soft forks into the MANDATORY_SCRIPT_VERIFY_FLAGS would be an option, but that would make this feature less useful – you couldn’t use it to opt-out of rules introduced in supported-but-not-yet-activated soft-forks.

I think it’s a lot more-straightforward and less fragile for the caller to indicate whether they’re trying to check for standardness or consensus; that’s at least easily reviewed at the call-site.

ajtowns commented at 5:40 am on March 30, 2026: contributor

I think after this PR, in non-block input script validation path, the result enum after a consensus input script failure is TX_NOT_STANDARD, which is incorrect; it should be TX_CONSENSUS.

We’ll already return TX_NOT_STANDARD for some consensus invalid txs (when we find a policy violation prior to finding the consensus violation), and since #33050 won’t rerun the checks to conclusively decide whether it’s violating consensus or just policy. So I don’t think this is a meaningful change, though perhaps it would help to change the description in consensus/validation.h or similar?

ismaelsadeeq commented at 2:17 pm on March 30, 2026: member

This form of automated check isn’t compatible with soft-forks: we don’t update MANDATORY_SCRIPT_VERIFY_FLAGS until a soft-fork has been active for some time, but do update STANDARD_SCRIPT_VERIFY_FLAGS so we don’t have invalid txs per the soft-fork included in the mempool. If/when the soft-fork does activate it will be included in the block flags, and at that point all block checks will be incorrectly reported as standardness failures rather consensus failures.

This is a good point, and I have not seen this before. Thanks for pointing it out.

Putting all potential soft forks into the MANDATORY_SCRIPT_VERIFY_FLAGS would be an option, but that would make this feature less useful – you couldn’t use it to opt-out of rules introduced in supported-but-not-yet-activated soft-forks.

I won’t suggest this either; it seems like a hack to me.

I think it’s a lot more-straightforward and less fragile for the caller to indicate whether they’re trying to check for standardness or consensus; that’s at least easily reviewed at the call-site.

Hmm. I would prefer to not have to review that check_for_block is set correctly at every call site at all. My question is: why does CheckInputScripts need to set the validation state? It doesn’t seem necessary to me.

I attempted to simply forward the raw ScriptError to callers and let them construct the state themselves. This approach fixes the underlying issue, I think? Block validation failures are always consensus failures, so we should unconditionally set that. The mempool validation path should owns the TX_NOT_STANDARD vs TX_CONSENSUS decision where it actually belongs, and we no longer need to document the behavior where CheckInputScripts can return TX_NOT_STANDARD for what is actually a consensus failure and block validation needs to update that.

Wdyt?

  0
  1diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp
  2index c8cfbadfa9..7ccc8ee661 100644
  3--- a/src/test/txvalidationcache_tests.cpp
  4+++ b/src/test/txvalidationcache_tests.cpp
  5@@ -20,10 +20,9 @@ struct Dersig100Setup : public TestChain100Setup {
  6         : TestChain100Setup{ChainType::REGTEST, {.extra_args = {"-testactivationheight=dersig@102"}}} {}
  7 };
  8
  9-bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
 10-                       const CCoinsViewCache& inputs, script_verify_flags flags, bool check_for_block,
 11-                       bool cacheSigStore,
 12-                       bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
 13+std::optional<ScriptCheckError> CheckInputScripts(const CTransaction& tx,
 14+                       const CCoinsViewCache& inputs, script_verify_flags flags,
 15+                       bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
 16                        ValidationCache& validation_cache,
 17                        std::vector<CScriptCheck>* pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
 18
 19@@ -128,8 +127,6 @@ static void ValidateCheckInputsForAllFlags(const CTransaction &tx, script_verify
 20     FastRandomContext insecure_rand(true);
 21
 22     for (int count = 0; count < 10000; ++count) {
 23-        TxValidationState state;
 24-
 25         // Randomly selects flag combinations
 26         script_verify_flags test_flags = script_verify_flags::from_int(insecure_rand.randrange(MAX_SCRIPT_VERIFY_FLAGS));
 27
 28@@ -143,23 +140,26 @@ static void ValidateCheckInputsForAllFlags(const CTransaction &tx, script_verify
 29             // WITNESS requires P2SH
 30             test_flags |= SCRIPT_VERIFY_P2SH;
 31         }
 32-        bool ret = CheckInputScripts(tx, state, &active_coins_tip, test_flags, /*check_for_block=*/false, true, add_to_cache, txdata, validation_cache, nullp
 33+        auto ret = CheckInputScripts(tx, &active_coins_tip, test_flags, true, add_to_cache, txdata, validation_cache, nullptr);
 34         // CheckInputScripts should succeed iff test_flags doesn't intersect with
 35         // failing_flags
 36         bool expected_return_value = !(test_flags & failing_flags);
 37-        BOOST_CHECK_EQUAL(ret, expected_return_value);
 38-
 39+        BOOST_CHECK_EQUAL(!ret.has_value(), expected_return_value);
 40+        if (ret.has_value()) {
 41+            BOOST_CHECK(ret->error != SCRIPT_ERR_OK);
 42+            BOOST_CHECK(!ret->debug_message.empty());
 43+        }
 44         // Test the caching
 45-        if (ret && add_to_cache) {
 46+        if (!ret.has_value() && add_to_cache) {
 47             // Check that we get a cache hit if the tx was valid
 48             std::vector<CScriptCheck> scriptchecks;
 49-            BOOST_CHECK(CheckInputScripts(tx, state, &active_coins_tip, test_flags, /*check_for_block=*/false, true, add_to_cache, txdata, validation_cache,
 50+            BOOST_CHECK(!CheckInputScripts(tx, &active_coins_tip, test_flags, true, add_to_cache, txdata, validation_cache, &scriptchecks).has_value());
 51             BOOST_CHECK(scriptchecks.empty());
 52         } else {
 53             // Check that we get script executions to check, if the transaction
 54             // was invalid, or we didn't add to cache.
 55             std::vector<CScriptCheck> scriptchecks;
 56-            BOOST_CHECK(CheckInputScripts(tx, state, &active_coins_tip, test_flags, /*check_for_block=*/false, true, add_to_cache, txdata, validation_cache,
 57+            BOOST_CHECK(!CheckInputScripts(tx, &active_coins_tip, test_flags, true, add_to_cache, txdata, validation_cache, &scriptchecks).has_value());
 58             BOOST_CHECK_EQUAL(scriptchecks.size(), tx.vin.size());
 59         }
 60     }
 61@@ -214,16 +214,18 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup)
 62     {
 63         LOCK(cs_main);
 64
 65-        TxValidationState state;
 66         PrecomputedTransactionData ptd_spend_tx;
 67-
 68-        BOOST_CHECK(!CheckInputScripts(CTransaction(spend_tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DER
 69+        auto ret = CheckInputScripts(CTransaction(spend_tx), &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true
 70+        BOOST_CHECK(ret.has_value());
 71+        BOOST_CHECK(ret->error == SCRIPT_ERR_SIG_DER);
 72+        BOOST_CHECK_EQUAL(ret->debug_message, strprintf("input 0 of %s (wtxid %s), spending %s:0", spend_tx.GetHash().ToString(), CTransaction(spend_tx).GetW
 73
 74         // If we call again asking for scriptchecks (as happens in
 75         // ConnectBlock), we should add a script check object for this -- we're
 76         // not caching invalidity (if that changes, delete this test case).
 77         std::vector<CScriptCheck> scriptchecks;
 78-        BOOST_CHECK(CheckInputScripts(CTransaction(spend_tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERS
 79+        auto non_cached_ret{CheckInputScripts(CTransaction(spend_tx), &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DER
 80+        BOOST_CHECK(!non_cached_ret.has_value());
 81         BOOST_CHECK_EQUAL(scriptchecks.size(), 1U);
 82
 83         // Test that CheckInputScripts returns true iff DERSIG-enforcing flags are
 84@@ -283,9 +285,8 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup)
 85
 86         // Make it valid, and check again
 87         invalid_with_cltv_tx.vin[0].scriptSig = CScript() << vchSig << 100;
 88-        TxValidationState state;
 89         PrecomputedTransactionData txdata;
 90-        BOOST_CHECK(CheckInputScripts(CTransaction(invalid_with_cltv_tx), state, m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKLOCKTIMEV
 91+        BOOST_CHECK(!CheckInputScripts(CTransaction(invalid_with_cltv_tx), m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY,
 92     }
 93
 94     // TEST CHECKSEQUENCEVERIFY
 95@@ -311,9 +312,8 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup)
 96
 97         // Make it valid, and check again
 98         invalid_with_csv_tx.vin[0].scriptSig = CScript() << vchSig << 100;
 99-        TxValidationState state;
100         PrecomputedTransactionData txdata;
101-        BOOST_CHECK(CheckInputScripts(CTransaction(invalid_with_csv_tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKSEQUENCEV
102+        BOOST_CHECK(!CheckInputScripts(CTransaction(invalid_with_csv_tx), &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKSEQUENCEVERIFY,
103     }
104
105     // TODO: add tests for remaining script flags
106@@ -372,15 +372,17 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup)
107         // Invalidate vin[1]
108         tx.vin[1].scriptWitness.SetNull();
109
110-        TxValidationState state;
111         PrecomputedTransactionData txdata;
112         // This transaction is now invalid under segwit, because of the second input.
113-        BOOST_CHECK(!CheckInputScripts(CTransaction(tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS,
114+        auto ret = CheckInputScripts(CTransaction(tx), &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, tru
115+        BOOST_CHECK(ret.has_value());
116+        BOOST_CHECK(ret->error == SCRIPT_ERR_WITNESS_PROGRAM_MISMATCH);
117+        BOOST_CHECK_EQUAL(ret->debug_message, strprintf("input 1 of %s (wtxid %s), spending %s:1", tx.GetHash().ToString(), CTransaction(tx).GetWitnessHash()
118
119         std::vector<CScriptCheck> scriptchecks;
120         // Make sure this transaction was not cached (ie because the first
121         // input was valid)
122-        BOOST_CHECK(CheckInputScripts(CTransaction(tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, /
123+        BOOST_CHECK(!CheckInputScripts(CTransaction(tx), &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, t
124         // Should get 2 script checks back -- caching is on a whole-transaction basis.
125         BOOST_CHECK_EQUAL(scriptchecks.size(), 2U);
126     }
127diff --git a/src/validation.cpp b/src/validation.cpp
128index 5d95edf4db..005702d09b 100644
129--- a/src/validation.cpp
130+++ b/src/validation.cpp
131@@ -138,8 +138,8 @@ const CBlockIndex* Chainstate::FindForkInGlobalIndex(const CBlockLocator& locato
132     return m_chain.Genesis();
133 }
134
135-bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
136-                       const CCoinsViewCache& inputs, script_verify_flags flags, bool check_for_block,
137+std::optional<ScriptCheckError> CheckInputScripts(const CTransaction& tx,
138+                       const CCoinsViewCache& inputs, script_verify_flags flags,
139                        bool cacheSigStore,
140                        bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
141                        ValidationCache& validation_cache,
142@@ -429,7 +429,12 @@ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, TxValidationS
143     }
144
145     // Call CheckInputScripts() to cache signature and script validity against current tip consensus rules.
146-    return CheckInputScripts(tx, state, view, flags, /*check_for_block=*/false, /* cacheSigStore= */ true, /* cacheFullScriptStore= */ true, txdata, validati
147+    if (auto err = CheckInputScripts(tx, view, flags, /* cacheSigStore= */ true, /* cacheFullScriptStore= */ true, txdata, validation_cache)) {
148+        auto validation_result = flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS ? TxValidationResult::TX_NOT_STANDARD : TxValidationResult::TX_CONSENSUS;
149+        state.Invalid(validation_result, strprintf("mempool-script-verify-flag-failed (%s)", ScriptErrorString(err->error)), err->debug_message);
150+        return false;
151+    }
152+    return true;
153 }
154
155 namespace {
156@@ -1253,13 +1258,14 @@ bool MemPoolAccept::PolicyScriptChecks(const ATMPArgs& args, Workspace& ws)
157
158     // Check input scripts and signatures.
159     // This is done last to help prevent CPU exhaustion denial-of-service attacks.
160-    if (!CheckInputScripts(tx, state, m_view, scriptVerifyFlags, /*check_for_block=*/false, true, false, ws.m_precomputed_txdata, GetValidationCache())) {
161+    if (auto err = CheckInputScripts(tx, m_view, scriptVerifyFlags, true, false, ws.m_precomputed_txdata, GetValidationCache())) {
162+        auto validation_result = scriptVerifyFlags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS ? TxValidationResult::TX_NOT_STANDARD : TxValidationResult::TX_CONSE
163         // Detect a failure due to a missing witness so that p2p code can handle rejection caching appropriately.
164         if (!tx.HasWitness() && SpendsNonAnchorWitnessProg(tx, m_view)) {
165-            state.Invalid(TxValidationResult::TX_WITNESS_STRIPPED,
166-                    state.GetRejectReason(), state.GetDebugMessage());
167+            validation_result = TxValidationResult::TX_WITNESS_STRIPPED;
168         }
169-        return false; // state filled in by CheckInputScripts
170+        state.Invalid(validation_result, strprintf("mempool-script-verify-flag-failed (%s)", ScriptErrorString(err->error)),  err->debug_message);
171+        return false;
172     }
173
174     return true;
175@@ -2134,23 +2140,17 @@ ValidationCache::ValidationCache(const size_t script_execution_cache_bytes, cons
176  * which are matched. This is useful for checking blocks where we will likely never need the cache
177  * entry again.
178  *
179- * Note that we may set state.reason to NOT_STANDARD for extra soft-fork flags in flags, block-checking
180- * callers should probably reset it to CONSENSUS in such cases.
181  *
182  * Non-static (and redeclared) in src/test/txvalidationcache_tests.cpp
183  */
184-bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
185-                       const CCoinsViewCache& inputs, script_verify_flags flags, bool check_for_block,
186+std::optional<ScriptCheckError> CheckInputScripts(const CTransaction& tx,
187+                       const CCoinsViewCache& inputs, script_verify_flags flags,
188                        bool cacheSigStore,
189                        bool cacheFullScriptStore, PrecomputedTransactionData& txdata,
190                        ValidationCache& validation_cache,
191                        std::vector<CScriptCheck>* pvChecks)
192 {
193-    if (tx.IsCoinBase()) return true;
194-
195-    if (flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) {
196-        assert(!check_for_block);
197-    }
198+    if (tx.IsCoinBase()) return std::nullopt;
199
200     if (pvChecks) {
201         pvChecks->reserve(tx.vin.size());
202@@ -2166,7 +2166,7 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
203     hasher.Write(UCharCast(tx.GetWitnessHash().begin()), 32).Write((unsigned char*)&flags, sizeof(flags)).Finalize(hashCacheEntry.begin());
204     AssertLockHeld(cs_main); //TODO: Remove this requirement by making CuckooCache not require external locks
205     if (validation_cache.m_script_execution_cache.contains(hashCacheEntry, !cacheFullScriptStore)) {
206-        return true;
207+        return std::nullopt;
208     }
209
210     if (!txdata.m_spent_outputs_ready) {
211@@ -2196,17 +2196,7 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
212         if (pvChecks) {
213             pvChecks->emplace_back(std::move(check));
214         } else if (auto result = check(); result.has_value()) {
215-            // Tx failures never trigger disconnections/bans.
216-            // This is so that network splits aren't triggered
217-            // either due to non-consensus relay policies (such as
218-            // non-standard DER encodings or non-null dummy
219-            // arguments) or due to new consensus rules introduced in
220-            // soft forks.
221-            if (!check_for_block) {
222-                return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("mempool-script-verify-flag-failed (%s)", ScriptErrorString(result->first)), result->second);
223-            } else {
224-                return state.Invalid(TxValidationResult::TX_CONSENSUS, strprintf("block-script-verify-flag-failed (%s)", ScriptErrorString(result->first)), result->second);
225-            }
226+            return ScriptCheckError{result->first, result->second};
227         }
228     }
229
230@@ -2216,7 +2206,7 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
231         validation_cache.m_script_execution_cache.insert(hashCacheEntry);
232     }
233
234-    return true;
235+    return std::nullopt;
236 }
237
238 bool FatalError(Notifications& notifications, BlockValidationState& state, const bilingual_str& message)
239@@ -2663,21 +2653,20 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state,
240         if (!tx.IsCoinBase() && fScriptChecks)
241         {
242             bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */
243-            bool tx_ok;
244-            TxValidationState tx_state;
245+            std::optional<ScriptCheckError> script_check_err;
246             // If CheckInputScripts is called with a pointer to a checks vector, the resulting checks are appended to it. In that case
247             // they need to be added to control which runs them asynchronously. Otherwise, CheckInputScripts runs the checks before returning.
248             if (control) {
249                 std::vector<CScriptCheck> vChecks;
250-                tx_ok = CheckInputScripts(tx, tx_state, view, flags, /*check_for_block=*/true, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache, &vChecks);
251-                if (tx_ok) control->Add(std::move(vChecks));
252+                script_check_err = CheckInputScripts(tx, view, flags, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache, &vChecks);
253+                if (!script_check_err) control->Add(std::move(vChecks));
254             } else {
255-                tx_ok = CheckInputScripts(tx, tx_state, view, flags, /*check_for_block=*/true, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache);
256+                script_check_err = CheckInputScripts(tx, view, flags, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache);
257             }
258-            if (!tx_ok) {
259+            if (script_check_err) {
260                 // Any transaction validation failure in ConnectBlock is a block consensus failure
261                 state.Invalid(BlockValidationResult::BLOCK_CONSENSUS,
262-                              tx_state.GetRejectReason(), tx_state.GetDebugMessage());
263+                              strprintf("block-script-verify-flag-failed (%s)", ScriptErrorString(script_check_err->error)), script_check_err->debug_message);
264                 break;
265             }
266         }
267diff --git a/src/validation.h b/src/validation.h
268index cd448f3ca9..c34ae41159 100644
269--- a/src/validation.h
270+++ b/src/validation.h
271@@ -331,6 +331,11 @@ bool CheckSequenceLocksAtTip(CBlockIndex* tip,
272  * Closure representing one script verification
273  * Note that this stores references to the spending transaction
274  */
275+struct ScriptCheckError {
276+    ScriptError error;
277+    std::string debug_message;
278+};
279+
280 class CScriptCheck
281 {
282 private:
283

ismaelsadeeq commented at 2:21 pm on March 30, 2026: member

We’ll already return TX_NOT_STANDARD for some consensus invalid txs (when we find a policy violation prior to finding the consensus violation), and since #33050 won’t rerun the checks to conclusively decide whether it’s violating consensus or just policy.

Yeah, fair point, but worth noting that in that case, it makes sense we hit the policy first, but in this case, there is no policy violation, it is a consensus violation that is collapsed to policy.

So I don’t think this is a meaningful change, though perhaps it would help to change the description in consensus/validation.h or similar?

Yeah, worth making it explicit.

ismaelsadeeq commented at 2:22 pm on March 30, 2026: member

I noticed stale comment in MANDATORY_SCRIPT_VERIFY_FLAGS since #33050 , fixed in https://github.com/bitcoin/bitcoin/pull/34957

ajtowns commented at 9:22 am on April 2, 2026: contributor

So CheckInputScripts should just return a validation state indicating whether consensus or standardness is violated; callers should add the detail of whether it is block validation or mempool acceptance validation that triggered the failure.

That means you need to validate each caller correctly adds either TX_NOT_STANDARD and mempool-script-verify-flag-failed or TX_CONSENSUS and block-script-verify-flag-failed, rather than correct passing a boolean? That seems more work to review and more prone to error?

ismaelsadeeq commented at 2:35 am on April 4, 2026: member

That means you need to validate each caller correctly adds either TX_NOT_STANDARD and mempool-script-verify-flag-failed or TX_CONSENSUS and block-script-verify-flag-failed, rather than correct passing a boolean? That seems more work to review and more prone to error?

For the tx validation paths, the setting of the error message can be abstracted away to maintain consistency of the error string. Indeed, it adds some work for callers now to correctly map the result, but this is preferable to relying on a boolean, because it separates the logic of checking input scripts from that decision and makes testing and reasoning about the subroutine much more straightforward.

I’m curious to hear other reviewers’ thoughts, though, not rigid on this at all. Just bringing up ideas here.

in test/functional/p2p_segwit.py:1398 in 4ab49aa3ac outdated

1394@@ -1395,10 +1395,10 @@ def test_segwit_versions(self):
1395         # even with the node that accepts non-standard txs.
1396         test_transaction_acceptance(self.nodes[0], self.test_node, tx3, with_witness=True, accepted=False, reason="reserved for soft-fork upgrades")
1397 
1398-        # Building a block with the transaction must be valid, however.
1399+        # Building a block with the transaction must be valid, however even without -acceptnonstdtxn.

instagibbs commented at 5:08 pm on April 6, 2026:

0        # Building a block with the transaction must be valid, however, even without -acceptnonstdtxn.

in test/functional/p2p_invalid_tx.py:83 in 4a93f6e36c outdated

79@@ -75,6 +80,11 @@ def run_test(self):
80                 [tx], node, success=False,
81                 reject_reason=template.reject_reason,
82             )
83+            std_reject = template.std_reject_reason or template.reject_reason

instagibbs commented at 5:20 pm on April 6, 2026:

this or similar?

0            std_reject = template.std_reject_reason or template.reject_reason
1            assert std_reject is not None

instagibbs commented at 8:31 pm on April 6, 2026: member

One suggestion that limits the scope a bit: just don’t change the CheckInputScripts args, so the report per-tx message turns into “block-*” if it’s a block validation related script failure when standardness knob is off?

Is that too confusing/ugly or simply Broken in some way?

tests: in p2p_segwit, check non-mandatory errors with -acceptnonstdtxn=0 node

Prepare for updating -acceptnonstdtxn to allow txns that violate
STANDARD_SCRIPT_VERIFY_FLAGS but not MANDATORY_SCRIPT_VERIFY_FLAGS by
checking the non-mandatory flags with node that enforces standardness.

4c35d39f13

tests: test tx rejection for both -acceptnonstdtxn=0 and 1 aaa909d9c2

validation: Be explicit about whether CheckInputScripts is for block/mempool f876cffa9c

validation: Check only MANDATORY_SCRIPT_VERIFY_FLAGS when -acceptnonstdtxn is set dfe4cc6cc2

ajtowns force-pushed on Apr 7, 2026

ajtowns commented at 12:54 pm on April 7, 2026: contributor

For the tx validation paths, the setting of the error message can be abstracted away to maintain consistency of the error string.

That’s exactly what passing the bool is doing though?

One suggestion that limits the scope a bit: just don’t change the CheckInputScripts args, so the report per-tx message turns into “block-*” if it’s a block validation related script failure when standardness knob is off?

Having mempool errors become “block” errors depending on a config settings seems a bit confusing, so that doesn’t really seem preferable, unless it’s much easier. And making the change (check_for_block = !(flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS); at the top of CheckInputScripts()) interferes with tests that use acceptnonstdtxn for specific reasons (eg feature_cltv, feature_segwit, p2p_segwit). I think I looked at changing those previously, but it didn’t seem much easier, and also seemed more confusing to have mempool actions be called “block-” instead of “mempool-”.

The “validation: Be explicit about whether CheckInputScripts is for block/mempool” commit seems pretty easy to review with git show HEAD^ --word-diff to me, fwiw? I’m not really seeing the problem?

Also apparently this picked up a silent merge conflict via #31823 by the looks. Rebased and fixed.

instagibbs commented at 1:46 pm on April 7, 2026: member

I’m not really seeing the problem?

I was simply suggesting less code if the side effects were acceptable. I just don’t see a block level error being reported as one as particularly odd. If others disagree, then you can ignore the suggestion.

ajtowns commented at 3:43 pm on April 7, 2026: contributor

I was simply suggesting less code if the side effects were acceptable. I just don’t see a block level error being reported as one as particularly odd. If others disagree, then you can ignore the suggestion.

It’s not a block level error, though: it’s a tx level error? (We’re submitting a tx to the mempool that’s consensus invalid)

We previously differentiated consensus-level errors vs policy errors; we’re currently differentiating errors during block-validation (always consensus-level) vs errors during mempool-validation (currently consensus plus policy checks, but changed by this PR to be, depending on the acceptnonstdtxn setting, either consensus plus policy checks, or the hardcoded set of consensus checks we expect to be enforced at tip but which might be different from what’s actually being enforced at the node’s current tip). We could not differentiate at all, and just always report “script-verify-flag-failed” (with no “block-” or “mempool-” prefix), but that would be a bigger code change (though maybe could be a scripted-diff?), as lots of tests would need updating.

instagibbs commented at 3:55 pm on April 7, 2026: member

hardcoded set of consensus checks

Ok this difference vs actual chaintip is persuasive against my idea

in src/validation.cpp:431 in f876cffa9c

427@@ -427,7 +428,7 @@ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, TxValidationS
428     }
429 
430     // Call CheckInputScripts() to cache signature and script validity against current tip consensus rules.
431-    return CheckInputScripts(tx, state, view, flags, /* cacheSigStore= */ true, /* cacheFullScriptStore= */ true, txdata, validation_cache);
432+    return CheckInputScripts(tx, state, view, flags, /*check_for_block=*/false, /* cacheSigStore= */ true, /* cacheFullScriptStore= */ true, txdata, validation_cache);

instagibbs commented at 3:13 pm on April 8, 2026:

The log message in ConsensusScriptChecks on failure is a little off after this patch. Maybe

“BUG! PLEASE REPORT THIS! CheckInputScripts failed against latest-block but not mempool flag

or something? not a huge deal for a never should happen log

in src/validation.cpp:2262 in dfe4cc6cc2

2256@@ -2257,6 +2257,10 @@ script_verify_flags GetBlockScriptFlags(const CBlockIndex& block_index, const Ch
2257 {
2258     const Consensus::Params& consensusparams = chainman.GetConsensus();
2259 
2260+    // Note that any flags returned from this function (ie, specified
2261+    // here or in script_flag_exceptions) must also be included in
2262+    // MANDATORY_SCRIPT_VERIFY_FLAGS in policy/policy.h

instagibbs commented at 3:28 pm on April 8, 2026:

dfe4cc6cc2003b077f95e30e50c47f1fa28fb2e2

can we assert this property later in the function instead of just a comment?

in src/validation.cpp:2072 in f876cffa9c

2068                        std::vector<CScriptCheck>* pvChecks)
2069 {
2070     if (tx.IsCoinBase()) return true;
2071 
2072+    if (flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) {
2073+        assert(!check_for_block);

instagibbs commented at 3:33 pm on April 8, 2026:

f876cffa9c03151746a7fd03be76ec00e8b4a223

just noting crashing here in release build being the correct thing since it would be a block check with erroneous flags

instagibbs commented at 3:35 pm on April 8, 2026: member

logic looks correct, some suggestions dfe4cc6cc2003b077f95e30e50c47f1fa28fb2e2

instagibbs commented at 3:44 pm on April 8, 2026: member

If you enable this feature as a mining node, you have a small risk of ending up with invalid txs remaining in your mempool if a non-standard tx is accepted in your mempool and remains in there during a soft-fork activation that renders the tx invalid.

To check: Risk seems similar to what already can happen on testnet if any of the other standardness checks fail? Just now with script interpreter things?

ajtowns commented at 6:54 pm on April 9, 2026: contributor

If you enable this feature as a mining node, you have a small risk of ending up with invalid txs remaining in your mempool if a non-standard tx is accepted in your mempool and remains in there during a soft-fork activation that renders the tx invalid.

To check: Risk seems similar to what already can happen on testnet if any of the other standardness checks fail? Just now with script interpreter things?

Hmm, I think I’ve been contradictory here. On the one hand:

This form of automated check isn’t compatible with soft-forks: we don’t update MANDATORY_SCRIPT_VERIFY_FLAGS until a soft-fork has been active for some time, but do update STANDARD_SCRIPT_VERIFY_FLAGS so we don’t have invalid txs per the soft-fork included in the mempool. If/when the soft-fork does activate it will be included in the block flags, and at that point all block checks will be incorrectly reported as standardness failures rather consensus failures. Putting all potential soft forks into the MANDATORY_SCRIPT_VERIFY_FLAGS would be an option, but that would make this feature less useful – you couldn’t use it to opt-out of rules introduced in supported-but-not-yet-activated soft-forks.

That implies GetBlockScriptFlags can and should return values that aren’t inside MANDATORY_SCRIPT_VERIFY_FLAGS, but

0    if (flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) {
1        assert(!check_for_block);
2    }

implies that it should not (well, unless they’re flags that aren’t checked for mempool acceptance even with acceptnonstdtxn=0). That doesn’t make any difference today, but would make a difference with inquisition builds (where currently softfork flags are added to STANDARD_SCRIPT_VERIFY_FLAGS and GetBlockScriptFlags).

So I think that means the std-not-mandatory check-and-assert above is better removed from this PR.

Anyway:

To check: Risk seems similar to what already can happen on testnet if any of the other standardness checks fail? Just now with script interpreter things?

Similar, but not the same?

eg, with core v31, bip 54 is enforced as a standardness rule, and I believe you can disable it with -acceptnonstdtxn=1, so provided you can violate it without violating bad-txns-too-many-sigops, I think you can add a bip 54 violating tx to your mempool and mine it later.

However, if we setup bip 54 for consensus activation the same way as it’s implemented for inquisition then we’ll always enforce it in the mempool, and it won’t be possible to get a violation into the mempool even prior to activation.

instagibbs commented at 7:07 pm on April 9, 2026: member

eg, with core v31, bip 54 is enforced as a standardness rule, and I believe you can disable it with -acceptnonstdtxn=1, so provided you can violate it without violating bad-txns-too-many-sigops, I think you can add a bip 54 violating tx to your mempool and mine it later.

Not sure I understand the qualatative difference between this and, say, an unspent dust output with standardness checks off. If we somehow softforked a rule about that, they’d be stuck either way? Just making sure I’m not missing a nuance.

policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only) #29843

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)