Skip to content

test: Remove confusing DUMMY_P2WPKH_SCRIPT #26277

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
Merged
fanquake merged 1 commit into from
Oct 10, 2022
Merged

test: Remove confusing DUMMY_P2WPKH_SCRIPT #26277

fanquake merged 1 commit into from
Oct 10, 2022

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Oct 7, 2022

It is confusing because, it is not a P2WPKH script, and it is nonstandard.

See also https://github.com/bitcoin/bitcoin/pull/26265/files#r989827855

Fix all issues by removing it, and also remove the no longer needed -acceptnonstdtxn setting from the test.

@maflcko maflcko force-pushed the 2210-test-no-dummy- branch from 40db2cf to faf1a3d Compare October 7, 2022 09:18
theStack
theStack reviewed Oct 7, 2022
View reviewed changes
Copy link
Contributor

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

An alternative to introducing another (magic-number) constant would be to directly use script_to_p2wsh_script(CScript([OP_TRUE]))) instead, though it's slightly longer.

@maflcko maflcko force-pushed the 2210-test-no-dummy- branch from faf1a3d to fa8a305 Compare October 7, 2022 11:12
@maflcko
Copy link
Member Author

maflcko commented Oct 7, 2022

Thanks, pushed an alternative that only modifies a single file to reduce merge-conflicting lines.

@DrahtBot DrahtBot added the Tests label Oct 7, 2022
@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 7, 2022

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #26265 (POLICY: Relax MIN_STANDARD_TX_NONWITNESS_SIZE to 65 non-witness bytes 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.

@DrahtBot DrahtBot mentioned this pull request Oct 7, 2022
Closed
theStack
theStack approved these changes Oct 7, 2022
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

Code-review ACK fa8a305 📜

@instagibbs
Copy link
Member

instagibbs commented Oct 7, 2022

ACK fa8a305

All the replacements of scripts and injection of witnesses look correct, and allows simplification/removal of other tests and utils.

darosior
darosior suggested changes Oct 8, 2022
View reviewed changes
Copy link
Member

@darosior darosior left a comment

Choose a reason for hiding this comment

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

Looks like you didn't actually remove DUMMY_P2WPKH_SCRIPT's definition from test_framework/script_util.py?

@fanquake
Copy link
Member

fanquake commented Oct 10, 2022

Looks like you didn't actually remove DUMMY_P2WPKH_SCRIPT's definition from test_framework/script_util.py?

Yep. Looks like that and DUMMY_2_P2WPKH_SCRIPT can be deleted there.

@maflcko
Copy link
Member Author

maflcko commented Oct 10, 2022

DUMMY_2_P2WPKH_SCRIPT is already unused, so it seems better to remove them in a pull unrelated to this. As removing both would also remove the comment, which may or may not want to be kept. I didn't want to have the discussion here. The only goal here is to cleanup the consensus test.

@fanquake fanquake merged commit 857f07d into bitcoin:master Oct 10, 2022
@maflcko maflcko deleted the 2210-test-no-dummy-🌮 branch October 10, 2022 09:18
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 10, 2022
@fanquake
Merge bitcoin#26277: test: Remove confusing DUMMY_P2WPKH_SCRIPT
b99793e
@ajtowns ajtowns mentioned this pull request Feb 20, 2023
Closed
@bitcoin bitcoin locked and limited conversation to collaborators Oct 10, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@darosior darosior darosior requested changes

@theStack theStack theStack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@maflcko @DrahtBot @instagibbs @fanquake @theStack @darosior