Make bad-witness-nonstandard rejection more specific, and support overriding some

luke-jr · luke-jr · commit 9a7339f6726e · 2025-01-23T03:01:51.000Z
diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp
@@ -244,7 +244,7 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs,
     return true;
 }
 
-bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
+bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs, const std::string& reason_prefix, std::string& out_reason, const ignore_rejects_type& ignore_rejects)
 {
     if (tx.IsCoinBase())
         return true; // Coinbases are skipped
@@ -273,9 +273,15 @@ bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
             // into a stack. We do not check IsPushOnly nor compare the hash as these will be done later anyway.
             // If the check fails at this stage, we know that this txid must be a bad one.
             if (!EvalScript(stack, tx.vin[i].scriptSig, SCRIPT_VERIFY_NONE, BaseSignatureChecker(), SigVersion::BASE))
+            {
+                out_reason = reason_prefix + "scriptsig-failure";
                 return false;
+            }
             if (stack.empty())
+            {
+                out_reason = reason_prefix + "scriptcheck-missing";
                 return false;
+            }
             prevScript = CScript(stack.back().begin(), stack.back().end());
             p2sh = true;
         }
@@ -285,18 +291,21 @@ bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
 
         // Non-witness program must not be associated with any witness
         if (!prevScript.IsWitnessProgram(witnessversion, witnessprogram))
+        {
+            out_reason = reason_prefix + "nonwitness-input";
             return false;
+        }
 
         // Check P2WSH standard limits
         if (witnessversion == 0 && witnessprogram.size() == WITNESS_V0_SCRIPTHASH_SIZE) {
             if (tx.vin[i].scriptWitness.stack.back().size() > MAX_STANDARD_P2WSH_SCRIPT_SIZE)
-                return false;
+                MaybeReject("script-size");
             size_t sizeWitnessStack = tx.vin[i].scriptWitness.stack.size() - 1;
             if (sizeWitnessStack > MAX_STANDARD_P2WSH_STACK_ITEMS)
-                return false;
+                MaybeReject("stackitem-count");
             for (unsigned int j = 0; j < sizeWitnessStack; j++) {
                 if (tx.vin[i].scriptWitness.stack[j].size() > MAX_STANDARD_P2WSH_STACK_ITEM_SIZE)
-                    return false;
+                    MaybeReject("stackitem-size");
             }
         }
 
@@ -308,24 +317,36 @@ bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs)
             Span stack{tx.vin[i].scriptWitness.stack};
             if (stack.size() >= 2 && !stack.back().empty() && stack.back()[0] == ANNEX_TAG) {
                 // Annexes are nonstandard as long as no semantics are defined for them.
-                return false;
+                MaybeReject("taproot-annex");
+                // If reject reason is ignored, continue as if the annex wasn't there.
+                SpanPopBack(stack);
             }
             if (stack.size() >= 2) {
                 // Script path spend (2 or more stack elements after removing optional annex)
                 const auto& control_block = SpanPopBack(stack);
                 SpanPopBack(stack); // Ignore script
-                if (control_block.empty()) return false; // Empty control block is invalid
+                if (control_block.empty()) {
+                    // Empty control block is invalid
+                    out_reason = reason_prefix + "taproot-control-missing";
+                    return false;
+                }
                 if ((control_block[0] & TAPROOT_LEAF_MASK) == TAPROOT_LEAF_TAPSCRIPT) {
                     // Leaf version 0xc0 (aka Tapscript, see BIP 342)
+                    if (!ignore_rejects.count(reason_prefix + "taproot-stackitem-size")) {
                     for (const auto& item : stack) {
-                        if (item.size() > MAX_STANDARD_TAPSCRIPT_STACK_ITEM_SIZE) return false;
+                            if (item.size() > MAX_STANDARD_TAPSCRIPT_STACK_ITEM_SIZE) {
+                                out_reason = reason_prefix + "taproot-stackitem-size";
+                                return false;
+                            }
+                        }
                     }
                 }
             } else if (stack.size() == 1) {
                 // Key path spend (1 stack element after removing optional annex)
                 // (no policy rules apply)
             } else {
                 // 0 stack elements; this is already invalid by consensus rules
+                out_reason = reason_prefix + "taproot-witness-missing";
                 return false;
             }
         }
