bitcoin
validation: Improve input script check error reporting
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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Code Coverage
For detailed information about the code coverage, see the test coverage report.
Reviews
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | darosior, ariard, instagibbs |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
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.
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')
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?
That worked, thanks!