From 97088fa75aa0af5355587ce3522320f459e35204 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 4 Aug 2025 14:06:27 -0400 Subject: [PATCH 01/11] qa: test witness stripping in p2p_segwit A stripped witness is detected as a special case in mempool acceptance to make sure we do not add the wtxid (which is =txid since witness is stripped) to the reject filter. This is because it may interfere with 1p1c parent relay which currently uses orphan reconciliation (and originally it was until wtxid-relay was widely adopted on the network. This commit adds a test for this special case in the p2p_segwit function test, both when spending a native segwit output and when spending a p2sh-wrapped segwit output. Thanks to Eugene Siegel for pointing out the p2sh-wrapped detection did not have test coverage by finding a bug in a related patch of mine. Github-Pull: #33105 Rebased-From: eb073209db9efdbc2c94bc1f535a27ec6b20d954 --- test/functional/p2p_segwit.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py index 9caf5a19aad..e8f7f7e0f4e 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -707,6 +707,12 @@ class SegWitTest(BitcoinTestFramework): expected_msgs=[spend_tx.hash, 'was not accepted: mandatory-script-verify-flag-failed (Witness program was passed an empty witness)']): test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False) + # The transaction was detected as witness stripped above and not added to the reject + # filter. Trying again will check it again and result in the same error. + with self.nodes[0].assert_debug_log( + expected_msgs=[spend_tx.hash, 'was not accepted: mandatory-script-verify-flag-failed (Witness program was passed an empty witness)']): + test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False) + # Try to put the witness script in the scriptSig, should also fail. spend_tx.vin[0].scriptSig = CScript([p2wsh_pubkey, b'a']) spend_tx.rehash() @@ -1282,6 +1288,13 @@ class SegWitTest(BitcoinTestFramework): test_transaction_acceptance(self.nodes[0], self.test_node, tx2, with_witness=True, accepted=True) test_transaction_acceptance(self.nodes[0], self.test_node, tx3, with_witness=True, accepted=False) + # Now do the opposite: strip the witness entirely. This will be detected as witness stripping and + # the (w)txid won't be added to the reject filter: we can try again and get the same error. + tx3.wit.vtxinwit[0].scriptWitness.stack = [] + reason = "was not accepted: mandatory-script-verify-flag-failed (Witness program was passed an empty witness)" + test_transaction_acceptance(self.nodes[0], self.test_node, tx3, with_witness=False, accepted=False, reason=reason) + test_transaction_acceptance(self.nodes[0], self.test_node, tx3, with_witness=False, accepted=False, reason=reason) + # Get rid of the extra witness, and verify acceptance. tx3.wit.vtxinwit[0].scriptWitness.stack = [witness_script] # Also check that old_node gets a tx announcement, even though this is From 56626300b80dced9e111a39d5c560b0b81276cb8 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Mon, 4 Aug 2025 13:11:33 -0400 Subject: [PATCH 02/11] policy: introduce a helper to detect whether a transaction spends Segwit outputs We will use this helper in later commits to detect witness stripping without having to execute every input Script three times in a row. Github-Pull: #33105 Rebased-From: 2907b58834ab011f7dd0c42d323e440abd227c25 --- src/policy/policy.cpp | 36 ++++++++ src/policy/policy.h | 5 ++ src/test/transaction_tests.cpp | 155 +++++++++++++++++++++++++++++++++ 3 files changed, 196 insertions(+) diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp index 545387d150c..5942747d609 100644 --- a/src/policy/policy.cpp +++ b/src/policy/policy.cpp @@ -344,6 +344,42 @@ bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs) return true; } +bool SpendsNonAnchorWitnessProg(const CTransaction& tx, const CCoinsViewCache& prevouts) +{ + if (tx.IsCoinBase()) { + return false; + } + + int version; + std::vector program; + for (const auto& txin: tx.vin) { + const auto& prev_spk{prevouts.AccessCoin(txin.prevout).out.scriptPubKey}; + + // Note this includes not-yet-defined witness programs. + if (prev_spk.IsWitnessProgram(version, program) && !prev_spk.IsPayToAnchor(version, program)) { + return true; + } + + // For P2SH extract the redeem script and check if it spends a non-Taproot witness program. Note + // this is fine to call EvalScript (as done in AreInputsStandard/IsWitnessStandard) because this + // function is only ever called after IsStandardTx, which checks the scriptsig is pushonly. + if (prev_spk.IsPayToScriptHash()) { + // If EvalScript fails or results in an empty stack, the transaction is invalid by consensus. + std::vector > stack; + if (!EvalScript(stack, txin.scriptSig, SCRIPT_VERIFY_NONE, BaseSignatureChecker{}, SigVersion::BASE) + || stack.empty()) { + continue; + } + const CScript redeem_script{stack.back().begin(), stack.back().end()}; + if (redeem_script.IsWitnessProgram(version, program)) { + return true; + } + } + } + + return false; +} + int64_t GetVirtualTransactionSize(int64_t nWeight, int64_t nSigOpCost, unsigned int bytes_per_sigop) { return (std::max(nWeight, nSigOpCost * bytes_per_sigop) + WITNESS_SCALE_FACTOR - 1) / WITNESS_SCALE_FACTOR; diff --git a/src/policy/policy.h b/src/policy/policy.h index bf6224af3db..a6ce608bcfa 100644 --- a/src/policy/policy.h +++ b/src/policy/policy.h @@ -167,6 +167,11 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs) * Also enforce a maximum stack item size limit and no annexes for tapscript spends. */ bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs); +/** + * Check whether this transaction spends any witness program but P2A, including not-yet-defined ones. + * May return `false` early for consensus-invalid transactions. + */ +bool SpendsNonAnchorWitnessProg(const CTransaction& tx, const CCoinsViewCache& prevouts); /** Compute the virtual transaction size (weight reinterpreted as bytes). */ int64_t GetVirtualTransactionSize(int64_t nWeight, int64_t nSigOpCost, unsigned int bytes_per_sigop); diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index 2db30e20331..5844ab23bc8 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -1144,4 +1144,159 @@ BOOST_AUTO_TEST_CASE(max_standard_legacy_sigops) BOOST_CHECK(!::AreInputsStandard(CTransaction(tx_max_sigops), coins)); } +/** Sanity check the return value of SpendsNonAnchorWitnessProg for various output types. */ +BOOST_AUTO_TEST_CASE(spends_witness_prog) +{ + CCoinsView coins_dummy; + CCoinsViewCache coins(&coins_dummy); + CKey key; + key.MakeNewKey(true); + const CPubKey pubkey{key.GetPubKey()}; + CMutableTransaction tx_create{}, tx_spend{}; + tx_create.vout.emplace_back(0, CScript{}); + tx_spend.vin.emplace_back(Txid{}, 0); + std::vector> sol_dummy; + + // CNoDestination, PubKeyDestination, PKHash, ScriptHash, WitnessV0ScriptHash, WitnessV0KeyHash, + // WitnessV1Taproot, PayToAnchor, WitnessUnknown. + static_assert(std::variant_size_v == 9); + + // Go through all defined output types and sanity check SpendsNonAnchorWitnessProg. + + // P2PK + tx_create.vout[0].scriptPubKey = GetScriptForDestination(PubKeyDestination{pubkey}); + BOOST_CHECK_EQUAL(Solver(tx_create.vout[0].scriptPubKey, sol_dummy), TxoutType::PUBKEY); + tx_spend.vin[0].prevout.hash = tx_create.GetHash(); + AddCoins(coins, CTransaction{tx_create}, 0, false); + BOOST_CHECK(!::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins)); + + // P2PKH + tx_create.vout[0].scriptPubKey = GetScriptForDestination(PKHash{pubkey}); + BOOST_CHECK_EQUAL(Solver(tx_create.vout[0].scriptPubKey, sol_dummy), TxoutType::PUBKEYHASH); + tx_spend.vin[0].prevout.hash = tx_create.GetHash(); + AddCoins(coins, CTransaction{tx_create}, 0, false); + BOOST_CHECK(!::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins)); + + // P2SH + auto redeem_script{CScript{} << OP_1 << OP_CHECKSIG}; + tx_create.vout[0].scriptPubKey = GetScriptForDestination(ScriptHash{redeem_script}); + BOOST_CHECK_EQUAL(Solver(tx_create.vout[0].scriptPubKey, sol_dummy), TxoutType::SCRIPTHASH); + tx_spend.vin[0].prevout.hash = tx_create.GetHash(); + tx_spend.vin[0].scriptSig = CScript{} << OP_0 << ToByteVector(redeem_script); + AddCoins(coins, CTransaction{tx_create}, 0, false); + BOOST_CHECK(!::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins)); + tx_spend.vin[0].scriptSig.clear(); + + // native P2WSH + const auto witness_script{CScript{} << OP_12 << OP_HASH160 << OP_DUP << OP_EQUAL}; + tx_create.vout[0].scriptPubKey = GetScriptForDestination(WitnessV0ScriptHash{witness_script}); + BOOST_CHECK_EQUAL(Solver(tx_create.vout[0].scriptPubKey, sol_dummy), TxoutType::WITNESS_V0_SCRIPTHASH); + tx_spend.vin[0].prevout.hash = tx_create.GetHash(); + AddCoins(coins, CTransaction{tx_create}, 0, false); + BOOST_CHECK(::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins)); + + // P2SH-wrapped P2WSH + redeem_script = tx_create.vout[0].scriptPubKey; + tx_create.vout[0].scriptPubKey = GetScriptForDestination(ScriptHash(redeem_script)); + BOOST_CHECK_EQUAL(Solver(tx_create.vout[0].scriptPubKey, sol_dummy), TxoutType::SCRIPTHASH); + tx_spend.vin[0].prevout.hash = tx_create.GetHash(); + tx_spend.vin[0].scriptSig = CScript{} << ToByteVector(redeem_script); + AddCoins(coins, CTransaction{tx_create}, 0, false); + BOOST_CHECK(::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins)); + tx_spend.vin[0].scriptSig.clear(); + BOOST_CHECK(!::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins)); + + // native P2WPKH + tx_create.vout[0].scriptPubKey = GetScriptForDestination(WitnessV0KeyHash{pubkey}); + BOOST_CHECK_EQUAL(Solver(tx_create.vout[0].scriptPubKey, sol_dummy), TxoutType::WITNESS_V0_KEYHASH); + tx_spend.vin[0].prevout.hash = tx_create.GetHash(); + AddCoins(coins, CTransaction{tx_create}, 0, false); + BOOST_CHECK(::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins)); + + // P2SH-wrapped P2WPKH + redeem_script = tx_create.vout[0].scriptPubKey; + tx_create.vout[0].scriptPubKey = GetScriptForDestination(ScriptHash(redeem_script)); + BOOST_CHECK_EQUAL(Solver(tx_create.vout[0].scriptPubKey, sol_dummy), TxoutType::SCRIPTHASH); + tx_spend.vin[0].prevout.hash = tx_create.GetHash(); + tx_spend.vin[0].scriptSig = CScript{} << ToByteVector(redeem_script); + AddCoins(coins, CTransaction{tx_create}, 0, false); + BOOST_CHECK(::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins)); + tx_spend.vin[0].scriptSig.clear(); + BOOST_CHECK(!::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins)); + + // P2TR + tx_create.vout[0].scriptPubKey = GetScriptForDestination(WitnessV1Taproot{XOnlyPubKey{pubkey}}); + BOOST_CHECK_EQUAL(Solver(tx_create.vout[0].scriptPubKey, sol_dummy), TxoutType::WITNESS_V1_TAPROOT); + tx_spend.vin[0].prevout.hash = tx_create.GetHash(); + AddCoins(coins, CTransaction{tx_create}, 0, false); + BOOST_CHECK(::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins)); + + // P2SH-wrapped P2TR (undefined, non-standard) + redeem_script = tx_create.vout[0].scriptPubKey; + tx_create.vout[0].scriptPubKey = GetScriptForDestination(ScriptHash(redeem_script)); + BOOST_CHECK_EQUAL(Solver(tx_create.vout[0].scriptPubKey, sol_dummy), TxoutType::SCRIPTHASH); + tx_spend.vin[0].prevout.hash = tx_create.GetHash(); + tx_spend.vin[0].scriptSig = CScript{} << ToByteVector(redeem_script); + AddCoins(coins, CTransaction{tx_create}, 0, false); + BOOST_CHECK(::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins)); + tx_spend.vin[0].scriptSig.clear(); + BOOST_CHECK(!::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins)); + + // P2A + tx_create.vout[0].scriptPubKey = GetScriptForDestination(PayToAnchor{}); + BOOST_CHECK_EQUAL(Solver(tx_create.vout[0].scriptPubKey, sol_dummy), TxoutType::ANCHOR); + tx_spend.vin[0].prevout.hash = tx_create.GetHash(); + AddCoins(coins, CTransaction{tx_create}, 0, false); + BOOST_CHECK(!::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins)); + + // P2SH-wrapped P2A (undefined, non-standard) + redeem_script = tx_create.vout[0].scriptPubKey; + tx_create.vout[0].scriptPubKey = GetScriptForDestination(ScriptHash(redeem_script)); + BOOST_CHECK_EQUAL(Solver(tx_create.vout[0].scriptPubKey, sol_dummy), TxoutType::SCRIPTHASH); + tx_spend.vin[0].prevout.hash = tx_create.GetHash(); + tx_spend.vin[0].scriptSig = CScript{} << ToByteVector(redeem_script); + AddCoins(coins, CTransaction{tx_create}, 0, false); + BOOST_CHECK(::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins)); + tx_spend.vin[0].scriptSig.clear(); + + // Undefined version 1 witness program + tx_create.vout[0].scriptPubKey = GetScriptForDestination(WitnessUnknown{1, {0x42, 0x42}}); + BOOST_CHECK_EQUAL(Solver(tx_create.vout[0].scriptPubKey, sol_dummy), TxoutType::WITNESS_UNKNOWN); + tx_spend.vin[0].prevout.hash = tx_create.GetHash(); + AddCoins(coins, CTransaction{tx_create}, 0, false); + BOOST_CHECK(::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins)); + + // P2SH-wrapped undefined version 1 witness program + redeem_script = tx_create.vout[0].scriptPubKey; + tx_create.vout[0].scriptPubKey = GetScriptForDestination(ScriptHash(redeem_script)); + BOOST_CHECK_EQUAL(Solver(tx_create.vout[0].scriptPubKey, sol_dummy), TxoutType::SCRIPTHASH); + tx_spend.vin[0].prevout.hash = tx_create.GetHash(); + tx_spend.vin[0].scriptSig = CScript{} << ToByteVector(redeem_script); + AddCoins(coins, CTransaction{tx_create}, 0, false); + BOOST_CHECK(::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins)); + tx_spend.vin[0].scriptSig.clear(); + BOOST_CHECK(!::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins)); + + // Various undefined version >1 32-byte witness programs. + const auto program{ToByteVector(XOnlyPubKey{pubkey})}; + for (int i{2}; i <= 16; ++i) { + tx_create.vout[0].scriptPubKey = GetScriptForDestination(WitnessUnknown{i, program}); + BOOST_CHECK_EQUAL(Solver(tx_create.vout[0].scriptPubKey, sol_dummy), TxoutType::WITNESS_UNKNOWN); + tx_spend.vin[0].prevout.hash = tx_create.GetHash(); + AddCoins(coins, CTransaction{tx_create}, 0, false); + BOOST_CHECK(::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins)); + + // It's also detected within P2SH. + redeem_script = tx_create.vout[0].scriptPubKey; + tx_create.vout[0].scriptPubKey = GetScriptForDestination(ScriptHash(redeem_script)); + BOOST_CHECK_EQUAL(Solver(tx_create.vout[0].scriptPubKey, sol_dummy), TxoutType::SCRIPTHASH); + tx_spend.vin[0].prevout.hash = tx_create.GetHash(); + tx_spend.vin[0].scriptSig = CScript{} << ToByteVector(redeem_script); + AddCoins(coins, CTransaction{tx_create}, 0, false); + BOOST_CHECK(::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins)); + tx_spend.vin[0].scriptSig.clear(); + BOOST_CHECK(!::SpendsNonAnchorWitnessProg(CTransaction{tx_spend}, coins)); + } +} + BOOST_AUTO_TEST_SUITE_END() From 020ed613bed1148888692cb37e3522202bfca44e Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Wed, 30 Jul 2025 15:56:57 -0400 Subject: [PATCH 03/11] validation: detect witness stripping without re-running Script checks Since it was introduced in 4eb515574e1012bc8ea5dafc3042dcdf4c766f26 (#18044), the detection of a stripped witness relies on running the Script checks 3 times. In the worst case, this consists in running Script validation 3 times for every single input. Detection of a stripped witness is necessary because in this case wtxid==txid, and the transaction's wtxid must not be added to the reject filter or it could allow a malicious peer to interfere with txid-based orphan resolution as used in 1p1c package relay. However it is not necessary to run Script validation to detect a stripped witness (much less so doing it 3 times in a row). There are 3 types of witness program: defined program types (Taproot, P2WPKH, P2WSH), undefined types, and the Pay-to-anchor carve-out. For defined program types, Script validation with an empty witness will always fail (by consensus). For undefined program types, Script validation is always going to fail regardless of the witness (by standardness). For P2A, an empty witness is never going to lead to a failure. Therefore it holds that we can always detect a stripped witness without re-running Script validation. However this might lead to more "false positives" (cases where we return witness stripping for an otherwise invalid transaction) than the existing implementation. For instance a transaction with one P2PKH input with an invalid signature and one P2WPKH input with its witness stripped. The existing implementation would treat it as consensus invalid while the implementation in this commit would always consider it witness stripped. Github-Pull: #33105 Rebased-From: 27aefac42505e9c083fa131d3d7edbec7803f3c0 --- src/validation.cpp | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index fde064458dc..36734bc6122 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -1236,13 +1236,8 @@ 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, true, false, ws.m_precomputed_txdata, GetValidationCache())) { - // SCRIPT_VERIFY_CLEANSTACK requires SCRIPT_VERIFY_WITNESS, so we - // need to turn both off, and compare against just turning off CLEANSTACK - // to see if the failure is specifically due to witness validation. - TxValidationState state_dummy; // Want reported failures to be from first CheckInputScripts - if (!tx.HasWitness() && CheckInputScripts(tx, state_dummy, m_view, scriptVerifyFlags & ~(SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_CLEANSTACK), true, false, ws.m_precomputed_txdata, GetValidationCache()) && - !CheckInputScripts(tx, state_dummy, m_view, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true, false, ws.m_precomputed_txdata, GetValidationCache())) { - // Only the witness is missing, so the transaction itself may be fine. + // 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()); } From 5a0506eea03e423121dd2112c2ba5fb4320022e3 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 25 Apr 2025 16:13:25 -0400 Subject: [PATCH 04/11] tests: add sighash caching tests to feature_taproot Github-Pull: #32473 Rebased-From: 9014d4016ad9351cb59b587541895e55f5d589cc --- test/functional/feature_taproot.py | 98 ++++++++++++++++++++++++++++-- 1 file changed, 93 insertions(+), 5 deletions(-) diff --git a/test/functional/feature_taproot.py b/test/functional/feature_taproot.py index 4acb7524fba..198bec7df53 100755 --- a/test/functional/feature_taproot.py +++ b/test/functional/feature_taproot.py @@ -71,6 +71,7 @@ from test_framework.script import ( OP_PUSHDATA1, OP_RETURN, OP_SWAP, + OP_TUCK, OP_VERIFY, SIGHASH_DEFAULT, SIGHASH_ALL, @@ -171,9 +172,9 @@ def get(ctx, name): ctx[name] = expr return expr.value -def getter(name): +def getter(name, **kwargs): """Return a callable that evaluates name in its passed context.""" - return lambda ctx: get(ctx, name) + return lambda ctx: get({**ctx, **kwargs}, name) def override(expr, **kwargs): """Return a callable that evaluates expr in a modified context.""" @@ -217,6 +218,20 @@ def default_controlblock(ctx): """Default expression for "controlblock": combine leafversion, negflag, pubkey_internal, merklebranch.""" return bytes([get(ctx, "leafversion") + get(ctx, "negflag")]) + get(ctx, "pubkey_internal") + get(ctx, "merklebranch") +def default_scriptcode_suffix(ctx): + """Default expression for "scriptcode_suffix", the actually used portion of the scriptcode.""" + scriptcode = get(ctx, "scriptcode") + codesepnum = get(ctx, "codesepnum") + if codesepnum == -1: + return scriptcode + codeseps = 0 + for (opcode, data, sop_idx) in scriptcode.raw_iter(): + if opcode == OP_CODESEPARATOR: + if codeseps == codesepnum: + return CScript(scriptcode[sop_idx+1:]) + codeseps += 1 + assert False + def default_sigmsg(ctx): """Default expression for "sigmsg": depending on mode, compute BIP341, BIP143, or legacy sigmsg.""" tx = get(ctx, "tx") @@ -236,12 +251,12 @@ def default_sigmsg(ctx): return TaprootSignatureMsg(tx, utxos, hashtype, idx, scriptpath=False, annex=annex) elif mode == "witv0": # BIP143 signature hash - scriptcode = get(ctx, "scriptcode") + scriptcode = get(ctx, "scriptcode_suffix") utxos = get(ctx, "utxos") return SegwitV0SignatureMsg(scriptcode, tx, idx, hashtype, utxos[idx].nValue) else: # Pre-segwit signature hash - scriptcode = get(ctx, "scriptcode") + scriptcode = get(ctx, "scriptcode_suffix") return LegacySignatureMsg(scriptcode, tx, idx, hashtype)[0] def default_sighash(ctx): @@ -301,7 +316,12 @@ def default_hashtype_actual(ctx): def default_bytes_hashtype(ctx): """Default expression for "bytes_hashtype": bytes([hashtype_actual]) if not 0, b"" otherwise.""" - return bytes([x for x in [get(ctx, "hashtype_actual")] if x != 0]) + mode = get(ctx, "mode") + hashtype_actual = get(ctx, "hashtype_actual") + if mode != "taproot" or hashtype_actual != 0: + return bytes([hashtype_actual]) + else: + return bytes() def default_sign(ctx): """Default expression for "sign": concatenation of signature and bytes_hashtype.""" @@ -379,6 +399,8 @@ DEFAULT_CONTEXT = { "key_tweaked": default_key_tweaked, # The tweak to use (None for script path spends, the actual tweak for key path spends). "tweak": default_tweak, + # The part of the scriptcode after the last executed OP_CODESEPARATOR. + "scriptcode_suffix": default_scriptcode_suffix, # The sigmsg value (preimage of sighash) "sigmsg": default_sigmsg, # The sighash value (32 bytes) @@ -409,6 +431,8 @@ DEFAULT_CONTEXT = { "annex": None, # The codeseparator position (only when mode=="taproot"). "codeseppos": -1, + # Which OP_CODESEPARATOR is the last executed one in the script (in legacy/P2SH/P2WSH). + "codesepnum": -1, # The redeemscript to add to the scriptSig (if P2SH; None implies not P2SH). "script_p2sh": None, # The script to add to the witness in (if P2WSH; None implies P2WPKH) @@ -1210,6 +1234,70 @@ def spenders_taproot_active(): standard = hashtype in VALID_SIGHASHES_ECDSA and (p2sh or witv0) add_spender(spenders, "compat/nocsa", hashtype=hashtype, p2sh=p2sh, witv0=witv0, standard=standard, script=CScript([OP_IF, OP_11, pubkey1, OP_CHECKSIGADD, OP_12, OP_EQUAL, OP_ELSE, pubkey1, OP_CHECKSIG, OP_ENDIF]), key=eckey1, sigops_weight=4-3*witv0, inputs=[getter("sign"), b''], failure={"inputs": [getter("sign"), b'\x01']}, **ERR_UNDECODABLE) + # == sighash caching tests == + + # Sighash caching in legacy. + for p2sh in [False, True]: + for witv0 in [False, True]: + eckey1, pubkey1 = generate_keypair(compressed=compressed) + for _ in range(10): + # Construct a script with 20 checksig operations (10 sighash types, each 2 times), + # randomly ordered and interleaved with 4 OP_CODESEPARATORS. + ops = [1, 2, 3, 0x21, 0x42, 0x63, 0x81, 0x83, 0xe1, 0xc2, -1, -1] * 2 + # Make sure no OP_CODESEPARATOR appears last. + while True: + random.shuffle(ops) + if ops[-1] != -1: + break + script = [pubkey1] + inputs = [] + codeseps = -1 + for pos, op in enumerate(ops): + if op == -1: + codeseps += 1 + script.append(OP_CODESEPARATOR) + elif pos + 1 != len(ops): + script += [OP_TUCK, OP_CHECKSIGVERIFY] + inputs.append(getter("sign", codesepnum=codeseps, hashtype=op)) + else: + script += [OP_CHECKSIG] + inputs.append(getter("sign", codesepnum=codeseps, hashtype=op)) + inputs.reverse() + script = CScript(script) + add_spender(spenders, "sighashcache/legacy", p2sh=p2sh, witv0=witv0, standard=False, script=script, inputs=inputs, key=eckey1, sigops_weight=12*8*(4-3*witv0), no_fail=True) + + # Sighash caching in tapscript. + for _ in range(10): + # Construct a script with 700 checksig operations (7 sighash types, each 100 times), + # randomly ordered and interleaved with 100 OP_CODESEPARATORS. + ops = [0, 1, 2, 3, 0x81, 0x82, 0x83, -1] * 100 + # Make sure no OP_CODESEPARATOR appears last. + while True: + random.shuffle(ops) + if ops[-1] != -1: + break + script = [pubs[1]] + inputs = [] + opcount = 1 + codeseppos = -1 + for pos, op in enumerate(ops): + if op == -1: + codeseppos = opcount + opcount += 1 + script.append(OP_CODESEPARATOR) + elif pos + 1 != len(ops): + opcount += 2 + script += [OP_TUCK, OP_CHECKSIGVERIFY] + inputs.append(getter("sign", codeseppos=codeseppos, hashtype=op)) + else: + opcount += 1 + script += [OP_CHECKSIG] + inputs.append(getter("sign", codeseppos=codeseppos, hashtype=op)) + inputs.reverse() + script = CScript(script) + tap = taproot_construct(pubs[0], [("leaf", script)]) + add_spender(spenders, "sighashcache/taproot", tap=tap, leaf="leaf", inputs=inputs, standard=True, key=secs[1], no_fail=True) + return spenders From 354d46bc10c61c45140be7a425c5c29fed934d32 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 25 Apr 2025 13:11:30 -0400 Subject: [PATCH 05/11] script: (refactor) prepare for introducing sighash midstate cache Github-Pull: #32473 Rebased-From: 8f3ddb0bccebc930836b4a6745a7cf29b41eb302 --- src/script/interpreter.cpp | 44 +++++++++++++++++++------------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index a35306b6935..0e304973a98 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -1569,6 +1569,18 @@ uint256 SignatureHash(const CScript& scriptCode, const T& txTo, unsigned int nIn { assert(nIn < txTo.vin.size()); + if (sigversion != SigVersion::WITNESS_V0) { + // Check for invalid use of SIGHASH_SINGLE + if ((nHashType & 0x1f) == SIGHASH_SINGLE) { + if (nIn >= txTo.vout.size()) { + // nOut out of range + return uint256::ONE; + } + } + } + + HashWriter ss{}; + if (sigversion == SigVersion::WITNESS_V0) { uint256 hashPrevouts; uint256 hashSequence; @@ -1583,16 +1595,14 @@ uint256 SignatureHash(const CScript& scriptCode, const T& txTo, unsigned int nIn hashSequence = cacheready ? cache->hashSequence : SHA256Uint256(GetSequencesSHA256(txTo)); } - if ((nHashType & 0x1f) != SIGHASH_SINGLE && (nHashType & 0x1f) != SIGHASH_NONE) { hashOutputs = cacheready ? cache->hashOutputs : SHA256Uint256(GetOutputsSHA256(txTo)); } else if ((nHashType & 0x1f) == SIGHASH_SINGLE && nIn < txTo.vout.size()) { - HashWriter ss{}; - ss << txTo.vout[nIn]; - hashOutputs = ss.GetHash(); + HashWriter inner_ss{}; + inner_ss << txTo.vout[nIn]; + hashOutputs = inner_ss.GetHash(); } - HashWriter ss{}; // Version ss << txTo.version; // Input prevouts/nSequence (none/all, depending on flags) @@ -1609,26 +1619,16 @@ uint256 SignatureHash(const CScript& scriptCode, const T& txTo, unsigned int nIn ss << hashOutputs; // Locktime ss << txTo.nLockTime; - // Sighash type - ss << nHashType; + } else { + // Wrapper to serialize only the necessary parts of the transaction being signed + CTransactionSignatureSerializer txTmp(txTo, scriptCode, nIn, nHashType); - return ss.GetHash(); + // Serialize + ss << txTmp; } - // Check for invalid use of SIGHASH_SINGLE - if ((nHashType & 0x1f) == SIGHASH_SINGLE) { - if (nIn >= txTo.vout.size()) { - // nOut out of range - return uint256::ONE; - } - } - - // Wrapper to serialize only the necessary parts of the transaction being signed - CTransactionSignatureSerializer txTmp(txTo, scriptCode, nIn, nHashType); - - // Serialize and hash - HashWriter ss{}; - ss << txTmp << nHashType; + // Add sighash type and hash. + ss << nHashType; return ss.GetHash(); } From ddfb9150b80c0c692c06b91cefa988c7773b15ff Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Fri, 25 Apr 2025 13:31:18 -0400 Subject: [PATCH 06/11] script: (optimization) introduce sighash midstate caching Github-Pull: #32473 Rebased-From: 92af9f74d74e76681f7d98f293eab226972137b4 --- src/script/interpreter.cpp | 43 ++++++++++++++++++++++++++++++++++++-- src/script/interpreter.h | 22 ++++++++++++++++++- 2 files changed, 62 insertions(+), 3 deletions(-) diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index 0e304973a98..4b7bfcedc6c 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -1564,8 +1564,35 @@ bool SignatureHashSchnorr(uint256& hash_out, ScriptExecutionData& execdata, cons return true; } +int SigHashCache::CacheIndex(int32_t hash_type) const noexcept +{ + // Note that we do not distinguish between BASE and WITNESS_V0 to determine the cache index, + // because no input can simultaneously use both. + return 3 * !!(hash_type & SIGHASH_ANYONECANPAY) + + 2 * ((hash_type & 0x1f) == SIGHASH_SINGLE) + + 1 * ((hash_type & 0x1f) == SIGHASH_NONE); +} + +bool SigHashCache::Load(int32_t hash_type, const CScript& script_code, HashWriter& writer) const noexcept +{ + auto& entry = m_cache_entries[CacheIndex(hash_type)]; + if (entry.has_value()) { + if (script_code == entry->first) { + writer = HashWriter(entry->second); + return true; + } + } + return false; +} + +void SigHashCache::Store(int32_t hash_type, const CScript& script_code, const HashWriter& writer) noexcept +{ + auto& entry = m_cache_entries[CacheIndex(hash_type)]; + entry.emplace(script_code, writer); +} + template -uint256 SignatureHash(const CScript& scriptCode, const T& txTo, unsigned int nIn, int32_t nHashType, const CAmount& amount, SigVersion sigversion, const PrecomputedTransactionData* cache) +uint256 SignatureHash(const CScript& scriptCode, const T& txTo, unsigned int nIn, int32_t nHashType, const CAmount& amount, SigVersion sigversion, const PrecomputedTransactionData* cache, SigHashCache* sighash_cache) { assert(nIn < txTo.vin.size()); @@ -1581,6 +1608,13 @@ uint256 SignatureHash(const CScript& scriptCode, const T& txTo, unsigned int nIn HashWriter ss{}; + // Try to compute using cached SHA256 midstate. + if (sighash_cache && sighash_cache->Load(nHashType, scriptCode, ss)) { + // Add sighash type and hash. + ss << nHashType; + return ss.GetHash(); + } + if (sigversion == SigVersion::WITNESS_V0) { uint256 hashPrevouts; uint256 hashSequence; @@ -1627,6 +1661,11 @@ uint256 SignatureHash(const CScript& scriptCode, const T& txTo, unsigned int nIn ss << txTmp; } + // If a cache object was provided, store the midstate there. + if (sighash_cache != nullptr) { + sighash_cache->Store(nHashType, scriptCode, ss); + } + // Add sighash type and hash. ss << nHashType; return ss.GetHash(); @@ -1661,7 +1700,7 @@ bool GenericTransactionSignatureChecker::CheckECDSASignature(const std::vecto // Witness sighashes need the amount. if (sigversion == SigVersion::WITNESS_V0 && amount < 0) return HandleMissingData(m_mdb); - uint256 sighash = SignatureHash(scriptCode, *txTo, nIn, nHashType, amount, sigversion, this->txdata); + uint256 sighash = SignatureHash(scriptCode, *txTo, nIn, nHashType, amount, sigversion, this->txdata, &m_sighash_cache); if (!VerifyECDSASignature(vchSig, pubkey, sighash)) return false; diff --git a/src/script/interpreter.h b/src/script/interpreter.h index e2fb1998f0b..d613becb8f6 100644 --- a/src/script/interpreter.h +++ b/src/script/interpreter.h @@ -239,8 +239,27 @@ extern const HashWriter HASHER_TAPSIGHASH; //!< Hasher with tag "TapSighash" pre extern const HashWriter HASHER_TAPLEAF; //!< Hasher with tag "TapLeaf" pre-fed to it. extern const HashWriter HASHER_TAPBRANCH; //!< Hasher with tag "TapBranch" pre-fed to it. +/** Data structure to cache SHA256 midstates for the ECDSA sighash calculations + * (bare, P2SH, P2WPKH, P2WSH). */ +class SigHashCache +{ + /** For each sighash mode (ALL, SINGLE, NONE, ALL|ANYONE, SINGLE|ANYONE, NONE|ANYONE), + * optionally store a scriptCode which the hash is for, plus a midstate for the SHA256 + * computation just before adding the hash_type itself. */ + std::optional> m_cache_entries[6]; + + /** Given a hash_type, find which of the 6 cache entries is to be used. */ + int CacheIndex(int32_t hash_type) const noexcept; + +public: + /** Load into writer the SHA256 midstate if found in this cache. */ + [[nodiscard]] bool Load(int32_t hash_type, const CScript& script_code, HashWriter& writer) const noexcept; + /** Store into this cache object the provided SHA256 midstate. */ + void Store(int32_t hash_type, const CScript& script_code, const HashWriter& writer) noexcept; +}; + template -uint256 SignatureHash(const CScript& scriptCode, const T& txTo, unsigned int nIn, int32_t nHashType, const CAmount& amount, SigVersion sigversion, const PrecomputedTransactionData* cache = nullptr); +uint256 SignatureHash(const CScript& scriptCode, const T& txTo, unsigned int nIn, int32_t nHashType, const CAmount& amount, SigVersion sigversion, const PrecomputedTransactionData* cache = nullptr, SigHashCache* sighash_cache = nullptr); class BaseSignatureChecker { @@ -289,6 +308,7 @@ private: unsigned int nIn; const CAmount amount; const PrecomputedTransactionData* txdata; + mutable SigHashCache m_sighash_cache; protected: virtual bool VerifyECDSASignature(const std::vector& vchSig, const CPubKey& vchPubKey, const uint256& sighash) const; From 73d3ab8fc93119f14f72a6c5f3cdd9eedcb36a20 Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Tue, 22 Jul 2025 18:40:23 -0400 Subject: [PATCH 07/11] qa: simple differential fuzzing for sighash with/without caching Github-Pull: #32473 Rebased-From: b221aa80a081579b8d3b460e3403f7ac0daa7139 --- src/test/fuzz/script_interpreter.cpp | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/src/test/fuzz/script_interpreter.cpp b/src/test/fuzz/script_interpreter.cpp index 9e3ad02b2e5..2c2ce855d47 100644 --- a/src/test/fuzz/script_interpreter.cpp +++ b/src/test/fuzz/script_interpreter.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include @@ -45,3 +46,27 @@ FUZZ_TARGET(script_interpreter) (void)CastToBool(ConsumeRandomLengthByteVector(fuzzed_data_provider)); } } + +/** Differential fuzzing for SignatureHash with and without cache. */ +FUZZ_TARGET(sighash_cache) +{ + FuzzedDataProvider provider(buffer.data(), buffer.size()); + + // Get inputs to the sighash function that won't change across types. + const auto scriptcode{ConsumeScript(provider)}; + const auto tx{ConsumeTransaction(provider, std::nullopt)}; + if (tx.vin.empty()) return; + const auto in_index{provider.ConsumeIntegralInRange(0, tx.vin.size() - 1)}; + const auto amount{ConsumeMoney(provider)}; + const auto sigversion{(SigVersion)provider.ConsumeIntegralInRange(0, 1)}; + + // Check the sighash function will give the same result for 100 fuzzer-generated hash types whether or not a cache is + // provided. The cache is conserved across types to exercise cache hits. + SigHashCache sighash_cache{}; + for (int i{0}; i < 100; ++i) { + const auto hash_type{((i & 2) == 0) ? provider.ConsumeIntegral() : provider.ConsumeIntegral()}; + const auto nocache_res{SignatureHash(scriptcode, tx, in_index, hash_type, amount, sigversion)}; + const auto cache_res{SignatureHash(scriptcode, tx, in_index, hash_type, amount, sigversion, nullptr, &sighash_cache)}; + Assert(nocache_res == cache_res); + } +} From f24291bd96f92ecc0fc04317fd93747eeb2d557a Mon Sep 17 00:00:00 2001 From: Antoine Poinsot Date: Tue, 22 Jul 2025 11:23:16 -0400 Subject: [PATCH 08/11] qa: unit test sighash caching Github-Pull: #32473 Rebased-From: 83950275eddacac56c58a7a3648ed435a5593328 --- src/test/sighash_tests.cpp | 90 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/src/test/sighash_tests.cpp b/src/test/sighash_tests.cpp index d3320878ec0..6e2ec800e74 100644 --- a/src/test/sighash_tests.cpp +++ b/src/test/sighash_tests.cpp @@ -207,4 +207,94 @@ BOOST_AUTO_TEST_CASE(sighash_from_data) BOOST_CHECK_MESSAGE(sh.GetHex() == sigHashHex, strTest); } } + +BOOST_AUTO_TEST_CASE(sighash_caching) +{ + // Get a script, transaction and parameters as inputs to the sighash function. + CScript scriptcode; + RandomScript(scriptcode); + CScript diff_scriptcode{scriptcode}; + diff_scriptcode << OP_1; + CMutableTransaction tx; + RandomTransaction(tx, /*fSingle=*/false); + const auto in_index{static_cast(m_rng.randrange(tx.vin.size()))}; + const auto amount{m_rng.rand()}; + + // Exercise the sighash function under both legacy and segwit v0. + for (const auto sigversion: {SigVersion::BASE, SigVersion::WITNESS_V0}) { + // For each, run it against all the 6 standard hash types and a few additional random ones. + std::vector hash_types{{SIGHASH_ALL, SIGHASH_SINGLE, SIGHASH_NONE, SIGHASH_ALL | SIGHASH_ANYONECANPAY, + SIGHASH_SINGLE | SIGHASH_ANYONECANPAY, SIGHASH_NONE | SIGHASH_ANYONECANPAY, + SIGHASH_ANYONECANPAY, 0, std::numeric_limits::max()}}; + for (int i{0}; i < 10; ++i) { + hash_types.push_back(i % 2 == 0 ? m_rng.rand() : m_rng.rand()); + } + + // Reuse the same cache across script types. This must not cause any issue as the cached value for one hash type must never + // be confused for another (instantiating the cache within the loop instead would prevent testing this). + SigHashCache cache; + for (const auto hash_type: hash_types) { + const bool expect_one{sigversion == SigVersion::BASE && ((hash_type & 0x1f) == SIGHASH_SINGLE) && in_index >= tx.vout.size()}; + + // The result of computing the sighash should be the same with or without cache. + const auto sighash_with_cache{SignatureHash(scriptcode, tx, in_index, hash_type, amount, sigversion, nullptr, &cache)}; + const auto sighash_no_cache{SignatureHash(scriptcode, tx, in_index, hash_type, amount, sigversion, nullptr, nullptr)}; + BOOST_CHECK_EQUAL(sighash_with_cache, sighash_no_cache); + + // Calling the cached version again should return the same value again. + BOOST_CHECK_EQUAL(sighash_with_cache, SignatureHash(scriptcode, tx, in_index, hash_type, amount, sigversion, nullptr, &cache)); + + // While here we might as well also check that the result for legacy is the same as for the old SignatureHash() function. + if (sigversion == SigVersion::BASE) { + BOOST_CHECK_EQUAL(sighash_with_cache, SignatureHashOld(scriptcode, CTransaction(tx), in_index, hash_type)); + } + + // Calling with a different scriptcode (for instance in case a CODESEP is encountered) will not return the cache value but + // overwrite it. The sighash will always be different except in case of legacy SIGHASH_SINGLE bug. + const auto sighash_with_cache2{SignatureHash(diff_scriptcode, tx, in_index, hash_type, amount, sigversion, nullptr, &cache)}; + const auto sighash_no_cache2{SignatureHash(diff_scriptcode, tx, in_index, hash_type, amount, sigversion, nullptr, nullptr)}; + BOOST_CHECK_EQUAL(sighash_with_cache2, sighash_no_cache2); + if (!expect_one) { + BOOST_CHECK_NE(sighash_with_cache, sighash_with_cache2); + } else { + BOOST_CHECK_EQUAL(sighash_with_cache, sighash_with_cache2); + BOOST_CHECK_EQUAL(sighash_with_cache, uint256::ONE); + } + + // Calling the cached version again should return the same value again. + BOOST_CHECK_EQUAL(sighash_with_cache2, SignatureHash(diff_scriptcode, tx, in_index, hash_type, amount, sigversion, nullptr, &cache)); + + // And if we store a different value for this scriptcode and hash type it will return that instead. + { + HashWriter h{}; + h << 42; + cache.Store(hash_type, scriptcode, h); + const auto stored_hash{h.GetHash()}; + BOOST_CHECK(cache.Load(hash_type, scriptcode, h)); + const auto loaded_hash{h.GetHash()}; + BOOST_CHECK_EQUAL(stored_hash, loaded_hash); + } + + // And using this mutated cache with the sighash function will return the new value (except in the legacy SIGHASH_SINGLE bug + // case in which it'll return 1). + if (!expect_one) { + BOOST_CHECK_NE(SignatureHash(scriptcode, tx, in_index, hash_type, amount, sigversion, nullptr, &cache), sighash_with_cache); + HashWriter h{}; + BOOST_CHECK(cache.Load(hash_type, scriptcode, h)); + h << hash_type; + const auto new_hash{h.GetHash()}; + BOOST_CHECK_EQUAL(SignatureHash(scriptcode, tx, in_index, hash_type, amount, sigversion, nullptr, &cache), new_hash); + } else { + BOOST_CHECK_EQUAL(SignatureHash(scriptcode, tx, in_index, hash_type, amount, sigversion, nullptr, &cache), uint256::ONE); + } + + // Wipe the cache and restore the correct cached value for this scriptcode and hash_type before starting the next iteration. + HashWriter dummy{}; + cache.Store(hash_type, diff_scriptcode, dummy); + (void)SignatureHash(scriptcode, tx, in_index, hash_type, amount, sigversion, nullptr, &cache); + BOOST_CHECK(cache.Load(hash_type, scriptcode, dummy) || expect_one); + } + } +} + BOOST_AUTO_TEST_SUITE_END() From 65bcbbc538234957b1f7f76b2f21ad7c138efb87 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Wed, 23 Jul 2025 10:50:33 +1000 Subject: [PATCH 09/11] net_processing: drop MaybePunishNodeForTx Do not discourage nodes even when they send us consensus invalid transactions. Because we do not discourage nodes for transactions we consider non-standard, we don't get any DoS protection from this check in adversarial scenarios, so remove the check entirely both to simplify the code and reduce the risk of splitting the network due to changes in tx relay policy. NOTE: Backport required additional adjustment in test/functional/p2p_invalid_tx Github-Pull: #33050 Rebased-From: 266dd0e10d08c0bfde63205db15d6c210a021b90 --- src/net_processing.cpp | 34 ----------------------- test/functional/data/invalid_txs.py | 20 ++++++------- test/functional/p2p_invalid_tx.py | 5 ++-- test/functional/p2p_opportunistic_1p1c.py | 6 ++-- 4 files changed, 16 insertions(+), 49 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 1da3ec9d211..b25819c821b 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -553,12 +553,6 @@ private: bool via_compact_block, const std::string& message = "") EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); - /** - * Potentially disconnect and discourage a node based on the contents of a TxValidationState object - */ - void MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state) - EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex); - /** Maybe disconnect a peer and discourage future connections from its address. * * @param[in] pnode The node to check. @@ -1805,32 +1799,6 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati } } -void PeerManagerImpl::MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state) -{ - PeerRef peer{GetPeerRef(nodeid)}; - switch (state.GetResult()) { - case TxValidationResult::TX_RESULT_UNSET: - break; - // The node is providing invalid data: - case TxValidationResult::TX_CONSENSUS: - if (peer) Misbehaving(*peer, ""); - return; - // Conflicting (but not necessarily invalid) data or different policy: - case TxValidationResult::TX_INPUTS_NOT_STANDARD: - case TxValidationResult::TX_NOT_STANDARD: - case TxValidationResult::TX_MISSING_INPUTS: - case TxValidationResult::TX_PREMATURE_SPEND: - case TxValidationResult::TX_WITNESS_MUTATED: - case TxValidationResult::TX_WITNESS_STRIPPED: - case TxValidationResult::TX_CONFLICT: - case TxValidationResult::TX_MEMPOOL_POLICY: - case TxValidationResult::TX_NO_MEMPOOL: - case TxValidationResult::TX_RECONSIDERABLE: - case TxValidationResult::TX_UNKNOWN: - break; - } -} - bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex) { AssertLockHeld(cs_main); @@ -2987,8 +2955,6 @@ std::optional PeerManagerImpl::ProcessInvalidTx(NodeId if (peer) AddKnownTx(*peer, parent_txid); } - MaybePunishNodeForTx(nodeid, state); - return package_to_validate; } diff --git a/test/functional/data/invalid_txs.py b/test/functional/data/invalid_txs.py index d2d7202d860..48ec88fde0d 100644 --- a/test/functional/data/invalid_txs.py +++ b/test/functional/data/invalid_txs.py @@ -89,7 +89,7 @@ class BadTxTemplate: class OutputMissing(BadTxTemplate): reject_reason = "bad-txns-vout-empty" - expect_disconnect = True + expect_disconnect = False def get_tx(self): tx = CTransaction() @@ -100,7 +100,7 @@ class OutputMissing(BadTxTemplate): class InputMissing(BadTxTemplate): reject_reason = "bad-txns-vin-empty" - expect_disconnect = True + expect_disconnect = False # We use a blank transaction here to make sure # it is interpreted as a non-witness transaction. @@ -149,7 +149,7 @@ class BadInputOutpointIndex(BadTxTemplate): class DuplicateInput(BadTxTemplate): reject_reason = 'bad-txns-inputs-duplicate' - expect_disconnect = True + expect_disconnect = False def get_tx(self): tx = CTransaction() @@ -162,7 +162,7 @@ class DuplicateInput(BadTxTemplate): class PrevoutNullInput(BadTxTemplate): reject_reason = 'bad-txns-prevout-null' - expect_disconnect = True + expect_disconnect = False def get_tx(self): tx = CTransaction() @@ -188,7 +188,7 @@ class NonexistentInput(BadTxTemplate): class SpendTooMuch(BadTxTemplate): reject_reason = 'bad-txns-in-belowout' - expect_disconnect = True + expect_disconnect = False def get_tx(self): return create_tx_with_script( @@ -197,7 +197,7 @@ class SpendTooMuch(BadTxTemplate): class CreateNegative(BadTxTemplate): reject_reason = 'bad-txns-vout-negative' - expect_disconnect = True + expect_disconnect = False def get_tx(self): return create_tx_with_script(self.spend_tx, 0, amount=-1) @@ -205,7 +205,7 @@ class CreateNegative(BadTxTemplate): class CreateTooLarge(BadTxTemplate): reject_reason = 'bad-txns-vout-toolarge' - expect_disconnect = True + expect_disconnect = False def get_tx(self): return create_tx_with_script(self.spend_tx, 0, amount=MAX_MONEY + 1) @@ -213,7 +213,7 @@ class CreateTooLarge(BadTxTemplate): class CreateSumTooLarge(BadTxTemplate): reject_reason = 'bad-txns-txouttotal-toolarge' - expect_disconnect = True + expect_disconnect = False def get_tx(self): tx = create_tx_with_script(self.spend_tx, 0, amount=MAX_MONEY) @@ -224,7 +224,7 @@ class CreateSumTooLarge(BadTxTemplate): class InvalidOPIFConstruction(BadTxTemplate): reject_reason = "mandatory-script-verify-flag-failed (Invalid OP_IF construction)" - expect_disconnect = True + expect_disconnect = False valid_in_block = True def get_tx(self): @@ -266,7 +266,7 @@ def getDisabledOpcodeTemplate(opcode): class NonStandardAndInvalid(BadTxTemplate): """A non-standard transaction which is also consensus-invalid should return the consensus error.""" reject_reason = "mandatory-script-verify-flag-failed (OP_RETURN was encountered)" - expect_disconnect = True + expect_disconnect = False valid_in_block = False def get_tx(self): diff --git a/test/functional/p2p_invalid_tx.py b/test/functional/p2p_invalid_tx.py index ee8c6c16ca3..3785f725fef 100755 --- a/test/functional/p2p_invalid_tx.py +++ b/test/functional/p2p_invalid_tx.py @@ -73,7 +73,7 @@ class InvalidTxRequestTest(BitcoinTestFramework): tx = template.get_tx() node.p2ps[0].send_txs_and_test( [tx], node, success=False, - expect_disconnect=template.expect_disconnect, + expect_disconnect=False, reject_reason=template.reject_reason, ) @@ -144,7 +144,6 @@ class InvalidTxRequestTest(BitcoinTestFramework): # tx_orphan_2_no_fee, because it has too low fee (p2ps[0] is not disconnected for relaying that tx) # tx_orphan_2_invalid, because it has negative fee (p2ps[1] is disconnected for relaying that tx) - self.wait_until(lambda: 1 == len(node.getpeerinfo()), timeout=12) # p2ps[1] is no longer connected assert_equal(expected_mempool, set(node.getrawmempool())) self.log.info('Test orphan pool overflow') @@ -165,7 +164,7 @@ class InvalidTxRequestTest(BitcoinTestFramework): node.p2ps[0].send_txs_and_test([rejected_parent], node, success=False) self.log.info('Test that a peer disconnection causes erase its transactions from the orphan pool') - with node.assert_debug_log(['Erased 100 orphan transaction(s) from peer=26']): + with node.assert_debug_log(['Erased 100 orphan transaction(s) from peer=']): self.reconnect_p2p(num_connections=1) self.log.info('Test that a transaction in the orphan pool is included in a new tip block causes erase this transaction from the orphan pool') diff --git a/test/functional/p2p_opportunistic_1p1c.py b/test/functional/p2p_opportunistic_1p1c.py index 5fdbf74a573..def70b733a6 100755 --- a/test/functional/p2p_opportunistic_1p1c.py +++ b/test/functional/p2p_opportunistic_1p1c.py @@ -251,8 +251,10 @@ class PackageRelayTest(BitcoinTestFramework): assert tx_orphan_bad_wit.rehash() not in node_mempool # 5. Have the other peer send the tx too, so that tx_orphan_bad_wit package is attempted. - bad_orphan_sender.send_message(msg_tx(low_fee_parent["tx"])) - bad_orphan_sender.wait_for_disconnect() + bad_orphan_sender.send_and_ping(msg_tx(low_fee_parent["tx"])) + + # The bad orphan sender should not be disconnected. + bad_orphan_sender.sync_with_ping() # The peer that didn't provide the orphan should not be disconnected. parent_sender.sync_with_ping() From be0857745a5a0154d89a2aa9ddaa2a84e912598a Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Wed, 23 Jul 2025 10:51:06 +1000 Subject: [PATCH 10/11] validation: only check input scripts once Previously, we would check failing input scripts twice when considering a transaction for the mempool, in order to distinguish policy failures from consensus failures. This allowed us both to provide a different error message and to discourage peers for consensus failures. Because we are no longer discouraging peers for consensus failures during tx relay, and because checking a script can be expensive, only do this once. Also renames non-mandatory-script-verify-flag error to mempool-script-verify-flag-failed. NOTE: Backport required additional adjustment in test/functional/feature_block Github-Pull: #33050 Rebased-From: b29ae9efdfeeff774e32ee433ce67d8ed8ecd49f --- src/validation.cpp | 35 +++++++--------------------- test/functional/data/invalid_txs.py | 7 +++--- test/functional/feature_block.py | 5 +++- test/functional/feature_cltv.py | 18 +++++++------- test/functional/feature_dersig.py | 4 ++-- test/functional/feature_nulldummy.py | 12 +++++----- test/functional/feature_segwit.py | 24 +++++++++---------- test/functional/mempool_accept.py | 2 +- test/functional/p2p_segwit.py | 14 +++++------ test/functional/rpc_packages.py | 4 ++-- 10 files changed, 57 insertions(+), 68 deletions(-) diff --git a/src/validation.cpp b/src/validation.cpp index 36734bc6122..ebeb67ac78a 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -2207,34 +2207,17 @@ 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 (flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) { - // Check whether the failure was caused by a - // non-mandatory script verification check, such as - // non-standard DER encodings or non-null dummy - // arguments; if so, ensure we return NOT_STANDARD - // instead of CONSENSUS to avoid downstream users - // splitting the network between upgraded and - // non-upgraded nodes by banning CONSENSUS-failing - // data providers. - CScriptCheck check2(txdata.m_spent_outputs[i], tx, validation_cache.m_signature_cache, i, - flags & ~STANDARD_NOT_MANDATORY_VERIFY_FLAGS, cacheSigStore, &txdata); - auto mandatory_result = check2(); - if (!mandatory_result.has_value()) { - return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(result->first)), result->second); - } else { - // If the second check failed, it failed due to a mandatory script verification - // flag, but the first check might have failed on a non-mandatory script - // verification flag. - // - // Avoid reporting a mandatory script check failure with a non-mandatory error - // string by reporting the error from the second check. - result = mandatory_result; - } + 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("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(result->first)), result->second); } - - // MANDATORY flag failures correspond to - // TxValidationResult::TX_CONSENSUS. - return state.Invalid(TxValidationResult::TX_CONSENSUS, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(result->first)), result->second); } } diff --git a/test/functional/data/invalid_txs.py b/test/functional/data/invalid_txs.py index 48ec88fde0d..bb1931be2df 100644 --- a/test/functional/data/invalid_txs.py +++ b/test/functional/data/invalid_txs.py @@ -223,7 +223,7 @@ class CreateSumTooLarge(BadTxTemplate): class InvalidOPIFConstruction(BadTxTemplate): - reject_reason = "mandatory-script-verify-flag-failed (Invalid OP_IF construction)" + reject_reason = "mempool-script-verify-flag-failed (Invalid OP_IF construction)" expect_disconnect = False valid_in_block = True @@ -264,8 +264,9 @@ def getDisabledOpcodeTemplate(opcode): }) class NonStandardAndInvalid(BadTxTemplate): - """A non-standard transaction which is also consensus-invalid should return the consensus error.""" - reject_reason = "mandatory-script-verify-flag-failed (OP_RETURN was encountered)" + """A non-standard transaction which is also consensus-invalid should return the first error.""" + reject_reason = "mempool-script-verify-flag-failed (Using OP_CODESEPARATOR in non-witness script)" + block_reject_reason = "mandatory-script-verify-flag-failed (OP_RETURN was encountered)" expect_disconnect = False valid_in_block = False diff --git a/test/functional/feature_block.py b/test/functional/feature_block.py index 2dfa568c5b6..222b2387853 100755 --- a/test/functional/feature_block.py +++ b/test/functional/feature_block.py @@ -164,9 +164,12 @@ class FullBlockTest(BitcoinTestFramework): self.sign_tx(badtx, attempt_spend_tx) badtx.rehash() badblock = self.update_block(blockname, [badtx]) + reject_reason = (template.block_reject_reason or template.reject_reason) + if reject_reason and reject_reason.startswith("mempool-script-verify-flag-failed"): + reject_reason = "mandatory-script-verify-flag-failed" + reject_reason[33:] self.send_blocks( [badblock], success=False, - reject_reason=(template.block_reject_reason or template.reject_reason), + reject_reason=reject_reason, reconnect=True, timeout=2) self.move_tip(2) diff --git a/test/functional/feature_cltv.py b/test/functional/feature_cltv.py index 60b3fb4e20b..81cc10a5adf 100755 --- a/test/functional/feature_cltv.py +++ b/test/functional/feature_cltv.py @@ -154,12 +154,14 @@ class BIP65Test(BitcoinTestFramework): coin_vout = coin.prevout.n cltv_invalidate(spendtx, i) + blk_rej = "mandatory-script-verify-flag-failed" + tx_rej = "mempool-script-verify-flag-failed" expected_cltv_reject_reason = [ - "mandatory-script-verify-flag-failed (Operation not valid with the current stack size)", - "mandatory-script-verify-flag-failed (Negative locktime)", - "mandatory-script-verify-flag-failed (Locktime requirement not satisfied)", - "mandatory-script-verify-flag-failed (Locktime requirement not satisfied)", - "mandatory-script-verify-flag-failed (Locktime requirement not satisfied)", + " (Operation not valid with the current stack size)", + " (Negative locktime)", + " (Locktime requirement not satisfied)", + " (Locktime requirement not satisfied)", + " (Locktime requirement not satisfied)", ][i] # First we show that this tx is valid except for CLTV by getting it # rejected from the mempool for exactly that reason. @@ -170,8 +172,8 @@ class BIP65Test(BitcoinTestFramework): 'txid': spendtx_txid, 'wtxid': spendtx_wtxid, 'allowed': False, - 'reject-reason': expected_cltv_reject_reason, - 'reject-details': expected_cltv_reject_reason + f", input 0 of {spendtx_txid} (wtxid {spendtx_wtxid}), spending {coin_txid}:{coin_vout}" + 'reject-reason': tx_rej + expected_cltv_reject_reason, + 'reject-details': tx_rej + expected_cltv_reject_reason + f", input 0 of {spendtx_txid} (wtxid {spendtx_wtxid}), spending {coin_txid}:{coin_vout}" }], self.nodes[0].testmempoolaccept(rawtxs=[spendtx.serialize().hex()], maxfeerate=0), ) @@ -181,7 +183,7 @@ class BIP65Test(BitcoinTestFramework): block.hashMerkleRoot = block.calc_merkle_root() block.solve() - with self.nodes[0].assert_debug_log(expected_msgs=[f'Block validation error: {expected_cltv_reject_reason}']): + with self.nodes[0].assert_debug_log(expected_msgs=[f'Block validation error: {blk_rej + expected_cltv_reject_reason}']): peer.send_and_ping(msg_block(block)) assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip) peer.sync_with_ping() diff --git a/test/functional/feature_dersig.py b/test/functional/feature_dersig.py index 0c3b0f12243..2a7eb0d0f47 100755 --- a/test/functional/feature_dersig.py +++ b/test/functional/feature_dersig.py @@ -123,8 +123,8 @@ class BIP66Test(BitcoinTestFramework): 'txid': spendtx_txid, 'wtxid': spendtx_wtxid, 'allowed': False, - 'reject-reason': 'mandatory-script-verify-flag-failed (Non-canonical DER signature)', - 'reject-details': 'mandatory-script-verify-flag-failed (Non-canonical DER signature), ' + + 'reject-reason': 'mempool-script-verify-flag-failed (Non-canonical DER signature)', + 'reject-details': 'mempool-script-verify-flag-failed (Non-canonical DER signature), ' + f"input 0 of {spendtx_txid} (wtxid {spendtx_wtxid}), spending {coin_txid}:0" }], self.nodes[0].testmempoolaccept(rawtxs=[spendtx.serialize().hex()], maxfeerate=0), diff --git a/test/functional/feature_nulldummy.py b/test/functional/feature_nulldummy.py index 885bc4855b0..e7fe7d65e48 100755 --- a/test/functional/feature_nulldummy.py +++ b/test/functional/feature_nulldummy.py @@ -37,8 +37,8 @@ from test_framework.util import ( from test_framework.wallet import getnewdestination from test_framework.wallet_util import generate_keypair -NULLDUMMY_ERROR = "mandatory-script-verify-flag-failed (Dummy CHECKMULTISIG argument must be zero)" - +NULLDUMMY_TX_ERROR = "mempool-script-verify-flag-failed (Dummy CHECKMULTISIG argument must be zero)" +NULLDUMMY_BLK_ERROR = "mandatory-script-verify-flag-failed (Dummy CHECKMULTISIG argument must be zero)" def invalidate_nulldummy_tx(tx): """Transform a NULLDUMMY compliant tx (i.e. scriptSig starts with OP_0) @@ -105,7 +105,7 @@ class NULLDUMMYTest(BitcoinTestFramework): addr=self.ms_address, amount=47, privkey=self.privkey) invalidate_nulldummy_tx(test2tx) - assert_raises_rpc_error(-26, NULLDUMMY_ERROR, self.nodes[0].sendrawtransaction, test2tx.serialize_with_witness().hex(), 0) + assert_raises_rpc_error(-26, NULLDUMMY_TX_ERROR, self.nodes[0].sendrawtransaction, test2tx.serialize_with_witness().hex(), 0) self.log.info(f"Test 3: Non-NULLDUMMY base transactions should be accepted in a block before activation [{COINBASE_MATURITY + 4}]") self.block_submit(self.nodes[0], [test2tx], accept=True) @@ -116,7 +116,7 @@ class NULLDUMMYTest(BitcoinTestFramework): privkey=self.privkey) test6txs = [CTransaction(test4tx)] invalidate_nulldummy_tx(test4tx) - assert_raises_rpc_error(-26, NULLDUMMY_ERROR, self.nodes[0].sendrawtransaction, test4tx.serialize_with_witness().hex(), 0) + assert_raises_rpc_error(-26, NULLDUMMY_TX_ERROR, self.nodes[0].sendrawtransaction, test4tx.serialize_with_witness().hex(), 0) self.block_submit(self.nodes[0], [test4tx], accept=False) self.log.info("Test 5: Non-NULLDUMMY P2WSH multisig transaction invalid after activation") @@ -126,7 +126,7 @@ class NULLDUMMYTest(BitcoinTestFramework): privkey=self.privkey) test6txs.append(CTransaction(test5tx)) test5tx.wit.vtxinwit[0].scriptWitness.stack[0] = b'\x01' - assert_raises_rpc_error(-26, NULLDUMMY_ERROR, self.nodes[0].sendrawtransaction, test5tx.serialize_with_witness().hex(), 0) + assert_raises_rpc_error(-26, NULLDUMMY_TX_ERROR, self.nodes[0].sendrawtransaction, test5tx.serialize_with_witness().hex(), 0) self.block_submit(self.nodes[0], [test5tx], with_witness=True, accept=False) self.log.info(f"Test 6: NULLDUMMY compliant base/witness transactions should be accepted to mempool and in block after activation [{COINBASE_MATURITY + 5}]") @@ -142,7 +142,7 @@ class NULLDUMMYTest(BitcoinTestFramework): if with_witness: add_witness_commitment(block) block.solve() - assert_equal(None if accept else NULLDUMMY_ERROR, node.submitblock(block.serialize().hex())) + assert_equal(None if accept else NULLDUMMY_BLK_ERROR, node.submitblock(block.serialize().hex())) if accept: assert_equal(node.getbestblockhash(), block.hash) self.lastblockhash = block.hash diff --git a/test/functional/feature_segwit.py b/test/functional/feature_segwit.py index f98f326e8f4..cc664a83aa3 100755 --- a/test/functional/feature_segwit.py +++ b/test/functional/feature_segwit.py @@ -193,8 +193,8 @@ class SegWitTest(BitcoinTestFramework): assert_equal(self.nodes[2].getbalance(), 20 * Decimal("49.999")) self.log.info("Verify unsigned p2sh witness txs without a redeem script are invalid") - self.fail_accept(self.nodes[2], "mandatory-script-verify-flag-failed (Operation not valid with the current stack size)", p2sh_ids[NODE_2][P2WPKH][1], sign=False) - self.fail_accept(self.nodes[2], "mandatory-script-verify-flag-failed (Operation not valid with the current stack size)", p2sh_ids[NODE_2][P2WSH][1], sign=False) + self.fail_accept(self.nodes[2], "mempool-script-verify-flag-failed (Operation not valid with the current stack size)", p2sh_ids[NODE_2][P2WPKH][1], sign=False) + self.fail_accept(self.nodes[2], "mempool-script-verify-flag-failed (Operation not valid with the current stack size)", p2sh_ids[NODE_2][P2WSH][1], sign=False) self.generate(self.nodes[0], 1) # block 164 @@ -213,13 +213,13 @@ class SegWitTest(BitcoinTestFramework): self.log.info("Verify default node can't accept txs with missing witness") # unsigned, no scriptsig - self.fail_accept(self.nodes[0], "mandatory-script-verify-flag-failed (Witness program hash mismatch)", wit_ids[NODE_0][P2WPKH][0], sign=False) - self.fail_accept(self.nodes[0], "mandatory-script-verify-flag-failed (Witness program was passed an empty witness)", wit_ids[NODE_0][P2WSH][0], sign=False) - self.fail_accept(self.nodes[0], "mandatory-script-verify-flag-failed (Operation not valid with the current stack size)", p2sh_ids[NODE_0][P2WPKH][0], sign=False) - self.fail_accept(self.nodes[0], "mandatory-script-verify-flag-failed (Operation not valid with the current stack size)", p2sh_ids[NODE_0][P2WSH][0], sign=False) + self.fail_accept(self.nodes[0], "mempool-script-verify-flag-failed (Witness program hash mismatch)", wit_ids[NODE_0][P2WPKH][0], sign=False) + self.fail_accept(self.nodes[0], "mempool-script-verify-flag-failed (Witness program was passed an empty witness)", wit_ids[NODE_0][P2WSH][0], sign=False) + self.fail_accept(self.nodes[0], "mempool-script-verify-flag-failed (Operation not valid with the current stack size)", p2sh_ids[NODE_0][P2WPKH][0], sign=False) + self.fail_accept(self.nodes[0], "mempool-script-verify-flag-failed (Operation not valid with the current stack size)", p2sh_ids[NODE_0][P2WSH][0], sign=False) # unsigned with redeem script - self.fail_accept(self.nodes[0], "mandatory-script-verify-flag-failed (Witness program hash mismatch)", p2sh_ids[NODE_0][P2WPKH][0], sign=False, redeem_script=witness_script(False, self.pubkey[0])) - self.fail_accept(self.nodes[0], "mandatory-script-verify-flag-failed (Witness program was passed an empty witness)", p2sh_ids[NODE_0][P2WSH][0], sign=False, redeem_script=witness_script(True, self.pubkey[0])) + self.fail_accept(self.nodes[0], "mempool-script-verify-flag-failed (Witness program hash mismatch)", p2sh_ids[NODE_0][P2WPKH][0], sign=False, redeem_script=witness_script(False, self.pubkey[0])) + self.fail_accept(self.nodes[0], "mempool-script-verify-flag-failed (Witness program was passed an empty witness)", p2sh_ids[NODE_0][P2WSH][0], sign=False, redeem_script=witness_script(True, self.pubkey[0])) # Coinbase contains the witness commitment nonce, check that RPC shows us coinbase_txid = self.nodes[2].getblock(blockhash)['tx'][0] @@ -230,10 +230,10 @@ class SegWitTest(BitcoinTestFramework): assert_equal(witnesses[0], '00' * 32) self.log.info("Verify witness txs without witness data are invalid after the fork") - self.fail_accept(self.nodes[2], 'mandatory-script-verify-flag-failed (Witness program hash mismatch)', wit_ids[NODE_2][P2WPKH][2], sign=False) - self.fail_accept(self.nodes[2], 'mandatory-script-verify-flag-failed (Witness program was passed an empty witness)', wit_ids[NODE_2][P2WSH][2], sign=False) - self.fail_accept(self.nodes[2], 'mandatory-script-verify-flag-failed (Witness program hash mismatch)', p2sh_ids[NODE_2][P2WPKH][2], sign=False, redeem_script=witness_script(False, self.pubkey[2])) - self.fail_accept(self.nodes[2], 'mandatory-script-verify-flag-failed (Witness program was passed an empty witness)', p2sh_ids[NODE_2][P2WSH][2], sign=False, redeem_script=witness_script(True, self.pubkey[2])) + self.fail_accept(self.nodes[2], 'mempool-script-verify-flag-failed (Witness program hash mismatch)', wit_ids[NODE_2][P2WPKH][2], sign=False) + self.fail_accept(self.nodes[2], 'mempool-script-verify-flag-failed (Witness program was passed an empty witness)', wit_ids[NODE_2][P2WSH][2], sign=False) + self.fail_accept(self.nodes[2], 'mempool-script-verify-flag-failed (Witness program hash mismatch)', p2sh_ids[NODE_2][P2WPKH][2], sign=False, redeem_script=witness_script(False, self.pubkey[2])) + self.fail_accept(self.nodes[2], 'mempool-script-verify-flag-failed (Witness program was passed an empty witness)', p2sh_ids[NODE_2][P2WSH][2], sign=False, redeem_script=witness_script(True, self.pubkey[2])) self.log.info("Verify default node can now use witness txs") self.success_mine(self.nodes[0], wit_ids[NODE_0][P2WPKH][0], True) diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py index 2155b8de6b1..32d8f7f6eac 100755 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -441,7 +441,7 @@ class MempoolAcceptanceTest(BitcoinTestFramework): nested_anchor_spend.rehash() self.check_mempool_result( - result_expected=[{'txid': nested_anchor_spend.rehash(), 'allowed': False, 'reject-reason': 'non-mandatory-script-verify-flag (Witness version reserved for soft-fork upgrades)'}], + result_expected=[{'txid': nested_anchor_spend.rehash(), 'allowed': False, 'reject-reason': 'mempool-script-verify-flag-failed (Witness version reserved for soft-fork upgrades)'}], rawtxs=[nested_anchor_spend.serialize().hex()], maxfeerate=0, ) diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py index e8f7f7e0f4e..7815d6ea84e 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -704,20 +704,20 @@ class SegWitTest(BitcoinTestFramework): # segwit activation. Note that older bitcoind's that are not # segwit-aware would also reject this for failing CLEANSTACK. with self.nodes[0].assert_debug_log( - expected_msgs=[spend_tx.hash, 'was not accepted: mandatory-script-verify-flag-failed (Witness program was passed an empty witness)']): + expected_msgs=[spend_tx.hash, 'was not accepted: mempool-script-verify-flag-failed (Witness program was passed an empty witness)']): test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False) # The transaction was detected as witness stripped above and not added to the reject # filter. Trying again will check it again and result in the same error. with self.nodes[0].assert_debug_log( - expected_msgs=[spend_tx.hash, 'was not accepted: mandatory-script-verify-flag-failed (Witness program was passed an empty witness)']): + expected_msgs=[spend_tx.hash, 'was not accepted: mempool-script-verify-flag-failed (Witness program was passed an empty witness)']): test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False) # Try to put the witness script in the scriptSig, should also fail. spend_tx.vin[0].scriptSig = CScript([p2wsh_pubkey, b'a']) spend_tx.rehash() with self.nodes[0].assert_debug_log( - expected_msgs=[spend_tx.hash, 'was not accepted: mandatory-script-verify-flag-failed (Script evaluated without error but finished with a false/empty top stack element)']): + expected_msgs=[spend_tx.hash, 'was not accepted: mempool-script-verify-flag-failed (Script evaluated without error but finished with a false/empty top stack element)']): test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False) # Now put the witness script in the witness, should succeed after @@ -1291,7 +1291,7 @@ class SegWitTest(BitcoinTestFramework): # Now do the opposite: strip the witness entirely. This will be detected as witness stripping and # the (w)txid won't be added to the reject filter: we can try again and get the same error. tx3.wit.vtxinwit[0].scriptWitness.stack = [] - reason = "was not accepted: mandatory-script-verify-flag-failed (Witness program was passed an empty witness)" + reason = "was not accepted: mempool-script-verify-flag-failed (Witness program was passed an empty witness)" test_transaction_acceptance(self.nodes[0], self.test_node, tx3, with_witness=False, accepted=False, reason=reason) test_transaction_acceptance(self.nodes[0], self.test_node, tx3, with_witness=False, accepted=False, reason=reason) @@ -1490,7 +1490,7 @@ class SegWitTest(BitcoinTestFramework): sign_input_segwitv0(tx2, 0, script, tx.vout[0].nValue, key) # Should fail policy test. - test_transaction_acceptance(self.nodes[0], self.test_node, tx2, True, False, 'non-mandatory-script-verify-flag (Using non-compressed keys in segwit)') + test_transaction_acceptance(self.nodes[0], self.test_node, tx2, True, False, 'mempool-script-verify-flag-failed (Using non-compressed keys in segwit)') # But passes consensus. block = self.build_next_block() self.update_witness_block_with_transactions(block, [tx2]) @@ -1509,7 +1509,7 @@ class SegWitTest(BitcoinTestFramework): sign_p2pk_witness_input(witness_script, tx3, 0, SIGHASH_ALL, tx2.vout[0].nValue, key) # Should fail policy test. - test_transaction_acceptance(self.nodes[0], self.test_node, tx3, True, False, 'non-mandatory-script-verify-flag (Using non-compressed keys in segwit)') + test_transaction_acceptance(self.nodes[0], self.test_node, tx3, True, False, 'mempool-script-verify-flag-failed (Using non-compressed keys in segwit)') # But passes consensus. block = self.build_next_block() self.update_witness_block_with_transactions(block, [tx3]) @@ -1526,7 +1526,7 @@ class SegWitTest(BitcoinTestFramework): sign_p2pk_witness_input(witness_script, tx4, 0, SIGHASH_ALL, tx3.vout[0].nValue, key) # Should fail policy test. - test_transaction_acceptance(self.nodes[0], self.test_node, tx4, True, False, 'non-mandatory-script-verify-flag (Using non-compressed keys in segwit)') + test_transaction_acceptance(self.nodes[0], self.test_node, tx4, True, False, 'mempool-script-verify-flag-failed (Using non-compressed keys in segwit)') block = self.build_next_block() self.update_witness_block_with_transactions(block, [tx4]) test_witness_block(self.nodes[0], self.test_node, block, accepted=True) diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py index a2f9210f94d..539e9d09add 100755 --- a/test/functional/rpc_packages.py +++ b/test/functional/rpc_packages.py @@ -122,8 +122,8 @@ class RPCPackagesTest(BitcoinTestFramework): assert_equal(testres_bad_sig, self.independent_txns_testres + [{ "txid": tx_bad_sig_txid, "wtxid": tx_bad_sig_wtxid, "allowed": False, - "reject-reason": "mandatory-script-verify-flag-failed (Operation not valid with the current stack size)", - "reject-details": "mandatory-script-verify-flag-failed (Operation not valid with the current stack size), " + + "reject-reason": "mempool-script-verify-flag-failed (Operation not valid with the current stack size)", + "reject-details": "mempool-script-verify-flag-failed (Operation not valid with the current stack size), " + f"input 0 of {tx_bad_sig_txid} (wtxid {tx_bad_sig_wtxid}), spending {coin['txid']}:{coin['vout']}" }]) From 6f136cd3914b001752cce02adde00fccaed0ad48 Mon Sep 17 00:00:00 2001 From: Anthony Towns Date: Fri, 8 Aug 2025 23:15:17 +1000 Subject: [PATCH 11/11] tests: drop expect_disconnect behaviour for tx relay Github-Pull: #33050 Rebased-From: 876dbdfb4702410dfd4037614dc9298a0c09c63e --- test/functional/data/invalid_txs.py | 18 ------------------ test/functional/p2p_invalid_tx.py | 5 ----- test/functional/test_framework/p2p.py | 8 ++------ 3 files changed, 2 insertions(+), 29 deletions(-) diff --git a/test/functional/data/invalid_txs.py b/test/functional/data/invalid_txs.py index bb1931be2df..f96059d4ee8 100644 --- a/test/functional/data/invalid_txs.py +++ b/test/functional/data/invalid_txs.py @@ -69,9 +69,6 @@ class BadTxTemplate: # Only specified if it differs from mempool acceptance error. block_reject_reason = "" - # Do we expect to be disconnected after submitting this tx? - expect_disconnect = False - # Is this tx considered valid when included in a block, but not for acceptance into # the mempool (i.e. does it violate policy but not consensus)? valid_in_block = False @@ -89,7 +86,6 @@ class BadTxTemplate: class OutputMissing(BadTxTemplate): reject_reason = "bad-txns-vout-empty" - expect_disconnect = False def get_tx(self): tx = CTransaction() @@ -100,7 +96,6 @@ class OutputMissing(BadTxTemplate): class InputMissing(BadTxTemplate): reject_reason = "bad-txns-vin-empty" - expect_disconnect = False # We use a blank transaction here to make sure # it is interpreted as a non-witness transaction. @@ -117,7 +112,6 @@ class InputMissing(BadTxTemplate): # tree depth commitment (CVE-2017-12842) class SizeTooSmall(BadTxTemplate): reject_reason = "tx-size-small" - expect_disconnect = False valid_in_block = True def get_tx(self): @@ -134,7 +128,6 @@ class BadInputOutpointIndex(BadTxTemplate): # Won't be rejected - nonexistent outpoint index is treated as an orphan since the coins # database can't distinguish between spent outpoints and outpoints which never existed. reject_reason = None - expect_disconnect = False def get_tx(self): num_indices = len(self.spend_tx.vin) @@ -149,7 +142,6 @@ class BadInputOutpointIndex(BadTxTemplate): class DuplicateInput(BadTxTemplate): reject_reason = 'bad-txns-inputs-duplicate' - expect_disconnect = False def get_tx(self): tx = CTransaction() @@ -162,7 +154,6 @@ class DuplicateInput(BadTxTemplate): class PrevoutNullInput(BadTxTemplate): reject_reason = 'bad-txns-prevout-null' - expect_disconnect = False def get_tx(self): tx = CTransaction() @@ -175,7 +166,6 @@ class PrevoutNullInput(BadTxTemplate): class NonexistentInput(BadTxTemplate): reject_reason = None # Added as an orphan tx. - expect_disconnect = False def get_tx(self): tx = CTransaction() @@ -188,7 +178,6 @@ class NonexistentInput(BadTxTemplate): class SpendTooMuch(BadTxTemplate): reject_reason = 'bad-txns-in-belowout' - expect_disconnect = False def get_tx(self): return create_tx_with_script( @@ -197,7 +186,6 @@ class SpendTooMuch(BadTxTemplate): class CreateNegative(BadTxTemplate): reject_reason = 'bad-txns-vout-negative' - expect_disconnect = False def get_tx(self): return create_tx_with_script(self.spend_tx, 0, amount=-1) @@ -205,7 +193,6 @@ class CreateNegative(BadTxTemplate): class CreateTooLarge(BadTxTemplate): reject_reason = 'bad-txns-vout-toolarge' - expect_disconnect = False def get_tx(self): return create_tx_with_script(self.spend_tx, 0, amount=MAX_MONEY + 1) @@ -213,7 +200,6 @@ class CreateTooLarge(BadTxTemplate): class CreateSumTooLarge(BadTxTemplate): reject_reason = 'bad-txns-txouttotal-toolarge' - expect_disconnect = False def get_tx(self): tx = create_tx_with_script(self.spend_tx, 0, amount=MAX_MONEY) @@ -224,7 +210,6 @@ class CreateSumTooLarge(BadTxTemplate): class InvalidOPIFConstruction(BadTxTemplate): reject_reason = "mempool-script-verify-flag-failed (Invalid OP_IF construction)" - expect_disconnect = False valid_in_block = True def get_tx(self): @@ -236,7 +221,6 @@ class InvalidOPIFConstruction(BadTxTemplate): class TooManySigops(BadTxTemplate): reject_reason = "bad-txns-too-many-sigops" block_reject_reason = "bad-blk-sigops, out-of-bounds SigOpCount" - expect_disconnect = False def get_tx(self): lotsa_checksigs = CScript([OP_CHECKSIG] * (MAX_BLOCK_SIGOPS)) @@ -258,7 +242,6 @@ def getDisabledOpcodeTemplate(opcode): return type('DisabledOpcode_' + str(opcode), (BadTxTemplate,), { 'reject_reason': "disabled opcode", - 'expect_disconnect': True, 'get_tx': get_tx, 'valid_in_block' : True }) @@ -267,7 +250,6 @@ class NonStandardAndInvalid(BadTxTemplate): """A non-standard transaction which is also consensus-invalid should return the first error.""" reject_reason = "mempool-script-verify-flag-failed (Using OP_CODESEPARATOR in non-witness script)" block_reject_reason = "mandatory-script-verify-flag-failed (OP_RETURN was encountered)" - expect_disconnect = False valid_in_block = False def get_tx(self): diff --git a/test/functional/p2p_invalid_tx.py b/test/functional/p2p_invalid_tx.py index 3785f725fef..439735d178a 100755 --- a/test/functional/p2p_invalid_tx.py +++ b/test/functional/p2p_invalid_tx.py @@ -73,14 +73,9 @@ class InvalidTxRequestTest(BitcoinTestFramework): tx = template.get_tx() node.p2ps[0].send_txs_and_test( [tx], node, success=False, - expect_disconnect=False, reject_reason=template.reject_reason, ) - if template.expect_disconnect: - self.log.info("Reconnecting to peer") - self.reconnect_p2p() - # Make two p2p connections to provide the node with orphans # * p2ps[0] will send valid orphan txs (one with low fee) # * p2ps[1] will send an invalid orphan tx (and is later disconnected for that) diff --git a/test/functional/test_framework/p2p.py b/test/functional/test_framework/p2p.py index 207d19137b1..c5e518238ce 100755 --- a/test/functional/test_framework/p2p.py +++ b/test/functional/test_framework/p2p.py @@ -893,13 +893,12 @@ class P2PDataStore(P2PInterface): else: assert node.getbestblockhash() != blocks[-1].hash - def send_txs_and_test(self, txs, node, *, success=True, expect_disconnect=False, reject_reason=None): + def send_txs_and_test(self, txs, node, *, success=True, reject_reason=None): """Send txs to test node and test whether they're accepted to the mempool. - add all txs to our tx_store - send tx messages for all txs - if success is True/False: assert that the txs are/are not accepted to the mempool - - if expect_disconnect is True: Skip the sync with ping - if reject_reason is set: assert that the correct reject message is logged.""" with p2p_lock: @@ -911,10 +910,7 @@ class P2PDataStore(P2PInterface): for tx in txs: self.send_message(msg_tx(tx)) - if expect_disconnect: - self.wait_for_disconnect() - else: - self.sync_with_ping() + self.sync_with_ping() raw_mempool = node.getrawmempool() if success: