Skip to content

txscript: Correct nulldata standardness check.#935

Merged
davecgh merged 2 commits intodecred:masterfrom
dnldd:merge_txscript_correct_isnulldata_standardness_check
Dec 1, 2017
Merged

txscript: Correct nulldata standardness check.#935
davecgh merged 2 commits intodecred:masterfrom
dnldd:merge_txscript_correct_isnulldata_standardness_check

Conversation

@dnldd
Copy link
Copy Markdown
Member

@dnldd dnldd commented Nov 30, 2017

This PR contains the following upstream commits:


This corrects the isNullData standard transaction type test to work properly with canonically-encoded data pushes. In particular, single byte data pushes that are small integers (0-16) are converted to the
equivalent numeric opcodes when canonically encoded and the code failed to detect them properly.

It also adds several tests to ensure that both canonical and non-canonical nulldata scripts are recognized properly and modifies the test failure print to include the script that failed.

This does not affect consensus since it is just a standardness check.

This corrects the isNullData standard transaction type test to work
properly with canonically-encoded data pushes.  In particular, single
byte data pushes that are small integers (0-16) are converted to the
equivalent numeric opcodes when canonically encoded and the code failed
to detect them properly.

It also adds several tests to ensure that both canonical and
non-canonical nulldata scripts are recognized properly and modifies the
test failure print to include the script that failed.

This does not affect consensus since it is just a standardness check.
@davecgh
Copy link
Copy Markdown
Member

davecgh commented Dec 1, 2017

That standardness rules for nulldata are not exactly the same in Decred. You'll need to examine them and update this accordingly as a part of the merge commit.

In particular the MaxDataCarrierSize is larger.

@dnldd dnldd force-pushed the merge_txscript_correct_isnulldata_standardness_check branch 2 times, most recently from 4fc2c5b to 69735b7 Compare December 1, 2017 00:29
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems to have lost the updates from the upstream commit to this comment.

Copy link
Copy Markdown
Member Author

@dnldd dnldd Dec 1, 2017

Choose a reason for hiding this comment

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

maintained the script for that test because the one from upstream was categorized as nulldata. I suspect it's because the script doesn't exceed MaxDataCarrierSize.

--- FAIL: TestScriptClass (0.00s) standard_test.go:996: nulldata exceed max standard push: expected nonstandard got nulldata (script 6a4c51046708afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef3046708afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef308)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Right, but the comment update itself still applies.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

oh right, updating.

@davecgh
Copy link
Copy Markdown
Member

davecgh commented Dec 1, 2017

I updated the description to include the upstream PR description as well since it makes it much easier for reviewers.

@dnldd
Copy link
Copy Markdown
Member Author

dnldd commented Dec 1, 2017

thanks, will remember to do that next time.

@dnldd dnldd force-pushed the merge_txscript_correct_isnulldata_standardness_check branch from 69735b7 to 2d0d424 Compare December 1, 2017 01:24
@dnldd
Copy link
Copy Markdown
Member Author

dnldd commented Dec 1, 2017

Updated.

@davecgh davecgh merged commit 2d0d424 into decred:master Dec 1, 2017
@dnldd dnldd deleted the merge_txscript_correct_isnulldata_standardness_check branch June 4, 2018 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants