-
Notifications
You must be signed in to change notification settings - Fork 270
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
Reorder verify_ecdsa args to match those of verify_schnorr #751
base: master
Are you sure you want to change the base?
Conversation
Sure. Looks like dalek uses this order (sorta; the pubkey is OTOH there is no standard here at all so we should just pick one to be consistent within our own library. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 2d7cc5e successfully ran local tests
Totally bikeshedding, please feel free to merge around me. I think the other way is more intuitive, one verifies a signature and to do so one needs the message and pubkey, right? But that begs the question why |
I don't have a preference for the order, but I'm very new to crypto APIs so don't have an intuition on how it should be. I think it's more important that they're in the same order.
This makes sense to me, and it's no problem at all to update these changes. It's probably worth discussing a bit before deciding so we only change it once |
Yes, this is @apoelstra's domain, definitely wait for him to chime in. I am not a cryptographer. |
I definitely think the But OTOH the ECDSA API is probably more widely used and likely to annoy people if we break it. I'd be happy with either change. |
Okay, I'm onboard with switching to the |
2d7cc5e
to
3c8f5e0
Compare
Can you write a longer commit message please because over time downstream users are inevitably going to whinge about this, when they git blame it would be nice if they saw our reasoning. Something like When we introduced |
Will ACK this -- but @shinghim I would appreciate if you expanded the commit message, and I'll hold off on merging till you take a look at @tcharding's comment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 3c8f5e0 successfully ran local tests
3c8f5e0
to
0f9a513
Compare
Can you wrap the columns in the git commit log to 72 characters please - sorry to be picky. |
When `verify_schnorr` was first introduced, it used parameters in the inuitive order: sig, msg, key. It was later noticed that `verify_ecdsa` had the same parameters but in a different order. Since both functions have been released, this will be a breaking change, but fixing this now will add consistency and remove any confusion for users in the future.
0f9a513
to
51a954b
Compare
no worries! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK 51a954b successfully ran local tests
The issue (#617) seemed to have good support with 4 thumbs up, so this PR will reorder
verify_ecdsa
args to match those ofverify_schnorr
, which just swaps the place ofmsg
andsig
. This will break the API, but will potentially cause less confusion in the futureCloses #617