validation: Improve input script check error reporting #31097

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.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage

For detailed information about the code coverage, see the test coverage report.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #31112 (Improve parallel script validation error debug logging by sipa)

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

utACK f859ff8a4e9c3aa23bf5be6eceb7099ca72b2290

A bit surprising that all tests passed. Maybe a test-case can be modified, or a new test can be added, so that the test changes fail on master and pass on this pull request? Otherwise, this may be broken again in the future. Also, it is harder to review, because each reviewer will have to write the test themselves.

Here is a functional test which creates a transaction using an OP_CODESEPARATOR in its scriptSig (non-standard) but also uses an OP_RETURN right after, which makes it invalid by consensus. It returns the invalid combination of error strings fixed in this PR ("mandatory-script-verify-flag-failed" because it's a consensus failure, but "Using OP_CODESEPARATOR in non-witness script" because it returns the error string for the first check).

You should be able to trivially adapt it to the fix in this PR by updating the error message.

diff --git a/test/functional/data/invalid_txs.py b/test/functional/data/invalid_txs.py
index 33054fd517..6c259d59bb 100644
--- a/test/functional/data/invalid_txs.py
+++ b/test/functional/data/invalid_txs.py
@@ -245,6 +245,19 @@ class TooManySigops(BadTxTemplate):
             script_pub_key=lotsa_checksigs,
             amount=1)
 
+
+class NonStandardAndInvalid(BadTxTemplate):
+    """A non-standard transaction which is also consensus-invalid should return the consensus error."""
+    reject_reason = "mandatory-script-verify-flag-failed (Using OP_CODESEPARATOR in non-witness script)"
+    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))
+
+
 def getDisabledOpcodeTemplate(opcode):
     """ Creates disabled opcode tx template class"""
     def get_tx(self):
diff --git a/test/functional/p2p_invalid_tx.py b/test/functional/p2p_invalid_tx.py
index 0ae05d4b0b..7782217152 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')

@darosior added the test to this PR, thanks!

The test fails due to https://github.com/bitcoin/bitcoin/issues/30960

utACK ad51775

Re-tested, getting the correct RPC string.

I don't know what to do about the CI failure. I guess we need a fix for #30960? In the mean time I can remove the test commit.

-par=1 (or whatever to make the thread inline) should work, no?

-par=1 (or whatever to make the thread inline) should work, no?

That worked, thanks!

re-ACK 86e2a6b749c7fecbd086b361806ac9f6e9426d79

Re-Code Review ACK 86e2a6b7

The code nit improvement, ready to be git cherry-picked:https://github.com/ariard/bitcoin/tree/2024-10-check2-impr

ACK 86e2a6b749c7fecbd086b361806ac9f6e9426d79

played with tests to ensure the test was failing how I'd expect it with modifications

post merge tACK 86e2a6b749c7fecbd086b361806ac9f6e9426d79