diff --git a/src/policy/policy.h b/src/policy/policy.h
@@ -161,7 +161,7 @@ inline bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& map
 *
 * Also enforce a maximum stack item size limit and no annexes for tapscript spends.
 */
-bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs);
+bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs, const std::string& reason_prefix, std::string& out_reason, const ignore_rejects_type& ignore_rejects=empty_ignore_rejects);
 
 /** 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/fuzz/coins_view.cpp b/src/test/fuzz/coins_view.cpp
@@ -284,7 +284,8 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view)
                 (void)GetTransactionSigOpCost(transaction, coins_view_cache, flags);
             },
             [&] {
-                (void)IsWitnessStandard(CTransaction{random_mutable_transaction}, coins_view_cache);
+                std::string reason;
+                (void)IsWitnessStandard(CTransaction{random_mutable_transaction}, coins_view_cache, "bad-witness-", reason);
             });
     }
 }
diff --git a/src/test/fuzz/transaction.cpp b/src/test/fuzz/transaction.cpp
@@ -88,7 +88,8 @@ FUZZ_TARGET(transaction, .init = initialize_transaction)
     CCoinsView coins_view;
     const CCoinsViewCache coins_view_cache(&coins_view);
     (void)AreInputsStandard(tx, coins_view_cache);
-    (void)IsWitnessStandard(tx, coins_view_cache);
+    std::string reject_reason;
+    (void)IsWitnessStandard(tx, coins_view_cache, "fuzz", reject_reason);
 
     if (tx.GetTotalSize() < 250'000) { // Avoid high memory usage (with msan) due to json encoding
         {
diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp
@@ -426,7 +426,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup)
             auto it_parent = result_quit_early.m_tx_results.find(tx_parent_invalid->GetWitnessHash());
             auto it_child = result_quit_early.m_tx_results.find(tx_child->GetWitnessHash());
             BOOST_CHECK_EQUAL(it_parent->second.m_state.GetResult(), TxValidationResult::TX_WITNESS_MUTATED);
-            BOOST_CHECK_EQUAL(it_parent->second.m_state.GetRejectReason(), "bad-witness-nonstandard");
+            BOOST_CHECK_EQUAL(it_parent->second.m_state.GetRejectReason(), "bad-witness-nonwitness-input");
             BOOST_CHECK_EQUAL(it_child->second.m_state.GetResult(), TxValidationResult::TX_MISSING_INPUTS);
             BOOST_CHECK_EQUAL(it_child->second.m_state.GetRejectReason(), "bad-txns-inputs-missingorspent");
         }
diff --git a/src/validation.cpp b/src/validation.cpp
@@ -924,8 +924,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
     }
 
     // Check for non-standard witnesses.
-    if (tx.HasWitness() && m_pool.m_opts.require_standard && !IsWitnessStandard(tx, m_view)) {
-        MaybeReject(TxValidationResult::TX_WITNESS_MUTATED, "bad-witness-nonstandard");
+    if (tx.HasWitness() && m_pool.m_opts.require_standard && !IsWitnessStandard(tx, m_view, "bad-witness-", reason, ignore_rejects)) {
+        return state.Invalid(TxValidationResult::TX_WITNESS_MUTATED, reason);
     }
 
     int64_t nSigOpsCost = GetTransactionSigOpCost(tx, m_view, STANDARD_SCRIPT_VERIFY_FLAGS);
diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py
@@ -1750,7 +1750,7 @@ def test_non_standard_witness_blinding(self):
         tx2.rehash()
         # This will be rejected due to a policy check:
         # No witness is allowed, since it is not a witness program but a p2sh program
-        test_transaction_acceptance(self.nodes[1], self.std_node, tx2, True, False, 'bad-witness-nonstandard')
+        test_transaction_acceptance(self.nodes[1], self.std_node, tx2, True, False, 'bad-witness-nonwitness-input')
 
         # If we send without witness, it should be accepted.
         test_transaction_acceptance(self.nodes[1], self.std_node, tx2, False, True)
@@ -1819,13 +1819,13 @@ def test_non_standard_witness(self):
         # Testing native P2WSH
         # Witness stack size, excluding witnessScript, over 100 is non-standard
         p2wsh_txs[0].wit.vtxinwit[0].scriptWitness.stack = [pad] * 101 + [scripts[0]]
-        test_transaction_acceptance(self.nodes[1], self.std_node, p2wsh_txs[0], True, False, 'bad-witness-nonstandard')
+        test_transaction_acceptance(self.nodes[1], self.std_node, p2wsh_txs[0], True, False, 'bad-witness-stackitem-count')
         # Non-standard nodes should accept
         test_transaction_acceptance(self.nodes[0], self.test_node, p2wsh_txs[0], True, True)
 
         # Stack element size over 80 bytes is non-standard
         p2wsh_txs[1].wit.vtxinwit[0].scriptWitness.stack = [pad * 81] * 100 + [scripts[1]]
-        test_transaction_acceptance(self.nodes[1], self.std_node, p2wsh_txs[1], True, False, 'bad-witness-nonstandard')
+        test_transaction_acceptance(self.nodes[1], self.std_node, p2wsh_txs[1], True, False, 'bad-witness-stackitem-size')
         # Non-standard nodes should accept
         test_transaction_acceptance(self.nodes[0], self.test_node, p2wsh_txs[1], True, True)
         # Standard nodes should accept if element size is not over 80 bytes
@@ -1839,24 +1839,24 @@ def test_non_standard_witness(self):
 
         # witnessScript size at 3601 bytes is non-standard
         p2wsh_txs[3].wit.vtxinwit[0].scriptWitness.stack = [pad, pad, pad, scripts[3]]
-        test_transaction_acceptance(self.nodes[1], self.std_node, p2wsh_txs[3], True, False, 'bad-witness-nonstandard')
+        test_transaction_acceptance(self.nodes[1], self.std_node, p2wsh_txs[3], True, False, 'bad-witness-script-size')
         # Non-standard nodes should accept
         test_transaction_acceptance(self.nodes[0], self.test_node, p2wsh_txs[3], True, True)
 
         # Repeating the same tests with P2SH-P2WSH
         p2sh_txs[0].wit.vtxinwit[0].scriptWitness.stack = [pad] * 101 + [scripts[0]]
-        test_transaction_acceptance(self.nodes[1], self.std_node, p2sh_txs[0], True, False, 'bad-witness-nonstandard')
+        test_transaction_acceptance(self.nodes[1], self.std_node, p2sh_txs[0], True, False, 'bad-witness-stackitem-count')
         test_transaction_acceptance(self.nodes[0], self.test_node, p2sh_txs[0], True, True)
         p2sh_txs[1].wit.vtxinwit[0].scriptWitness.stack = [pad * 81] * 100 + [scripts[1]]
-        test_transaction_acceptance(self.nodes[1], self.std_node, p2sh_txs[1], True, False, 'bad-witness-nonstandard')
+        test_transaction_acceptance(self.nodes[1], self.std_node, p2sh_txs[1], True, False, 'bad-witness-stackitem-size')
         test_transaction_acceptance(self.nodes[0], self.test_node, p2sh_txs[1], True, True)
         p2sh_txs[1].wit.vtxinwit[0].scriptWitness.stack = [pad * 80] * 100 + [scripts[1]]
         test_transaction_acceptance(self.nodes[1], self.std_node, p2sh_txs[1], True, True)
         p2sh_txs[2].wit.vtxinwit[0].scriptWitness.stack = [pad, pad, scripts[2]]
         test_transaction_acceptance(self.nodes[0], self.test_node, p2sh_txs[2], True, True)
         test_transaction_acceptance(self.nodes[1], self.std_node, p2sh_txs[2], True, True)
         p2sh_txs[3].wit.vtxinwit[0].scriptWitness.stack = [pad, pad, pad, scripts[3]]
-        test_transaction_acceptance(self.nodes[1], self.std_node, p2sh_txs[3], True, False, 'bad-witness-nonstandard')
+        test_transaction_acceptance(self.nodes[1], self.std_node, p2sh_txs[3], True, False, 'bad-witness-script-size')
         test_transaction_acceptance(self.nodes[0], self.test_node, p2sh_txs[3], True, True)
 
         self.generate(self.nodes[0], 1)  # Mine and clean up the mempool of non-standard node

Original file line number	Diff line number	Diff line change
`@@ -161,7 +161,7 @@ inline bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& map`
`161`	`161`	`*`
`162`	`162`	`* Also enforce a maximum stack item size limit and no annexes for tapscript spends.`
`163`	`163`	`*/`
`164`		`-bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs);`
	`164`	`+bool IsWitnessStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs, const std::string& reason_prefix, std::string& out_reason, const ignore_rejects_type& ignore_rejects=empty_ignore_rejects);`
`165`	`165`
`166`	`166`	`/** Compute the virtual transaction size (weight reinterpreted as bytes). */`
`167`	`167`	`int64_t GetVirtualTransactionSize(int64_t nWeight, int64_t nSigOpCost, unsigned int bytes_per_sigop);`
Original file line number	Diff line number	Diff line change
`@@ -284,7 +284,8 @@ FUZZ_TARGET(coins_view, .init = initialize_coins_view)`
`284`	`284`	`(void)GetTransactionSigOpCost(transaction, coins_view_cache, flags);`
`285`	`285`	`},`
`286`	`286`	`[&] {`
`287`		`- (void)IsWitnessStandard(CTransaction{random_mutable_transaction}, coins_view_cache);`
	`287`	`+ std::string reason;`
	`288`	`+ (void)IsWitnessStandard(CTransaction{random_mutable_transaction}, coins_view_cache, "bad-witness-", reason);`
`288`	`289`	`});`
`289`	`290`	`}`
`290`	`291`	`}`
Original file line number	Diff line number	Diff line change
`@@ -88,7 +88,8 @@ FUZZ_TARGET(transaction, .init = initialize_transaction)`
`88`	`88`	`CCoinsView coins_view;`
`89`	`89`	`const CCoinsViewCache coins_view_cache(&coins_view);`
`90`	`90`	`(void)AreInputsStandard(tx, coins_view_cache);`
`91`		`- (void)IsWitnessStandard(tx, coins_view_cache);`
	`91`	`+ std::string reject_reason;`
	`92`	`+ (void)IsWitnessStandard(tx, coins_view_cache, "fuzz", reject_reason);`
`92`	`93`
`93`	`94`	`if (tx.GetTotalSize() < 250'000) { // Avoid high memory usage (with msan) due to json encoding`
`94`	`95`	`{`
Original file line number	Diff line number	Diff line change
`@@ -426,7 +426,7 @@ BOOST_FIXTURE_TEST_CASE(package_submission_tests, TestChain100Setup)`
`426`	`426`	`auto it_parent = result_quit_early.m_tx_results.find(tx_parent_invalid->GetWitnessHash());`
`427`	`427`	`auto it_child = result_quit_early.m_tx_results.find(tx_child->GetWitnessHash());`
`428`	`428`	`BOOST_CHECK_EQUAL(it_parent->second.m_state.GetResult(), TxValidationResult::TX_WITNESS_MUTATED);`
`429`		`- BOOST_CHECK_EQUAL(it_parent->second.m_state.GetRejectReason(), "bad-witness-nonstandard");`
	`429`	`+ BOOST_CHECK_EQUAL(it_parent->second.m_state.GetRejectReason(), "bad-witness-nonwitness-input");`
`430`	`430`	`BOOST_CHECK_EQUAL(it_child->second.m_state.GetResult(), TxValidationResult::TX_MISSING_INPUTS);`
`431`	`431`	`BOOST_CHECK_EQUAL(it_child->second.m_state.GetRejectReason(), "bad-txns-inputs-missingorspent");`
`432`	`432`	`}`
Original file line number	Diff line number	Diff line change
`@@ -924,8 +924,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)`
`924`	`924`	`}`
`925`	`925`
`926`	`926`	`// Check for non-standard witnesses.`
`927`		`- if (tx.HasWitness() && m_pool.m_opts.require_standard && !IsWitnessStandard(tx, m_view)) {`
`928`		`- MaybeReject(TxValidationResult::TX_WITNESS_MUTATED, "bad-witness-nonstandard");`
	`927`	`+ if (tx.HasWitness() && m_pool.m_opts.require_standard && !IsWitnessStandard(tx, m_view, "bad-witness-", reason, ignore_rejects)) {`
	`928`	`+ return state.Invalid(TxValidationResult::TX_WITNESS_MUTATED, reason);`
`929`	`929`	`}`
`930`	`930`
`931`	`931`	`int64_t nSigOpsCost = GetTransactionSigOpCost(tx, m_view, STANDARD_SCRIPT_VERIFY_FLAGS);`