policy: Report reason inputs are nonstandard from AreInputsStandard #13525

Empact · 2018-06-23T00:50:55Z

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

promag

Concept ACK.

src/policy/policy.cpp

src/policy/policy.h

src/policy/policy.cpp

src/bench/ccoins_caching.cpp

DrahtBot · 2018-06-26T11:19:18Z

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 · 2018-06-28T18:35:33Z

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

sipa · 2018-07-14T02:57:31Z

utACK 7298231aa8faaff65c261eac557448c223b76500

luke-jr · 2018-07-29T18:47:27Z

This is already part of #7533

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

luke-jr · 2018-07-29T18:47:41Z

c59e716

Empact · 2018-07-29T23:12:02Z

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 · 2018-07-29T23:12:52Z

Also moved the string instantiation out of the bench loop.

luke-jr · 2018-07-30T01:14:07Z

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

Empact · 2018-07-30T16:16:19Z

@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 · 2018-07-30T16:24:40Z

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

luke-jr · 2018-08-15T19:34:29Z

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

Empact · 2018-08-15T20:09:30Z

@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.

theStack · 2022-06-22T13:13:16Z

Concept ACK

src/policy/policy.cpp

theStack

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):

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

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

…d::pair>

Empact · 2022-10-10T17:38:52Z

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):
...
AssertionError: [node 1] Expected messages "['bad-txns-input-script-nonstandard']" does not partially match log:
...

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

achow101

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

achow101 · 2022-11-21T20:11:15Z

src/test/script_p2sh_tests.cpp

+    txWitnessUnknown.vin.resize(1);
+    txWitnessUnknown.vin[0].prevout.n = 9;
+    txWitnessUnknown.vin[0].prevout.hash = txFrom.GetHash();
+    txWitnessUnknown.vin[0].scriptSig << std::vector<unsigned char>(witnessUnknown.begin(), witnessUnknown.end());


In ba538fb "Propagate validation debug information from AreInputsStandard"

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

aureleoules

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 · 2023-04-25T15:47:25Z

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.

promag reviewed Jun 23, 2018

View reviewed changes

src/policy/policy.cpp Outdated Show resolved Hide resolved

src/policy/policy.h Outdated Show resolved Hide resolved

src/policy/policy.cpp Outdated Show resolved Hide resolved

src/bench/ccoins_caching.cpp Outdated Show resolved Hide resolved

Empact changed the title ~~Report reason inputs are nonstandard from AreInputsStandard~~ WIP: Report reason inputs are nonstandard from AreInputsStandard Jun 25, 2018

This was referenced Jun 26, 2018

WIP [bench] CCoinsView(Cache): measure various scenarios #13470

Closed

Return the script type from Solver #13429

Merged

Empact force-pushed the are-inputs-standard-reason branch from a1a07ae to 7298231 Compare June 28, 2018 18:33

Empact changed the title ~~WIP: Report reason inputs are nonstandard from AreInputsStandard~~ Report reason inputs are nonstandard from AreInputsStandard Jun 28, 2018

laanwj added the Validation label Jul 9, 2018

Empact force-pushed the are-inputs-standard-reason branch from 7298231 to dab2384 Compare July 29, 2018 23:09

Empact force-pushed the are-inputs-standard-reason branch from dab2384 to b39bc02 Compare July 29, 2018 23:12

Empact force-pushed the are-inputs-standard-reason branch from b39bc02 to 54f120c Compare July 29, 2018 23:56

Empact force-pushed the are-inputs-standard-reason branch from 54f120c to bf785af Compare July 30, 2018 16:13

Empact force-pushed the are-inputs-standard-reason branch 2 times, most recently from 3135078 to 7986ccf Compare July 30, 2018 16:22

This was referenced Aug 1, 2018

Include tinyformat as a subtree #13842

Closed

Include tinyformat as a subtree #13845

Closed

Move src/tinyformat.h to src/tinyformat/tinyformat.h #13846

Closed

Empact force-pushed the are-inputs-standard-reason branch from 7986ccf to 781c350 Compare August 15, 2018 20:06

