validation: Improve input script check error reporting

dergoegge commented at 9:47 am on October 16, 2024: member

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.

[validation] Improve script check error reporting f859ff8a4e

DrahtBot commented at 9:47 am on October 16, 2024: contributor

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.

DrahtBot added the label Validation on Oct 16, 2024

darosior commented at 10:16 am on October 16, 2024: member

utACK f859ff8a4e9c3aa23bf5be6eceb7099ca72b2290

maflcko commented at 11:12 am on October 16, 2024: member

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.

darosior commented at 12:24 pm on October 16, 2024: member

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.

 0diff --git a/test/functional/data/invalid_txs.py b/test/functional/data/invalid_txs.py
 1index 33054fd517..6c259d59bb 100644
 2--- a/test/functional/data/invalid_txs.py
 3+++ b/test/functional/data/invalid_txs.py
 4@@ -245,6 +245,19 @@ class TooManySigops(BadTxTemplate):
 5             script_pub_key=lotsa_checksigs,
 6             amount=1)
 7 
 8+
 9+class NonStandardAndInvalid(BadTxTemplate):
10+    """A non-standard transaction which is also consensus-invalid should return the consensus error."""
11+    reject_reason = "mandatory-script-verify-flag-failed (Using OP_CODESEPARATOR in non-witness script)"
12+    expect_disconnect = True
13+    valid_in_block = False
14+
15+    def get_tx(self):
16+        return create_tx_with_script(
17+            self.spend_tx, 0, script_sig=b'\x00' * 3 + b'\xab\x6a',
18+            amount=(self.spend_avail // 2))
19+
20+
21 def getDisabledOpcodeTemplate(opcode):
22     """ Creates disabled opcode tx template class"""
23     def get_tx(self):
24diff --git a/test/functional/p2p_invalid_tx.py b/test/functional/p2p_invalid_tx.py
25index 0ae05d4b0b..7782217152 100755
26--- a/test/functional/p2p_invalid_tx.py
27+++ b/test/functional/p2p_invalid_tx.py
28@@ -165,7 +165,7 @@ class InvalidTxRequestTest(BitcoinTestFramework):
29             node.p2ps[0].send_txs_and_test([rejected_parent], node, success=False)
30 
31         self.log.info('Test that a peer disconnection causes erase its transactions from the orphan pool')
32-        with node.assert_debug_log(['Erased 100 orphan transaction(s) from peer=25']):
33+        with node.assert_debug_log(['Erased 100 orphan transaction(s) from peer=26']):
34             self.reconnect_p2p(num_connections=1)
35 
36         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')

dergoegge commented at 12:36 pm on October 16, 2024: member

@darosior added the test to this PR, thanks!

DrahtBot added the label CI failed on Oct 16, 2024

maflcko commented at 3:23 pm on October 16, 2024: member

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

in src/validation.cpp:2200 in ad517754c6 outdated

2193@@ -2192,6 +2194,14 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
2194                         flags & ~STANDARD_NOT_MANDATORY_VERIFY_FLAGS, cacheSigStore, &txdata);
2195                 if (check2())
2196                     return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(check.GetScriptError())));
2197+
2198+                // If the second check failed, it failed due to a mandatory script verification
2199+                // flag, but the first check might have failed on a non-mandatory script
2200+                // verification flag.

ariard commented at 8:22 pm on October 16, 2024:

I was reading the comment just above this one i.e “Check whether the failure was caused by a non-mandatory script verification check, such as non-standard DER encodings or non-null dummy arguments”.

I think the 2 examples given in this comment are actually “mandatory-script-verify-flag”, i.e CONSENSUS and not NOT_STANDARD:

DER encodings (BIP66 and SCRIPT_VERIFY_DERSIG), interpreter flag is set in GetBlockScriptFlags https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L2378
NULLDUMMY for CHECKMULTISIG (BIP147 and SCRIPT_VERIFY_NULLDUMMY), interpreter flag is set in GetBlockScriptFlags https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L2393

