policy: Report reason inputs are nonstandard from AreInputsStandard

Empact commented at 0:50 am on June 23, 2018: member

This results in a more informative debug string to the validation state in the case of failure.

in src/policy/policy.cpp:188 in a1a07ae271 outdated

186+                reason = strprintf("input %u script error: %s", i, ScriptErrorString(serror));
187                 return false;
188-            if (stack.empty())
189+            }
190+            if (stack.empty()) {
191+                reason = strprintf("input %u script stack empty", i);

promag commented at 1:32 pm on June 23, 2018:

Not tested?

in src/policy/policy.h:91 in a1a07ae271 outdated

87@@ -88,9 +88,10 @@ bool IsStandardTx(const CTransaction& tx, std::string& reason);
88     /**
89      * Check for standard transaction types
90      * @param[in] mapInputs    Map of previous transactions that have outputs we're spending
91+     * @param[out] reason      If return is false, contains the reason the inputs were judged non-standard

promag commented at 1:34 pm on June 23, 2018:

contains the reason of the first non-standard input?

in src/policy/policy.cpp:184 in a1a07ae271 outdated

181             std::vector<std::vector<unsigned char> > stack;
182             // convert the scriptSig into a stack, so we can inspect the redeemScript
183-            if (!EvalScript(stack, tx.vin[i].scriptSig, SCRIPT_VERIFY_NONE, BaseSignatureChecker(), SigVersion::BASE))
184+            ScriptError serror;
185+            if (!EvalScript(stack, tx.vin[i].scriptSig, SCRIPT_VERIFY_NONE, BaseSignatureChecker(), SigVersion::BASE, &serror)) {
186+                reason = strprintf("input %u script error: %s", i, ScriptErrorString(serror));

promag commented at 1:35 pm on June 23, 2018:

Not tested?

in src/bench/ccoins_caching.cpp:82 in a1a07ae271 outdated

76@@ -77,8 +77,10 @@ static void CCoinsCaching(benchmark::State& state)
77 
78     // Benchmark.
79     while (state.KeepRunning()) {
80-        bool success = AreInputsStandard(t1, coins);
81-        assert(success);
82+        std::string reason;
83+        if (!AreInputsStandard(t1, coins, reason)) {
84+            throw std::runtime_error(reason);

promag commented at 1:36 pm on June 23, 2018:

Why this change? It must succeed no?

promag commented at 1:37 pm on June 23, 2018: member

Concept ACK.

Empact renamed this:
~~Report reason inputs are nonstandard from AreInputsStandard~~
WIP: Report reason inputs are nonstandard from AreInputsStandard
on Jun 25, 2018

DrahtBot commented at 11:19 am on June 26, 2018: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	promag, MarcoFalke, 0xB10C, glozow, theStack
Stale ACK	sipa

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:

#26403 ([POLICY] Ephemeral anchors by instagibbs)

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.

Empact force-pushed on Jun 28, 2018

Empact commented at 6:35 pm on June 28, 2018: member

@promag Addressed your comments, including tests for each case.

Empact renamed this:
~~WIP: Report reason inputs are nonstandard from AreInputsStandard~~
Report reason inputs are nonstandard from AreInputsStandard
on Jun 28, 2018

laanwj added the label Validation on Jul 9, 2018

sipa commented at 2:57 am on July 14, 2018: member

utACK 7298231aa8faaff65c261eac557448c223b76500

luke-jr commented at 6:47 pm on July 29, 2018: member

This is already part of #7533

At the very least, please try to use the short rejection reasons therein…

luke-jr commented at 6:47 pm on July 29, 2018: member

https://github.com/bitcoin/bitcoin/pull/7533/commits/c59e7166253e962043b5fdccd5de8a21969d21e0

Empact force-pushed on Jul 29, 2018

Empact commented at 11:12 pm on July 29, 2018: member

Thanks @luke-jr, I adapted the short reasons from c59e716. How does that look? Is it alright to put the input index in the rejection reason or better to append?

Empact force-pushed on Jul 29, 2018

Empact commented at 11:12 pm on July 29, 2018: member

Also moved the string instantiation out of the bench loop.

Empact force-pushed on Jul 29, 2018

luke-jr commented at 1:14 am on July 30, 2018: member

Might be better to have the index at the end, not sure.

Empact force-pushed on Jul 30, 2018

Empact commented at 4:16 pm on July 30, 2018: member

@luke-jr moved the input index to the end. Now I’m questioning the need for the bad-txns- prefix, given that these are to become debug output, rather than the reason string, which is what usually bears this prefix.

Empact force-pushed on Jul 30, 2018

Empact commented at 4:24 pm on July 30, 2018: member

Removed bad-txns-. Note these are propagated as the debug string alongside a bad-txns-nonstandard-inputs CValidationState reject reason.

luke-jr commented at 7:34 pm on August 15, 2018: member

Why do you want to limit it to debug output? Reasons should be in the reason field.

Empact force-pushed on Aug 15, 2018

Empact commented at 8:09 pm on August 15, 2018: member

@luke-jr Good point - split the reason and debug fields, and replaced bad-txns-nonstandard-inputs with one of: bad-txns-script-nonstandard, bad-txns-scriptsig-error, or bad-txns-scriptcheck-sigops.

Empact force-pushed on Aug 15, 2018

luke-jr commented at 8:25 pm on August 15, 2018: member

Prefer to use the existing bad-txns-input-script-unknown, bad-txns-input-scriptsig-failure, bad-txns-input-scriptcheck-missing, and bad-txns-input-scriptcheck-sigops

Empact force-pushed on Aug 16, 2018

DrahtBot added the label Needs rebase on Aug 25, 2018

Empact force-pushed on Aug 25, 2018

Empact commented at 6:44 pm on August 25, 2018: member

Rebased for #13429

DrahtBot removed the label Needs rebase on Aug 25, 2018

Empact force-pushed on Aug 30, 2018

Empact commented at 6:26 am on August 30, 2018: member

Fixed rebase for #13429, dropped refactor commit, and made use of every reason recommended by @luke-jr

in src/test/script_p2sh_tests.cpp:410 in c9f429aa91 outdated

406+    txToNonStd4.vout[0].scriptPubKey = GetScriptForDestination(key[1].GetPubKey().GetID());
407+    txToNonStd4.vout[0].nValue = 1000;
408+    txToNonStd4.vin.resize(1);
409+    txToNonStd4.vin[0].prevout.n = 8;
410+    txToNonStd4.vin[0].prevout.hash = txFrom.GetHash();
411+    txToNonStd4.vin[0].scriptSig = CScript(&op_return[0], &op_return[sizeof(op_return)]);

practicalswift commented at 4:04 pm on September 14, 2018:

Isn’t &op_return[sizeof(op_return)] an array overrun?

in src/test/script_tests.cpp:1041 in c9f429aa91 outdated

1036+{
1037+    static const unsigned char op_return[] = { OP_RETURN }; // SCRIPT_ERR_OP_RETURN
1038+
1039+    ScriptError err;
1040+    std::vector<std::vector<unsigned char>> op_return_stack;
1041+    BOOST_CHECK(!EvalScript(op_return_stack, CScript(&op_return[0], &op_return[sizeof(op_return)]), SCRIPT_VERIFY_P2SH, BaseSignatureChecker(), SigVersion::BASE, &err));

practicalswift commented at 4:04 pm on September 14, 2018:

Same here?

Empact commented at 4:34 pm on September 15, 2018: member

@practicalswift replaced existing and new uses of &array[sizeof(array] with array + sizeof(array)

in src/validation.cpp:693 in 5270e588b9 outdated

689@@ -690,8 +690,10 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
690         }
691 
692         // Check for non-standard pay-to-script-hash in inputs
693-        if (fRequireStandard && !AreInputsStandard(tx, view))
694-            return state.Invalid(false, REJECT_NONSTANDARD, "bad-txns-nonstandard-inputs");
695+        std::string reason, debug;

practicalswift commented at 8:37 pm on October 4, 2018:

reason shadows an existing local variable here. Please choose another name :-)

Empact force-pushed on Oct 5, 2018

Empact commented at 7:41 am on October 5, 2018: member

Stopped shadowing reason var (by reusing the existing var). Limited test array access changes to only affecting the added tests.

maflcko referenced this in commit 2a747337ae on Oct 8, 2018

DrahtBot added the label Needs rebase on Dec 12, 2018

Empact force-pushed on Dec 12, 2018

DrahtBot removed the label Needs rebase on Dec 12, 2018

Empact force-pushed on Dec 12, 2018

Empact commented at 11:28 pm on December 12, 2018: member

Rebased for #14908, added test for bad-txns-input-scriptcheck-missing, so testing covers all failure conditions, and removed some test precondition checks that were already captured in the AreInputsStandard outputs.

Empact force-pushed on Dec 12, 2018

Empact commented at 11:33 pm on December 12, 2018: member

Added code-blocks to tests to protect against accidental reuse, e.g. of txToNonStd1 in other test cases.

Empact force-pushed on Dec 13, 2018

Empact commented at 11:29 am on December 14, 2018: member

Fixed a MSVC-specific test failure: “error C2466: cannot allocate an array of constant size 0”, ran clang format. https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/20971133

DrahtBot added the label Needs rebase on May 4, 2019

Empact force-pushed on May 17, 2019

DrahtBot removed the label Needs rebase on May 17, 2019

in src/policy/policy.cpp:193 in 85e2ddb14c outdated

176             std::vector<std::vector<unsigned char> > stack;
177             // convert the scriptSig into a stack, so we can inspect the redeemScript
178-            if (!EvalScript(stack, tx.vin[i].scriptSig, SCRIPT_VERIFY_NONE, BaseSignatureChecker(), SigVersion::BASE))
179+            ScriptError serror;
180+            if (!EvalScript(stack, tx.vin[i].scriptSig, SCRIPT_VERIFY_NONE, BaseSignatureChecker(), SigVersion::BASE, &serror)) {
181+                reason = "bad-txns-input-scriptsig-failure";

sipa commented at 6:50 pm on May 22, 2019:

Bikeshedding: I think it’d be less confusing to call this “bad-txns-malformed-p2sh-scriptsig” (as it’s specific for P2SH, and triggers when the scriptSig can’t be evaluated).

luke-jr commented at 10:47 pm on May 28, 2019:

The order of words typically indicates subcategories/relationships. For that reason, “bad-txns-scriptsig-p2sh-malformed” would fit better.

Empact commented at 0:13 am on May 29, 2019:

I’m currently using: “bad-txns-input-p2sh-scriptsig-malformed” for this.

Because all reasons in this function relate to inputs, of which are tested/subdivided according to type (p2sh in this case), then the subject scrptsig, then the issue.

Not that there’s a right or wrong here, but that’s the rationale.

in src/policy/policy.cpp:198 in 85e2ddb14c outdated

182+                debug = strprintf("input %u: %s", i, ScriptErrorString(serror));
183                 return false;
184-            if (stack.empty())
185+            }
186+            if (stack.empty()) {
187+                reason = "bad-txns-input-scriptcheck-missing";

sipa commented at 6:50 pm on May 22, 2019:

How about “bad-txns-input-p2sh-no-redeemscript”?

sipa commented at 7:02 pm on May 22, 2019: member

utACK 85e2ddb14c15b9613093ec0cb991b77091c4287c, with or without nits addressed.

maflcko renamed this:
~~Report reason inputs are nonstandard from AreInputsStandard~~
policy: Report reason inputs are nonstandard from AreInputsStandard
on May 22, 2019

maflcko removed the label Validation on May 22, 2019

maflcko added the label TX fees and policy on May 22, 2019

Empact force-pushed on May 28, 2019

Empact commented at 6:22 pm on May 28, 2019: member

Updated:

bad-txns-input-scriptsig-failure -> bad-txns-input-p2sh-scriptsig-malformed
bad-txns-input-scriptcheck-missing -> bad-txns-input-p2sh-no-redeemscript
bad-txns-input-scriptcheck-sigops -> bad-txns-input-p2sh-redeemscript-sigops

Note the current behavior returns bad-txns-nonstandard-inputs in all cases.

in src/validation.cpp:710 in 6ca56b796b outdated

705@@ -706,8 +706,9 @@ static bool AcceptToMemoryPoolWorker(const CChainParams& chainparams, CTxMemPool
706         }
707 
708         // Check for non-standard pay-to-script-hash in inputs
709-        if (fRequireStandard && !AreInputsStandard(tx, view))
710-            return state.Invalid(ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, "bad-txns-nonstandard-inputs");
711+        std::string debug;
712+        if (fRequireStandard && !AreInputsStandard(tx, view, reason, debug))

maflcko commented at 6:50 pm on June 17, 2019:

style-nit: Could add {?

Empact commented at 11:45 am on October 9, 2019:

Added 👍

maflcko commented at 6:50 pm on June 17, 2019: member

Concept ACK

DrahtBot added the label Needs rebase on Sep 18, 2019

Empact force-pushed on Oct 9, 2019

Empact commented at 11:46 am on October 9, 2019: member

Rebased, added missing braces and tinyformat include.

Empact force-pushed on Oct 9, 2019

DrahtBot removed the label Needs rebase on Oct 9, 2019

DrahtBot added the label Needs rebase on Oct 24, 2019

Empact force-pushed on Jan 11, 2020

Empact commented at 0:14 am on January 11, 2020: member

Rebased

DrahtBot removed the label Needs rebase on Jan 11, 2020

sipa commented at 6:32 pm on March 28, 2020: member

utACK 6a241604fec4346d90495fd468f54a6bf6a0ee88

DrahtBot added the label Needs rebase on May 4, 2020

Empact force-pushed on May 9, 2020

Empact commented at 7:10 am on May 9, 2020: member

Rebased for #18859

DrahtBot removed the label Needs rebase on May 9, 2020

Empact force-pushed on Jun 22, 2020

DrahtBot added the label Needs rebase on Jun 28, 2020

0xB10C commented at 1:46 pm on August 3, 2021: contributor

Concept ACK

in src/policy/policy.h:112 in 7758133557 outdated

89@@ -90,9 +90,11 @@ bool IsStandardTx(const CTransaction& tx, bool permit_bare_multisig, const CFeeR
90     /**
91      * Check for standard transaction types
92      * @param[in] mapInputs    Map of previous transactions that have outputs we're spending
93+     * @param[out] reason      If return is false, contains the first non-standard input's reason for being judged non-standard
94+     * @param[out] debug       If return is false, contains additional debug info related to the invalid input, including its index
95      * @return True if all inputs (scriptSigs) use only standard transaction forms
96      */
97-bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs);
98+bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs, std::string& reason, std::string& debug);

