Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Policy: Report reason inputs are non standard #29060

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Policy: Report reason inputs are non standard #29060

wants to merge 2 commits into from
+136 −30

Conversation

ismaelsadeeq
Copy link
Member

This PR is another attempt at #13525.

Transactions that fail PreChecks Validation due to non-standard inputs now returns invalid validation stateTxValidationResult::TX_INPUTS_NOT_STANDARD along with a debug error message.

Previously, the debug error message for non-standard inputs do not specify why the inputs were considered non-standard.
Instead, the same error string, bad-txns-nonstandard-inputs, used for all types of non-standard input scriptSigs.

This PR updates the AreInputsStandard to include the reason why inputs are non-standard in the debug message.
This improves the Precheck debug message to be more descriptive.

Furthermore, I have addressed all remaining comments from #13525 in this PR.

@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 12, 2023 •
edited

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
Concept ACK stickies-v

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

Conflicts

No conflicts as of last run.

@ismaelsadeeq ismaelsadeeq force-pushed the 11-2023-non-standard-inputs-error-messages branch from 3fc397a to 6ffb956 Compare December 12, 2023 13:48
stickies-v
stickies-v reviewed Dec 13, 2023
View reviewed changes
Copy link
Contributor

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK

src/policy/policy.cpp Outdated Show resolved Hide resolved
src/policy/policy.cpp Outdated Show resolved Hide resolved
src/test/script_p2sh_tests.cpp Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
src/bench/ccoins_caching.cpp Outdated Show resolved Hide resolved
src/policy/policy.h Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
src/policy/policy.cpp Outdated Show resolved Hide resolved
@ismaelsadeeq ismaelsadeeq force-pushed the 11-2023-non-standard-inputs-error-messages branch from 6ffb956 to e032462 Compare December 14, 2023 11:43
@ismaelsadeeq
Copy link
Member Author

Thank you for review @stickies-v

I Addressed your comments and taken suggestions.
Force pushed from 6ffb956 to e032462 , Compare diff

@DrahtBot DrahtBot mentioned this pull request Dec 15, 2023
Merged
luke-jr
luke-jr suggested changes Dec 17, 2023
View reviewed changes
src/policy/policy.cpp Outdated Show resolved Hide resolved
src/policy/policy.cpp Outdated Show resolved Hide resolved
@ismaelsadeeq ismaelsadeeq force-pushed the 11-2023-non-standard-inputs-error-messages branch from e032462 to 814571b Compare December 18, 2023 14:53
@ismaelsadeeq
Copy link
Member Author

ismaelsadeeq commented Dec 18, 2023 •
edited

Thanks for reviewing @luke-jr
Force pushed from e032462 to 0a39b07

Compare diff

The changes in latest push are:

  • Rebased on master to fix CI
  • Updated the non-standardness reason to address @luke-jr comments
  • bad-txns-input-script-nonstandard --> bad-txns-input-script-unknown
  • bad-txns-input-scriptsig-failure --> bad-txns-input-p2sh-scriptsig-malformed
  • bad-txns-input-p2sh-no-redeemscript --> bad-txns-input-scriptcheck-missing
  • bad-txns-input-scriptcheck-sigops --> bad-txns-input-p2sh-redeemscript-sigops

@ismaelsadeeq ismaelsadeeq force-pushed the 11-2023-non-standard-inputs-error-messages branch from 814571b to 0a39b07 Compare December 18, 2023 15:46
glozow
glozow reviewed Dec 20, 2023
View reviewed changes
src/policy/policy.h Outdated Show resolved Hide resolved
@ismaelsadeeq ismaelsadeeq force-pushed the 11-2023-non-standard-inputs-error-messages branch 2 times, most recently from d9e71f0 to 5ef3f9b Compare December 24, 2023 14:41
@ismaelsadeeq ismaelsadeeq force-pushed the 11-2023-non-standard-inputs-error-messages branch from 5ef3f9b to f4c1945 Compare January 25, 2024 23:05
@ismaelsadeeq
Copy link
Member Author

Rebased on master for green CI.

@achow101 achow101 requested review from luke-jr and glozow April 9, 2024 15:37
ismaelsadeeq and others added 2 commits May 17, 2024 08:59
@ismaelsadeeq
policy: update AreInputsStandard to return error string
4a59b1e 
This commit renames AreInputsStandard to HasNonStandardInput.

HasNonStandardInput now returns valid TxValidationState if all inputs
(scriptSigs) use only standard transaction forms else returns invalid
TxValidationState which states why an input is not standard.
@ismaelsadeeq @Empact
test: ensure HasNonStandardInput optionally returns debug string
101a308 
Test the various input scriptSig non standardness, and ensure HasNonStandardInput
Returns the resin why the input is nonstandard.

Co-authored-by: Ben Woosley <ben.woosley@gmail.com>
@ismaelsadeeq ismaelsadeeq force-pushed the 11-2023-non-standard-inputs-error-messages branch from f4c1945 to 101a308 Compare May 17, 2024 08:00
luke-jr
luke-jr reviewed May 31, 2024
View reviewed changes
src/validation.cpp
if (m_pool.m_opts.require_standard && !AreInputsStandard(tx, m_view)) {
return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, "bad-txns-nonstandard-inputs");
if (m_pool.m_opts.require_standard) {
state = HasNonStandardInput(tx, m_view);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it'd be nicer to just pass state into the function rather than creating a new one and copying it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like it'd be nicer to just pass state into the function

I am taking this review comment as a non-blocking (nit) suggestion based on the tone.

I will update this when there are blocking comments that indicate a need to retouch the diffs.

Also, I would like to know whether this PR is conceptually acknowledged, @luke-jr.
Thanks for your review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luke-jr luke-jr luke-jr requested changes

@glozow glozow Awaiting requested review from glozow

@stickies-v stickies-v Awaiting requested review from stickies-v

Assignees
No one assigned
Labels
TX fees and policy
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ismaelsadeeq @DrahtBot @luke-jr @glozow @stickies-v