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

Reorder verify_ecdsa args to match those of verify_schnorr #751

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shinghim
Copy link
Contributor

@shinghim shinghim commented Oct 12, 2024

The issue (#617) seemed to have good support with 4 thumbs up, so this PR will reorder verify_ecdsa args to match those of verify_schnorr, which just swaps the place of msg and sig. This will break the API, but will potentially cause less confusion in the future

Closes #617

@apoelstra
Copy link
Member

Sure. Looks like dalek uses this order (sorta; the pubkey is self), while Core verifymessage uses pubkey then sig then message. libsodium uses sig, message, key for its crypto_auth_verify method.

OTOH there is no standard here at all so we should just pick one to be consistent within our own library.

apoelstra
apoelstra previously approved these changes Oct 13, 2024
Copy link
Member

@apoelstra apoelstra left a 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

@tcharding
Copy link
Member

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 verify_ecdsa is the other way, presumably it was intuitive to the original author, so maybe my intuition is wrong.

@shinghim
Copy link
Contributor Author

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.

I think the other way is more intuitive, one verifies a signature and to do so one needs the message and pubkey, right?

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

@tcharding
Copy link
Member

Yes, this is @apoelstra's domain, definitely wait for him to chime in. I am not a cryptographer.

@apoelstra
Copy link
Member

I definitely think the schnorr API is more intuitive and that if we want to pick the "best" one we should change ECDSA instead.

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.

@shinghim
Copy link
Contributor Author

I definitely think the schnorr API is more intuitive and that if we want to pick the "best" one we should change ECDSA instead.

Okay, I'm onboard with switching to the schnorr order. I'll update these changes

@shinghim shinghim changed the title Reorder verify_schnorr args to match those of verify_ecdsa Reorder verify_ecdsa args to match those of verify_schnorr Oct 13, 2024
@tcharding
Copy link
Member

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 verify_schnorr we used parameters in the intuitive order sig, msg, key but did not notice that we had a very similar verify_ecdsa function with the parameters in a different order. Since both functions have been released any fix is breaking. We decided to do the annoying fix-now so that users benefit for years to come.

@apoelstra
Copy link
Member

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.

Copy link
Member

@apoelstra apoelstra left a 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

@tcharding
Copy link
Member

tcharding commented Oct 14, 2024

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.
@shinghim
Copy link
Contributor Author

no worries!

Copy link
Member

@apoelstra apoelstra left a 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

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.

verify_ecdsa and verify_schnorr have the args in a different order
3 participants