Empact force-pushed the are-inputs-standard-reason branch from 781c350 to 54020f6 Compare August 15, 2018 20:22

Empact force-pushed the are-inputs-standard-reason branch from 84bc9d3 to d152d78 Compare February 28, 2022 16:20

DrahtBot mentioned this pull request Mar 1, 2022

Simplify BaseSignatureChecker virtual functions and GenericTransactionSignatureChecker constructors #22793

Closed

DrahtBot mentioned this pull request May 25, 2022

doc: remove misleading AreInputsStandard() comment #25210

Merged

DrahtBot added the Needs rebase label May 26, 2022

Empact force-pushed the are-inputs-standard-reason branch from d152d78 to 8383577 Compare June 18, 2022 14:28

DrahtBot removed the Needs rebase label Jun 18, 2022

DrahtBot mentioned this pull request Jul 20, 2022

refactor: Remove all policy globals #25648

Merged

DrahtBot added the Needs rebase label Aug 3, 2022

Empact force-pushed the are-inputs-standard-reason branch 2 times, most recently from df4089e to 631d3c9 Compare August 8, 2022 05:15

DrahtBot removed the Needs rebase label Aug 8, 2022

glozow requested a review from theStack August 24, 2022 15:45

theStack reviewed Aug 25, 2022

View reviewed changes

src/policy/policy.cpp Outdated Show resolved Hide resolved

Empact force-pushed the are-inputs-standard-reason branch from 631d3c9 to 1f3c017 Compare October 9, 2022 22:57

theStack reviewed Oct 10, 2022

View reviewed changes

Empact added 2 commits October 10, 2022 11:44

Propagate validation debug information from AreInputsStandard

ba538fb

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

refactor: Convert AreInputsStandard to return results via std::opt<st…

dfa1943

…d::pair>

Empact force-pushed the are-inputs-standard-reason branch from 1f3c017 to dfa1943 Compare October 10, 2022 15:44

DrahtBot mentioned this pull request Oct 20, 2022

Make P2SH redeem script "IF .. PUSH <x> ELSE ... PUSH <y> ENDIF CHECKMULTISIG .. " standard #26348

Closed

DrahtBot mentioned this pull request Nov 4, 2022

policy: Ephemeral anchors #26403

Closed

achow101 reviewed Nov 21, 2022

View reviewed changes

aureleoules reviewed Jan 2, 2023

View reviewed changes

achow101 closed this Apr 25, 2023

glozow added the Up for grabs label Apr 25, 2023

ismaelsadeeq mentioned this pull request Dec 12, 2023

Policy: Report debug message why inputs are non standard #29060

Open

maflcko removed the Up for grabs label Dec 12, 2023

bitcoin locked and limited conversation to collaborators Dec 11, 2024

policy: Report reason inputs are nonstandard from AreInputsStandard #13525

policy: Report reason inputs are nonstandard from AreInputsStandard #13525

Uh oh!

Conversation

Empact commented Jun 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DrahtBot commented Jun 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

Empact commented Jun 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sipa commented Jul 14, 2018

Uh oh!

luke-jr commented Jul 29, 2018

Uh oh!

luke-jr commented Jul 29, 2018

Uh oh!

Empact commented Jul 29, 2018

Uh oh!

Empact commented Jul 29, 2018

Uh oh!

luke-jr commented Jul 30, 2018

Uh oh!

Empact commented Jul 30, 2018

Uh oh!

Empact commented Jul 30, 2018

Uh oh!

luke-jr commented Aug 15, 2018

Uh oh!

Empact commented Aug 15, 2018

Uh oh!

theStack commented Jun 22, 2022

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Empact commented Oct 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 Nov 21, 2022

Choose a reason for hiding this comment

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Apr 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

Empact commented Jun 23, 2018 •

edited

Loading

DrahtBot commented Jun 26, 2018 •

edited

Loading

Empact commented Jun 28, 2018 •

edited

Loading

Empact commented Oct 10, 2022 •

edited

Loading