Merge bitcoin/bitcoin#31097: validation: Improve input script check error reporting

86e2a6b749c7fecbd086b361806ac9f6e9426d79 [test] A non-standard transaction which is also consensus-invalid should return the consensus error (Antoine Poinsot)
f859ff8a4e9c3aa23bf5be6eceb7099ca72b2290 [validation] Improve script check error reporting (dergoegge)

Pull request description:

  An input script might be invalid for multiple reasons. For example, it might fail both a standardness check and a consensus check, which can lead to a `mandatory-script-verify-flag-failed` error being reported that includes the script error string from the standardness failure (e.g. `mandatory-script-verify-flag-failed (Using OP_CODESEPARATOR in non-witness script)`), which is confusing.

ACKs for top commit:
  darosior:
    re-ACK 86e2a6b749c7fecbd086b361806ac9f6e9426d79
  ariard:
    Re-Code Review ACK 86e2a6b7
  instagibbs:
    ACK 86e2a6b749c7fecbd086b361806ac9f6e9426d79

Tree-SHA512: 053939107c0bcd6643e9006b2518ddc3a6de47d2c6c66af71a04e8af5cf9ec207f19e54583b7a056efd77571edf5fd4f36c31ebe80d1f0777219c756c055eb42

4 files changed, 24 insertions, 2 deletions

diff --git a/src/validation.cpp b/src/validation.cpp
index fe07686209..ab94ac02eb 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -2179,6 +2179,8 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
         if (pvChecks) {
             pvChecks->emplace_back(std::move(check));
         } else if (!check()) {
+            ScriptError error{check.GetScriptError()};
+
             if (flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) {
                 // Check whether the failure was caused by a
                 // non-mandatory script verification check, such as
@@ -2192,6 +2194,14 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
                         flags & ~STANDARD_NOT_MANDATORY_VERIFY_FLAGS, cacheSigStore, &txdata);
                 if (check2())
                     return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(check.GetScriptError())));
+
+                // 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.
+                error = check2.GetScriptError();
             }
             // MANDATORY flag failures correspond to
             // TxValidationResult::TX_CONSENSUS. Because CONSENSUS
@@ -2202,7 +2212,7 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
             // support, to avoid splitting the network (but this
             // depends on the details of how net_processing handles
             // such errors).
-            return state.Invalid(TxValidationResult::TX_CONSENSUS, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(check.GetScriptError())));
+            return state.Invalid(TxValidationResult::TX_CONSENSUS, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(error)));
         }
     }
 
diff --git a/test/functional/data/invalid_txs.py b/test/functional/data/invalid_txs.py
index 2e4ca83bf0..d2d7202d86 100644
--- a/test/functional/data/invalid_txs.py
+++ b/test/functional/data/invalid_txs.py
@@ -263,6 +263,17 @@ def getDisabledOpcodeTemplate(opcode):
         'valid_in_block' : True
         })
 
+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
+    valid_in_block = False
+
+    def get_tx(self):
+        return create_tx_with_script(
+            self.spend_tx, 0, script_sig=b'\x00' * 3 + b'\xab\x6a',
+            amount=(self.spend_avail // 2))
+
 # Disabled opcode tx templates (CVE-2010-5137)
 DisabledOpcodeTemplates = [getDisabledOpcodeTemplate(opcode) for opcode in [
     OP_CAT,
diff --git a/test/functional/feature_block.py b/test/functional/feature_block.py
index 384ca311c7..43bf61c174 100755
--- a/test/functional/feature_block.py
+++ b/test/functional/feature_block.py
@@ -88,6 +88,7 @@ class FullBlockTest(BitcoinTestFramework):
         self.extra_args = [[
             '-acceptnonstdtxn=1',  # This is a consensus block test, we don't care about tx policy
             '-testactivationheight=bip34@2',
+            '-par=1', # Until https://github.com/bitcoin/bitcoin/issues/30960 is fixed
         ]]
 
     def run_test(self):
diff --git a/test/functional/p2p_invalid_tx.py b/test/functional/p2p_invalid_tx.py
index 241aefab24..ee8c6c16ca 100755
--- a/test/functional/p2p_invalid_tx.py
+++ b/test/functional/p2p_invalid_tx.py
@@ -165,7 +165,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=25']):
+        with node.assert_debug_log(['Erased 100 orphan transaction(s) from peer=26']):
             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')