glozow commented at 2:32 pm on October 25, 2021:

I think the best way to do this is to change the AreInputsStandard interface to return a std::optional<std::pair<std::string, std::string>> instead of passing in/reusing mutable references.

Alternatively, you can pass in a TxValidationState& state which already has reason and debug fields.

glozow commented at 2:37 pm on October 25, 2021: member

Concept ACK to more informative error messages

Empact force-pushed on Feb 28, 2022

DrahtBot removed the label Needs rebase on Feb 28, 2022

Empact force-pushed on Feb 28, 2022

DrahtBot added the label Needs rebase on May 26, 2022

Empact force-pushed on Jun 18, 2022

DrahtBot removed the label Needs rebase on Jun 18, 2022

theStack commented at 1:13 pm on June 22, 2022: contributor

Concept ACK

DrahtBot added the label Needs rebase on Aug 3, 2022

Empact force-pushed on Aug 8, 2022

DrahtBot removed the label Needs rebase on Aug 8, 2022

glozow requested review from theStack on Aug 24, 2022

in src/policy/policy.cpp:200 in 127955c701 outdated

195             // flag in the script interpreter, but it can be helpful to catch
196             // this type of NONSTANDARD transaction earlier in transaction
197             // validation.
198+            reason = "bad-txns-input-witness-unknown";
199+            debug = strprintf("input %u", i);
200             return false;

