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.

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

txscript: Tighten standardness pubkey checks. #1649

Merged
merged 1 commit into from Mar 12, 2019

Conversation

davecgh
Copy link
Member

@davecgh davecgh commented Mar 9, 2019

This tightens the multisig and pay-to-pubkey standard script identification functions to use the same strict pubkey requirements as the consensus rules since standardness rules are generally intended to be more restrictive than the consensus rules which implies they are at a minimum at least as restrictive.

The tests are also updated to deal with the additional restriction accordingly.

@davecgh davecgh added this to the 1.5.0 milestone Mar 9, 2019
@davecgh davecgh force-pushed the txscript_tighten_pubkey_standardness branch 2 times, most recently from 49f8a9e to c18bada Compare March 9, 2019 03:10
Copy link
Member

@matheusd matheusd left a comment

Choose a reason for hiding this comment

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

Just a small comment: other places in the code using the same naming style (Is*()/Check*()) usually have the Check() function return a more specific error while the Is() function just checks whether there is an error (eg IsSSTx vs CheckSSTX).

Do you think it's worth following the same style here or are the possible errors for pubkey so simple and obvious that it doesn't really make a difference?

@davecgh
Copy link
Member Author

davecgh commented Mar 11, 2019

It's a fair point, although in this particular case, I'd rather not do that for a couple of reasons:

  • This existing error has already been there for years and has tests in the reference data which specifically look for it
  • This change is really also part of a much bigger change that I've been working on which significantly optimizes the entire script engine and returning an error that gets thrown away is counter to that goal and even creates an allocation due to the fact it's an interface

As an aside, I wouldn't use the stake package as an example of code representative of dcrd in general since it, unfortunately generally does not follow the style used in the rest of code and ultimately needs to be significantly cleaned up as a result.

This tightens the multisig and pay-to-pubkey standard script
identification functions to use the same strict pubkey requirements as
the consensus rules since standardness rules are generally intended to
be more restrictive than the consensus rules which implies they are at a
minimum at least as restrictive.

The tests are also updated to deal with the additional restriction
accordingly.
@davecgh davecgh force-pushed the txscript_tighten_pubkey_standardness branch from c18bada to a729ce2 Compare March 12, 2019 00:33
Copy link
Member

@matheusd matheusd left a comment

Choose a reason for hiding this comment

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

Ok, thanks for the response!

@davecgh davecgh merged commit a729ce2 into decred:master Mar 12, 2019
@davecgh davecgh deleted the txscript_tighten_pubkey_standardness branch March 12, 2019 15:12
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.

None yet

4 participants