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.
policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only) #29843
pull ajtowns wants to merge 4 commits into bitcoin:master from ajtowns:202303-acceptnonstdscript changing 5 files +65 −35-
ajtowns commented at 9:08 AM on April 10, 2024: contributor
-
DrahtBot commented at 9:08 AM on April 10, 2024: contributor
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
Code Coverage & Benchmarks
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/29843.
<!--021abf342d371248e50ceaed478a90ca-->
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 <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Conflicts
Reviewers, this pull request conflicts with the following ones:
- #34875 (refactor: separate deferred script check collection from
CheckInputScriptsby l0rinc) - #32575 (consensus: 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.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
LLM Linter (✨ experimental)
Possible places where named args for integral literals may be used (e.g.
func(x, /*named_arg=*/0)in C++, andfunc(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)insrc/test/txvalidationcache_tests.cppCheckInputScripts(tx, state, &active_coins_tip, test_flags, /*check_for_block=*/false, true, add_to_cache, txdata, validation_cache, &scriptchecks)insrc/test/txvalidationcache_tests.cppCheckInputScripts(tx, state, &active_coins_tip, test_flags, /*check_for_block=*/false, true, add_to_cache, txdata, validation_cache, &scriptchecks)insrc/test/txvalidationcache_tests.cppCheckInputScripts(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)insrc/test/txvalidationcache_tests.cppCheckInputScripts(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)insrc/test/txvalidationcache_tests.cppCheckInputScripts(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)insrc/test/txvalidationcache_tests.cppCheckInputScripts(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)insrc/test/txvalidationcache_tests.cppCheckInputScripts(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)insrc/test/txvalidationcache_tests.cppCheckInputScripts(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)insrc/test/txvalidationcache_tests.cppCheckInputScripts(tx, state, m_view, scriptVerifyFlags, /*check_for_block=*/false, true, false, ws.m_precomputed_txdata, GetValidationCache())insrc/validation.cpptest_transaction_acceptance(self.nodes[1], self.std_node, tx2, True, False, 'mempool-script-verify-flag-failed (Using non-compressed keys in segwit)')intest/functional/p2p_segwit.pytest_transaction_acceptance(self.nodes[1], self.std_node, tx3, True, False, 'mempool-script-verify-flag-failed (Using non-compressed keys in segwit)')intest/functional/p2p_segwit.pytest_transaction_acceptance(self.nodes[1], self.std_node, tx4, True, False, 'mempool-script-verify-flag-failed (Using non-compressed keys in segwit)')intest/functional/p2p_segwit.py
<sup>2026-04-07 12:54:55</sup>
- #34875 (refactor: separate deferred script check collection from
- 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,
-acceptnonstdtxnenables 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
=2at 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 12:51 AM on May 15, 2024: contributor
<!--85328a0da195eb286784d51f73fa0af9-->
🚧 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.
<sub>Debug: https://github.com/bitcoin/bitcoin/runs/23651574970</sub>
- ajtowns force-pushed on May 16, 2024
-
- 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
CheckInputScriptsfunction; 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. SoCheckInputScriptsshould 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 beTX_CONSENSUS.
The diff below should fix this as well.
<details> <summary>diff</summary>
diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp index c8cfbadfa9..9edad1d020 100644 --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -20,8 +20,8 @@ struct Dersig100Setup : public TestChain100Setup { : TestChain100Setup{ChainType::REGTEST, {.extra_args = {"-testactivationheight=dersig@102"}}} {} }; -bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, - const CCoinsViewCache& inputs, script_verify_flags flags, bool check_for_block, +TxValidationState CheckInputScripts(const CTransaction& tx, + const CCoinsViewCache& inputs, script_verify_flags flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, ValidationCache& validation_cache, @@ -110,6 +110,21 @@ BOOST_FIXTURE_TEST_CASE(tx_mempool_block_doublespend, Dersig100Setup) BOOST_CHECK_EQUAL(m_node.mempool->size(), 0U); } +void ValidTxValidationState(const TxValidationState& ret) +{ + BOOST_CHECK_EQUAL(ret.GetResult(), TxValidationResult::TX_RESULT_UNSET); + BOOST_CHECK(ret.GetRejectReason().empty()); + BOOST_CHECK(ret.GetDebugMessage().empty()); +} + +void InvalidTxValidationState(const TxValidationState& ret, TxValidationResult result, const std::string& reject_reason, const std::string& debug_message) +{ + BOOST_CHECK(!ret.IsValid()); + BOOST_CHECK_EQUAL(ret.GetResult(), result); + BOOST_CHECK_EQUAL(ret.GetRejectReason(), reject_reason); + BOOST_CHECK_EQUAL(ret.GetDebugMessage(), debug_message); +} + // Run CheckInputScripts (using CoinsTip()) on the given transaction, for all script // flags. Test that CheckInputScripts passes for all flags that don't overlap with // the failing_flags argument, but otherwise fails. @@ -128,8 +143,6 @@ static void ValidateCheckInputsForAllFlags(const CTransaction &tx, script_verify FastRandomContext insecure_rand(true); for (int count = 0; count < 10000; ++count) { - TxValidationState state; - // Randomly selects flag combinations script_verify_flags test_flags = script_verify_flags::from_int(insecure_rand.randrange(MAX_SCRIPT_VERIFY_FLAGS)); @@ -143,23 +156,35 @@ static void ValidateCheckInputsForAllFlags(const CTransaction &tx, script_verify // WITNESS requires P2SH test_flags |= SCRIPT_VERIFY_P2SH; } - bool ret = CheckInputScripts(tx, state, &active_coins_tip, test_flags, /*check_for_block=*/false, true, add_to_cache, txdata, validation_cache, nullptr); + auto ret = CheckInputScripts(tx, &active_coins_tip, test_flags, true, add_to_cache, txdata, validation_cache, nullptr); // CheckInputScripts should succeed iff test_flags doesn't intersect with // failing_flags bool expected_return_value = !(test_flags & failing_flags); - BOOST_CHECK_EQUAL(ret, expected_return_value); + BOOST_CHECK_EQUAL(ret.IsValid(), expected_return_value); + if (!ret.IsValid()) { + if (test_flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) { + BOOST_CHECK_EQUAL(ret.GetResult(), TxValidationResult::TX_NOT_STANDARD); + } else { + BOOST_CHECK_EQUAL(ret.GetResult(), TxValidationResult::TX_CONSENSUS); + } + BOOST_CHECK(!ret.GetRejectReason().empty()); + BOOST_CHECK(!ret.GetDebugMessage().empty()); + } else { + ValidTxValidationState(ret); + } // Test the caching - if (ret && add_to_cache) { + if (ret.IsValid() && add_to_cache) { // Check that we get a cache hit if the tx was valid std::vector<CScriptCheck> scriptchecks; - BOOST_CHECK(CheckInputScripts(tx, state, &active_coins_tip, test_flags, /*check_for_block=*/false, true, add_to_cache, txdata, validation_cache, &scriptchecks)); + BOOST_CHECK(CheckInputScripts(tx, &active_coins_tip, test_flags, true, add_to_cache, txdata, validation_cache, &scriptchecks).IsValid()); BOOST_CHECK(scriptchecks.empty()); } else { // Check that we get script executions to check, if the transaction // was invalid, or we didn't add to cache. std::vector<CScriptCheck> scriptchecks; - BOOST_CHECK(CheckInputScripts(tx, state, &active_coins_tip, test_flags, /*check_for_block=*/false, true, add_to_cache, txdata, validation_cache, &scriptchecks)); + auto cached_ret{CheckInputScripts(tx, &active_coins_tip, test_flags, true, add_to_cache, txdata, validation_cache, &scriptchecks)}; + ValidTxValidationState(cached_ret); BOOST_CHECK_EQUAL(scriptchecks.size(), tx.vin.size()); } } @@ -214,16 +239,16 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup) { LOCK(cs_main); - TxValidationState state; PrecomputedTransactionData ptd_spend_tx; - - 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)); + 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); + 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())); // If we call again asking for scriptchecks (as happens in // ConnectBlock), we should add a script check object for this -- we're // not caching invalidity (if that changes, delete this test case). std::vector<CScriptCheck> scriptchecks; - 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)); + 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)}; + ValidTxValidationState(non_cached_ret); BOOST_CHECK_EQUAL(scriptchecks.size(), 1U); // Test that CheckInputScripts returns true iff DERSIG-enforcing flags are @@ -283,9 +308,9 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup) // Make it valid, and check again invalid_with_cltv_tx.vin[0].scriptSig = CScript() << vchSig << 100; - TxValidationState state; PrecomputedTransactionData txdata; - 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)); + 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)}; + ValidTxValidationState(valid_ret); } // TEST CHECKSEQUENCEVERIFY @@ -311,9 +336,9 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup) // Make it valid, and check again invalid_with_csv_tx.vin[0].scriptSig = CScript() << vchSig << 100; - TxValidationState state; PrecomputedTransactionData txdata; - 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)); + 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)}; + ValidTxValidationState(valid_ret); } // TODO: add tests for remaining script flags @@ -372,15 +397,16 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup) // Invalidate vin[1] tx.vin[1].scriptWitness.SetNull(); - TxValidationState state; PrecomputedTransactionData txdata; // This transaction is now invalid under segwit, because of the second input. - 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)); + 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); + 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())); std::vector<CScriptCheck> scriptchecks; // Make sure this transaction was not cached (ie because the first // input was valid) - 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)); + 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)}; + ValidTxValidationState(valid_ret); // Should get 2 script checks back -- caching is on a whole-transaction basis. BOOST_CHECK_EQUAL(scriptchecks.size(), 2U); } diff --git a/src/validation.cpp b/src/validation.cpp index 5d95edf4db..6c3644f9a5 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -138,8 +138,8 @@ const CBlockIndex* Chainstate::FindForkInGlobalIndex(const CBlockLocator& locato return m_chain.Genesis(); } -bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, - const CCoinsViewCache& inputs, script_verify_flags flags, bool check_for_block, +TxValidationState CheckInputScripts(const CTransaction& tx, + const CCoinsViewCache& inputs, script_verify_flags flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, ValidationCache& validation_cache, @@ -429,7 +429,11 @@ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, TxValidationS } // Call CheckInputScripts() to cache signature and script validity against current tip consensus rules. - return CheckInputScripts(tx, state, view, flags, /*check_for_block=*/false, /* cacheSigStore= */ true, /* cacheFullScriptStore= */ true, txdata, validation_cache); + auto check_state = CheckInputScripts(tx, view, flags, /* cacheSigStore= */ true, /* cacheFullScriptStore= */ true, txdata, validation_cache); + if (!check_state.IsValid()) { + return state.Invalid(check_state.GetResult(), strprintf("mempool-script-verify-flag-failed (%s)", check_state.GetRejectReason()), check_state.GetDebugMessage()); + } + return true; } namespace { @@ -1253,13 +1257,15 @@ bool MemPoolAccept::PolicyScriptChecks(const ATMPArgs& args, Workspace& ws) // Check input scripts and signatures. // This is done last to help prevent CPU exhaustion denial-of-service attacks. - if (!CheckInputScripts(tx, state, m_view, scriptVerifyFlags, /*check_for_block=*/false, true, false, ws.m_precomputed_txdata, GetValidationCache())) { + TxValidationState check_state = CheckInputScripts(tx, m_view, scriptVerifyFlags, true, false, ws.m_precomputed_txdata, GetValidationCache()); + if (!check_state.IsValid()) { + state.Invalid(check_state.GetResult(), strprintf("mempool-script-verify-flag-failed (%s)", check_state.GetRejectReason()), check_state.GetDebugMessage()); // Detect a failure due to a missing witness so that p2p code can handle rejection caching appropriately. if (!tx.HasWitness() && SpendsNonAnchorWitnessProg(tx, m_view)) { state.Invalid(TxValidationResult::TX_WITNESS_STRIPPED, state.GetRejectReason(), state.GetDebugMessage()); } - return false; // state filled in by CheckInputScripts + return false; } return true; @@ -2139,18 +2145,15 @@ ValidationCache::ValidationCache(const size_t script_execution_cache_bytes, cons * * Non-static (and redeclared) in src/test/txvalidationcache_tests.cpp */ -bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, - const CCoinsViewCache& inputs, script_verify_flags flags, bool check_for_block, +TxValidationState CheckInputScripts(const CTransaction& tx, + const CCoinsViewCache& inputs, script_verify_flags flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, ValidationCache& validation_cache, std::vector<CScriptCheck>* pvChecks) { - if (tx.IsCoinBase()) return true; - - if (flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) { - assert(!check_for_block); - } + TxValidationState state; + if (tx.IsCoinBase()) return state; if (pvChecks) { pvChecks->reserve(tx.vin.size()); @@ -2166,7 +2169,7 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, hasher.Write(UCharCast(tx.GetWitnessHash().begin()), 32).Write((unsigned char*)&flags, sizeof(flags)).Finalize(hashCacheEntry.begin()); AssertLockHeld(cs_main); //TODO: Remove this requirement by making CuckooCache not require external locks if (validation_cache.m_script_execution_cache.contains(hashCacheEntry, !cacheFullScriptStore)) { - return true; + return state; } if (!txdata.m_spent_outputs_ready) { @@ -2202,11 +2205,9 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, // non-standard DER encodings or non-null dummy // arguments) or due to new consensus rules introduced in // soft forks. - if (!check_for_block) { - return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("mempool-script-verify-flag-failed (%s)", ScriptErrorString(result->first)), result->second); - } else { - return state.Invalid(TxValidationResult::TX_CONSENSUS, strprintf("block-script-verify-flag-failed (%s)", ScriptErrorString(result->first)), result->second); - } + auto validation_result = flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS ? TxValidationResult::TX_NOT_STANDARD : TxValidationResult::TX_CONSENSUS; + state.Invalid(validation_result, ScriptErrorString(result->first), result->second); + return state; } } @@ -2216,7 +2217,7 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, validation_cache.m_script_execution_cache.insert(hashCacheEntry); } - return true; + return state; } bool FatalError(Notifications& notifications, BlockValidationState& state, const bilingual_str& message) @@ -2663,21 +2664,20 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, if (!tx.IsCoinBase() && fScriptChecks) { bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */ - bool tx_ok; TxValidationState tx_state; // If CheckInputScripts is called with a pointer to a checks vector, the resulting checks are appended to it. In that case // they need to be added to control which runs them asynchronously. Otherwise, CheckInputScripts runs the checks before returning. if (control) { std::vector<CScriptCheck> vChecks; - tx_ok = CheckInputScripts(tx, tx_state, view, flags, /*check_for_block=*/true, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache, &vChecks); - if (tx_ok) control->Add(std::move(vChecks)); + tx_state = CheckInputScripts(tx, view, flags, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache, &vChecks); + if (tx_state.IsValid()) control->Add(std::move(vChecks)); } else { - tx_ok = CheckInputScripts(tx, tx_state, view, flags, /*check_for_block=*/true, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache); + tx_state = CheckInputScripts(tx, view, flags, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache); } - if (!tx_ok) { + if (!tx_state.IsValid()) { // Any transaction validation failure in ConnectBlock is a block consensus failure state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, - tx_state.GetRejectReason(), tx_state.GetDebugMessage()); + strprintf("block-script-verify-flag-failed (%s)", tx_state.GetRejectReason()), tx_state.GetDebugMessage()); break; } }</details>
-
ajtowns commented at 5:34 AM on March 30, 2026: contributor
+ 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_FLAGSuntil a soft-fork has been active for some time, but do updateSTANDARD_SCRIPT_VERIFY_FLAGSso 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 theMANDATORY_SCRIPT_VERIFY_FLAGSwould 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 beTX_CONSENSUS.
We'll already return
TX_NOT_STANDARDfor 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? - I think after this PR, in non-block input script validation path, the result enum after a consensus input script failure is
-
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_blockis set correctly at every call site at all. My question is: why doesCheckInputScriptsneed to set the validation state? It doesn't seem necessary to me.I attempted to simply forward the raw
ScriptErrorto 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 theTX_NOT_STANDARDvsTX_CONSENSUSdecision where it actually belongs, and we no longer need to document the behavior whereCheckInputScriptscan returnTX_NOT_STANDARDfor what is actually a consensus failure and block validation needs to update that.Wdyt?
<details> <summary>diff</summary>
diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp index c8cfbadfa9..7ccc8ee661 100644 --- a/src/test/txvalidationcache_tests.cpp +++ b/src/test/txvalidationcache_tests.cpp @@ -20,10 +20,9 @@ struct Dersig100Setup : public TestChain100Setup { : TestChain100Setup{ChainType::REGTEST, {.extra_args = {"-testactivationheight=dersig@102"}}} {} }; -bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, - const CCoinsViewCache& inputs, script_verify_flags flags, bool check_for_block, - bool cacheSigStore, - bool cacheFullScriptStore, PrecomputedTransactionData& txdata, +std::optional<ScriptCheckError> CheckInputScripts(const CTransaction& tx, + const CCoinsViewCache& inputs, script_verify_flags flags, + bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, ValidationCache& validation_cache, std::vector<CScriptCheck>* pvChecks) EXCLUSIVE_LOCKS_REQUIRED(cs_main); @@ -128,8 +127,6 @@ static void ValidateCheckInputsForAllFlags(const CTransaction &tx, script_verify FastRandomContext insecure_rand(true); for (int count = 0; count < 10000; ++count) { - TxValidationState state; - // Randomly selects flag combinations script_verify_flags test_flags = script_verify_flags::from_int(insecure_rand.randrange(MAX_SCRIPT_VERIFY_FLAGS)); @@ -143,23 +140,26 @@ static void ValidateCheckInputsForAllFlags(const CTransaction &tx, script_verify // WITNESS requires P2SH test_flags |= SCRIPT_VERIFY_P2SH; } - bool ret = CheckInputScripts(tx, state, &active_coins_tip, test_flags, /*check_for_block=*/false, true, add_to_cache, txdata, validation_cache, nullp + auto ret = CheckInputScripts(tx, &active_coins_tip, test_flags, true, add_to_cache, txdata, validation_cache, nullptr); // CheckInputScripts should succeed iff test_flags doesn't intersect with // failing_flags bool expected_return_value = !(test_flags & failing_flags); - BOOST_CHECK_EQUAL(ret, expected_return_value); - + BOOST_CHECK_EQUAL(!ret.has_value(), expected_return_value); + if (ret.has_value()) { + BOOST_CHECK(ret->error != SCRIPT_ERR_OK); + BOOST_CHECK(!ret->debug_message.empty()); + } // Test the caching - if (ret && add_to_cache) { + if (!ret.has_value() && add_to_cache) { // Check that we get a cache hit if the tx was valid std::vector<CScriptCheck> scriptchecks; - BOOST_CHECK(CheckInputScripts(tx, state, &active_coins_tip, test_flags, /*check_for_block=*/false, true, add_to_cache, txdata, validation_cache, + BOOST_CHECK(!CheckInputScripts(tx, &active_coins_tip, test_flags, true, add_to_cache, txdata, validation_cache, &scriptchecks).has_value()); BOOST_CHECK(scriptchecks.empty()); } else { // Check that we get script executions to check, if the transaction // was invalid, or we didn't add to cache. std::vector<CScriptCheck> scriptchecks; - BOOST_CHECK(CheckInputScripts(tx, state, &active_coins_tip, test_flags, /*check_for_block=*/false, true, add_to_cache, txdata, validation_cache, + BOOST_CHECK(!CheckInputScripts(tx, &active_coins_tip, test_flags, true, add_to_cache, txdata, validation_cache, &scriptchecks).has_value()); BOOST_CHECK_EQUAL(scriptchecks.size(), tx.vin.size()); } } @@ -214,16 +214,18 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup) { LOCK(cs_main); - TxValidationState state; PrecomputedTransactionData ptd_spend_tx; - - BOOST_CHECK(!CheckInputScripts(CTransaction(spend_tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DER + auto ret = CheckInputScripts(CTransaction(spend_tx), &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERSIG, true + BOOST_CHECK(ret.has_value()); + BOOST_CHECK(ret->error == SCRIPT_ERR_SIG_DER); + BOOST_CHECK_EQUAL(ret->debug_message, strprintf("input 0 of %s (wtxid %s), spending %s:0", spend_tx.GetHash().ToString(), CTransaction(spend_tx).GetW // If we call again asking for scriptchecks (as happens in // ConnectBlock), we should add a script check object for this -- we're // not caching invalidity (if that changes, delete this test case). std::vector<CScriptCheck> scriptchecks; - BOOST_CHECK(CheckInputScripts(CTransaction(spend_tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DERS + auto non_cached_ret{CheckInputScripts(CTransaction(spend_tx), &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_DER + BOOST_CHECK(!non_cached_ret.has_value()); BOOST_CHECK_EQUAL(scriptchecks.size(), 1U); // Test that CheckInputScripts returns true iff DERSIG-enforcing flags are @@ -283,9 +285,8 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup) // Make it valid, and check again invalid_with_cltv_tx.vin[0].scriptSig = CScript() << vchSig << 100; - TxValidationState state; PrecomputedTransactionData txdata; - BOOST_CHECK(CheckInputScripts(CTransaction(invalid_with_cltv_tx), state, m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKLOCKTIMEV + BOOST_CHECK(!CheckInputScripts(CTransaction(invalid_with_cltv_tx), m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY, } // TEST CHECKSEQUENCEVERIFY @@ -311,9 +312,8 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup) // Make it valid, and check again invalid_with_csv_tx.vin[0].scriptSig = CScript() << vchSig << 100; - TxValidationState state; PrecomputedTransactionData txdata; - BOOST_CHECK(CheckInputScripts(CTransaction(invalid_with_csv_tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKSEQUENCEV + BOOST_CHECK(!CheckInputScripts(CTransaction(invalid_with_csv_tx), &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_CHECKSEQUENCEVERIFY, } // TODO: add tests for remaining script flags @@ -372,15 +372,17 @@ BOOST_FIXTURE_TEST_CASE(checkinputs_test, Dersig100Setup) // Invalidate vin[1] tx.vin[1].scriptWitness.SetNull(); - TxValidationState state; PrecomputedTransactionData txdata; // This transaction is now invalid under segwit, because of the second input. - BOOST_CHECK(!CheckInputScripts(CTransaction(tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, + auto ret = CheckInputScripts(CTransaction(tx), &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, tru + BOOST_CHECK(ret.has_value()); + BOOST_CHECK(ret->error == SCRIPT_ERR_WITNESS_PROGRAM_MISMATCH); + BOOST_CHECK_EQUAL(ret->debug_message, strprintf("input 1 of %s (wtxid %s), spending %s:1", tx.GetHash().ToString(), CTransaction(tx).GetWitnessHash() std::vector<CScriptCheck> scriptchecks; // Make sure this transaction was not cached (ie because the first // input was valid) - BOOST_CHECK(CheckInputScripts(CTransaction(tx), state, &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, / + BOOST_CHECK(!CheckInputScripts(CTransaction(tx), &m_node.chainman->ActiveChainstate().CoinsTip(), SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS, true, t // Should get 2 script checks back -- caching is on a whole-transaction basis. BOOST_CHECK_EQUAL(scriptchecks.size(), 2U); } diff --git a/src/validation.cpp b/src/validation.cpp index 5d95edf4db..005702d09b 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -138,8 +138,8 @@ const CBlockIndex* Chainstate::FindForkInGlobalIndex(const CBlockLocator& locato return m_chain.Genesis(); } -bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, - const CCoinsViewCache& inputs, script_verify_flags flags, bool check_for_block, +std::optional<ScriptCheckError> CheckInputScripts(const CTransaction& tx, + const CCoinsViewCache& inputs, script_verify_flags flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, ValidationCache& validation_cache, @@ -429,7 +429,12 @@ static bool CheckInputsFromMempoolAndCache(const CTransaction& tx, TxValidationS } // Call CheckInputScripts() to cache signature and script validity against current tip consensus rules. - return CheckInputScripts(tx, state, view, flags, /*check_for_block=*/false, /* cacheSigStore= */ true, /* cacheFullScriptStore= */ true, txdata, validati + if (auto err = CheckInputScripts(tx, view, flags, /* cacheSigStore= */ true, /* cacheFullScriptStore= */ true, txdata, validation_cache)) { + auto validation_result = flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS ? TxValidationResult::TX_NOT_STANDARD : TxValidationResult::TX_CONSENSUS; + state.Invalid(validation_result, strprintf("mempool-script-verify-flag-failed (%s)", ScriptErrorString(err->error)), err->debug_message); + return false; + } + return true; } namespace { @@ -1253,13 +1258,14 @@ bool MemPoolAccept::PolicyScriptChecks(const ATMPArgs& args, Workspace& ws) // Check input scripts and signatures. // This is done last to help prevent CPU exhaustion denial-of-service attacks. - if (!CheckInputScripts(tx, state, m_view, scriptVerifyFlags, /*check_for_block=*/false, true, false, ws.m_precomputed_txdata, GetValidationCache())) { + if (auto err = CheckInputScripts(tx, m_view, scriptVerifyFlags, true, false, ws.m_precomputed_txdata, GetValidationCache())) { + auto validation_result = scriptVerifyFlags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS ? TxValidationResult::TX_NOT_STANDARD : TxValidationResult::TX_CONSE // Detect a failure due to a missing witness so that p2p code can handle rejection caching appropriately. if (!tx.HasWitness() && SpendsNonAnchorWitnessProg(tx, m_view)) { - state.Invalid(TxValidationResult::TX_WITNESS_STRIPPED, - state.GetRejectReason(), state.GetDebugMessage()); + validation_result = TxValidationResult::TX_WITNESS_STRIPPED; } - return false; // state filled in by CheckInputScripts + state.Invalid(validation_result, strprintf("mempool-script-verify-flag-failed (%s)", ScriptErrorString(err->error)), err->debug_message); + return false; } return true; @@ -2134,23 +2140,17 @@ ValidationCache::ValidationCache(const size_t script_execution_cache_bytes, cons * which are matched. This is useful for checking blocks where we will likely never need the cache * entry again. * - * Note that we may set state.reason to NOT_STANDARD for extra soft-fork flags in flags, block-checking - * callers should probably reset it to CONSENSUS in such cases. * * Non-static (and redeclared) in src/test/txvalidationcache_tests.cpp */ -bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, - const CCoinsViewCache& inputs, script_verify_flags flags, bool check_for_block, +std::optional<ScriptCheckError> CheckInputScripts(const CTransaction& tx, + const CCoinsViewCache& inputs, script_verify_flags flags, bool cacheSigStore, bool cacheFullScriptStore, PrecomputedTransactionData& txdata, ValidationCache& validation_cache, std::vector<CScriptCheck>* pvChecks) { - if (tx.IsCoinBase()) return true; - - if (flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) { - assert(!check_for_block); - } + if (tx.IsCoinBase()) return std::nullopt; if (pvChecks) { pvChecks->reserve(tx.vin.size()); @@ -2166,7 +2166,7 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, hasher.Write(UCharCast(tx.GetWitnessHash().begin()), 32).Write((unsigned char*)&flags, sizeof(flags)).Finalize(hashCacheEntry.begin()); AssertLockHeld(cs_main); //TODO: Remove this requirement by making CuckooCache not require external locks if (validation_cache.m_script_execution_cache.contains(hashCacheEntry, !cacheFullScriptStore)) { - return true; + return std::nullopt; } if (!txdata.m_spent_outputs_ready) { @@ -2196,17 +2196,7 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, if (pvChecks) { pvChecks->emplace_back(std::move(check)); } else if (auto result = check(); result.has_value()) { - // Tx failures never trigger disconnections/bans. - // This is so that network splits aren't triggered - // either due to non-consensus relay policies (such as - // non-standard DER encodings or non-null dummy - // arguments) or due to new consensus rules introduced in - // soft forks. - if (!check_for_block) { - return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("mempool-script-verify-flag-failed (%s)", ScriptErrorString(result->first)), result->second); - } else { - return state.Invalid(TxValidationResult::TX_CONSENSUS, strprintf("block-script-verify-flag-failed (%s)", ScriptErrorString(result->first)), result->second); - } + return ScriptCheckError{result->first, result->second}; } } @@ -2216,7 +2206,7 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state, validation_cache.m_script_execution_cache.insert(hashCacheEntry); } - return true; + return std::nullopt; } bool FatalError(Notifications& notifications, BlockValidationState& state, const bilingual_str& message) @@ -2663,21 +2653,20 @@ bool Chainstate::ConnectBlock(const CBlock& block, BlockValidationState& state, if (!tx.IsCoinBase() && fScriptChecks) { bool fCacheResults = fJustCheck; /* Don't cache results if we're actually connecting blocks (still consult the cache, though) */ - bool tx_ok; - TxValidationState tx_state; + std::optional<ScriptCheckError> script_check_err; // If CheckInputScripts is called with a pointer to a checks vector, the resulting checks are appended to it. In that case // they need to be added to control which runs them asynchronously. Otherwise, CheckInputScripts runs the checks before returning. if (control) { std::vector<CScriptCheck> vChecks; - tx_ok = CheckInputScripts(tx, tx_state, view, flags, /*check_for_block=*/true, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache, &vChecks); - if (tx_ok) control->Add(std::move(vChecks)); + script_check_err = CheckInputScripts(tx, view, flags, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache, &vChecks); + if (!script_check_err) control->Add(std::move(vChecks)); } else { - tx_ok = CheckInputScripts(tx, tx_state, view, flags, /*check_for_block=*/true, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache); + script_check_err = CheckInputScripts(tx, view, flags, fCacheResults, fCacheResults, txsdata[i], m_chainman.m_validation_cache); } - if (!tx_ok) { + if (script_check_err) { // Any transaction validation failure in ConnectBlock is a block consensus failure state.Invalid(BlockValidationResult::BLOCK_CONSENSUS, - tx_state.GetRejectReason(), tx_state.GetDebugMessage()); + strprintf("block-script-verify-flag-failed (%s)", ScriptErrorString(script_check_err->error)), script_check_err->debug_message); break; } } diff --git a/src/validation.h b/src/validation.h index cd448f3ca9..c34ae41159 100644 --- a/src/validation.h +++ b/src/validation.h @@ -331,6 +331,11 @@ bool CheckSequenceLocksAtTip(CBlockIndex* tip, * Closure representing one script verification * Note that this stores references to the spending transaction */ +struct ScriptCheckError { + ScriptError error; + std::string debug_message; +}; + class CScriptCheck { private:</details>
-
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_FLAGSsince #33050 , fixed in https://github.com/bitcoin/bitcoin/pull/34957 -
ajtowns commented at 9:22 AM on April 2, 2026: contributor
So
CheckInputScriptsshould 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_STANDARDandmempool-script-verify-flag-failedorTX_CONSENSUSandblock-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:# 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?
std_reject = template.std_reject_reason or template.reject_reason assert std_reject is not Noneinstagibbs commented at 8:31 PM on April 6, 2026: memberOne 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?
4c35d39f13tests: 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.
tests: test tx rejection for both -acceptnonstdtxn=0 and 1 aaa909d9c2validation: Be explicit about whether CheckInputScripts is for block/mempool f876cffa9cvalidation: Check only MANDATORY_SCRIPT_VERIFY_FLAGS when -acceptnonstdtxn is set dfe4cc6cc2ajtowns force-pushed on Apr 7, 2026ajtowns commented at 12:54 PM on April 7, 2026: contributorFor 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 ofCheckInputScripts()) 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-diffto 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: memberI'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: contributorI 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: memberhardcoded 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: memberlogic looks correct, some suggestions dfe4cc6cc2003b077f95e30e50c47f1fa28fb2e2
instagibbs commented at 3:44 PM on April 8, 2026: memberIf 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: contributorIf 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_FLAGSuntil a soft-fork has been active for some time, but do updateSTANDARD_SCRIPT_VERIFY_FLAGSso 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 theMANDATORY_SCRIPT_VERIFY_FLAGSwould 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
GetBlockScriptFlagscan and should return values that aren't insideMANDATORY_SCRIPT_VERIFY_FLAGS, butif (flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) { assert(!check_for_block); }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 toSTANDARD_SCRIPT_VERIFY_FLAGSandGetBlockScriptFlags).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 violatingbad-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: membereg, 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.
github-metadata-mirror
This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-07 12:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me