theStack commented at 0:25 am on August 25, 2022:

This if-branch is currently dead code, since the type WITNESS_UNKNOWN is already matched in the first if-condition a few lines above (obvioulsy, it was forgotten to be removed there). I.e. we want

 0diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp
 1index 08f41376a..581fdcce3 100644
 2--- a/src/policy/policy.cpp
 3+++ b/src/policy/policy.cpp
 4@@ -186,7 +186,7 @@ std::optional<std::pair<std::string, std::string>> AreInputsStandard(const CTran
 5
 6         std::vector<std::vector<unsigned char> > vSolutions;
 7         TxoutType whichType = Solver(prev.scriptPubKey, vSolutions);
 8-        if (whichType == TxoutType::NONSTANDARD || whichType == TxoutType::WITNESS_UNKNOWN) {
 9+        if (whichType == TxoutType::NONSTANDARD) {
10             return std::make_pair(
11                 "bad-txns-input-script-nonstandard",
12                 strprintf("input %u", i)

To avoid issues like this, should probably also add a test for “bad-txns-input-witness-unknown”?

Empact commented at 11:00 pm on October 9, 2022:

Good point! Updated here: https://github.com/bitcoin/bitcoin/compare/631d3c980090343f2f0d9dd97dac585c7fa94f13..1f3c017cce798bdcee1d719bf160dc07783239f2

Empact force-pushed on Oct 9, 2022

theStack commented at 10:16 am on October 10, 2022: contributor

CI fails, seems like some functional tests need to be adapted to match the more specific failure reason string (https://cirrus-ci.com/task/6048174287618048?logs=ci#L3337):

0...
1AssertionError: [node 1] Expected messages "['bad-txns-input-script-nonstandard']" does not partially match log:
2...

Propagate validation debug information from AreInputsStandard

This adds specific debug information including the input index and manner of
failure.

ba538fba89

refactor: Convert AreInputsStandard to return results via std::opt<std::pair> dfa194343f

Empact force-pushed on Oct 10, 2022

Empact commented at 5:38 pm on October 10, 2022: member

CI fails, seems like some functional tests need to be adapted to match the more specific failure reason string (https://cirrus-ci.com/task/6048174287618048?logs=ci#L3337):
0...
1AssertionError: [node 1] Expected messages "['bad-txns-input-script-nonstandard']" does not partially match log:
2...

@theStack fixed: https://github.com/bitcoin/bitcoin/compare/1f3c017cce798bdcee1d719bf160dc07783239f2..dfa194343fde1557420a3b4a52b6a93e42adb848

in src/test/script_p2sh_tests.cpp:444 in ba538fba89 outdated

440+    txWitnessUnknown.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key[1].GetPubKey()));
441+    txWitnessUnknown.vout[0].nValue = 1000;
442+    txWitnessUnknown.vin.resize(1);
443+    txWitnessUnknown.vin[0].prevout.n = 9;
444+    txWitnessUnknown.vin[0].prevout.hash = txFrom.GetHash();
445+    txWitnessUnknown.vin[0].scriptSig << std::vector<unsigned char>(witnessUnknown.begin(), witnessUnknown.end());

achow101 commented at 8:11 pm on November 21, 2022:

In ba538fba899482f5549a6377d657d9d89b13bfd5 “Propagate validation debug information from AreInputsStandard”

nit: setting scriptSig is unnecessary as the thing being checked is the scriptPubKey of the output being spent.

achow101 commented at 8:15 pm on November 21, 2022: member

The more descriptive reason strings seem to have gotten lost in the past couple of years.

aureleoules commented at 12:48 pm on January 2, 2023: member

Instead of returning an option would the Result class make more sense? Also, instead of a pair of strings, maybe create a new struct?

achow101 commented at 3:47 pm on April 25, 2023: member

Closing this as it has not had any activity in a while. If you are interested in continuing work on this, please leave a comment so that it can be reopened.

achow101 closed this on Apr 25, 2023

glozow added the label Up for grabs on Apr 25, 2023

maflcko removed the label Up for grabs on Dec 12, 2023

policy: Report reason inputs are nonstandard from AreInputsStandard #13525

Reviews

Conflicts