Those 2 consensus changes have been buried, so I think the comment could reflet there are TX_NOT_STANDARD before the buried height (correspondingly 363725 and 481824) and verification check error for DER encoding and NULLDUMMY should be TX_CONSENSUS after.

in test/functional/data/invalid_txs.py:272 in ad517754c6 outdated

267+    """A non-standard transaction which is also consensus-invalid should return the consensus error."""
268+    reject_reason = "mandatory-script-verify-flag-failed (OP_RETURN was encountered)"
269+    expect_disconnect = True
270+    valid_in_block = False
271+
272+    def get_tx(self):

ariard commented at 8:43 pm on October 16, 2024:

I think we should have further tests that a non-standard transaction which is consensus valid before an activation height, is considered as consensus invalid after the activation height.

E.g on regtest, the deployment heights are always active unless overridden. I don’t think we do that in invalid_txs.py currently: https://github.com/bitcoin/bitcoin/blob/master/test/functional/p2p_invalid_tx.py#L28

ariard commented at 8:45 pm on October 16, 2024: contributor

utACK ad51775

DrahtBot requested review from darosior on Oct 16, 2024

in src/validation.cpp:2196 in ad517754c6 outdated

2193@@ -2192,6 +2194,14 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
2194                         flags & ~STANDARD_NOT_MANDATORY_VERIFY_FLAGS, cacheSigStore, &txdata);
2195                 if (check2())
2196                     return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(check.GetScriptError())));

ariard commented at 2:37 am on October 17, 2024:

 0diff --git a/src/validation.cpp b/src/validation.cpp
 1index fc61b14dc1..ecc1ab22dd 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -1970,7 +1970,7 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
 5                 CScriptCheck check2(txdata.m_spent_outputs[i], tx, i,
 6                         flags & ~STANDARD_NOT_MANDATORY_VERIFY_FLAGS, cacheSigStore, &txdata);
 7                 if (check2())
 8-                    return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(check.GetScriptError())));
 9+                    return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(error)));
10 
11                 // If the second check failed, it failed due to a mandatory script verification
12                 // flag, but the first check might have failed on a non-mandatory script

dergoegge commented at 11:58 am on October 17, 2024:

Will do if i push again

instagibbs commented at 1:15 pm on October 21, 2024:

nit: Agreed, this would help readability.

fanquake commented at 1:50 pm on October 21, 2024:

@sipa might be able to pull this into #31112, given it’s following up.

sipa commented at 1:54 pm on October 21, 2024:

My follow-up PR removes CScriptCheck::GetScriptError, so this modification isn’t really applicable anymore.

ariard commented at 2:37 am on October 17, 2024: contributor

Re-tested, getting the correct RPC string.

dergoegge commented at 8:56 am on October 17, 2024: member

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.

maflcko commented at 9:43 am on October 17, 2024: member

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

[test] A non-standard transaction which is also consensus-invalid should return the consensus error 86e2a6b749

dergoegge force-pushed on Oct 17, 2024

DrahtBot removed the label CI failed on Oct 17, 2024

dergoegge commented at 11:58 am on October 17, 2024: member

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

That worked, thanks!

darosior commented at 2:06 pm on October 17, 2024: member

re-ACK 86e2a6b749c7fecbd086b361806ac9f6e9426d79

DrahtBot requested review from ariard on Oct 17, 2024

ariard commented at 9:47 pm on October 17, 2024: contributor

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

fanquake requested review from instagibbs on Oct 21, 2024

instagibbs approved

instagibbs commented at 1:45 pm on October 21, 2024: member

ACK 86e2a6b749c7fecbd086b361806ac9f6e9426d79

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

fanquake merged this on Oct 21, 2024

fanquake closed this on Oct 21, 2024

danielabrozzoni commented at 2:10 pm on October 21, 2024: contributor

post merge tACK 86e2a6b749c7fecbd086b361806ac9f6e9426d79

validation: Improve input script check error reporting #31097

Code Coverage

Reviews

Conflicts