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

Bump secp256k1 to 0.19, use global context #438

Merged
merged 4 commits into from Nov 13, 2020
Merged

Bump secp256k1 to 0.19, use global context #438

merged 4 commits into from Nov 13, 2020

Conversation

vorot93
Copy link
Contributor

@vorot93 vorot93 commented Nov 7, 2020

No description provided.

dvdplm
dvdplm previously approved these changes Nov 9, 2020
View reviewed changes
@dvdplm
Copy link
Contributor

dvdplm commented Nov 9, 2020

Needs cargo fmt run.

The test failure is actually relevant: all-zero messages are not invalid anymore. I think this is alright for ethereum but my memory is weak and I don't remember the details tbh.

@dvdplm dvdplm dismissed their stale review November 9, 2020 08:32

Jumped the gun, sorry!

@niklasad1
Copy link
Member

niklasad1 commented Nov 9, 2020 •
edited

The test failure is actually relevant: all-zero messages are not invalid anymore. I think this is alright for ethereum but my memory is weak and I don't remember the details tbh.

rust-bitcoin/rust-secp256k1#207 is included in this release. I think you are right.

Do you remember the ethereum ecrecover builtin bug :)?

//cc @sorpaas I guess you know this ^^

@ordian
Copy link
Member

ordian commented Nov 9, 2020

yeah, I think we should remove the failing test and ZeroesAllowedMessage or do you have another suggestion? :)

@niklasad1
Copy link
Member

niklasad1 commented Nov 9, 2020 •
edited

yeah, I think we should remove the failing test and ZeroesAllowedMessage or do you have another suggestion? :)

I completely agree to remove the failing test.

@sorpaas
Copy link
Member

sorpaas commented Nov 9, 2020 •
edited

@ordian @niklasad1 I'd propose we change this failing test to be "test zero message is valid".

I'll let the OpenEthereum people know about this just in case.

dvdplm
dvdplm approved these changes Nov 12, 2020
View reviewed changes
Copy link
Contributor

@dvdplm dvdplm left a comment

Choose a reason for hiding this comment

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

LGTM. Can you please add this to the CHANGELOG as a breaking change? :)

@vorot93
Copy link
Contributor Author

vorot93 commented Nov 12, 2020

@dvdplm done

@vorot93
Copy link
Contributor Author

vorot93 commented Nov 13, 2020

@dvdplm @ordian @sorpaas should be ready for merge

@niklasad1 niklasad1 merged commit 539a279 into paritytech:master Nov 13, 2020
@vorot93 vorot93 deleted the bump-secp256k1 branch November 13, 2020 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dvdplm dvdplm dvdplm approved these changes

@ordian ordian ordian approved these changes

@sorpaas sorpaas sorpaas approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@vorot93 @dvdplm @niklasad1 @ordian @sorpaas