rust-bitcoin / rust-secp256k1

Rust language bindings for Bitcoin secp256k1 library.
Creative Commons Zero v1.0 Universal
350 stars 270 forks source link

Reorder verify_ecdsa args to match those of verify_schnorr #751

Open shinghim opened 1 month ago

shinghim commented 1 month ago

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 commented 1 month ago

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.

tcharding commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

apoelstra commented 1 month ago

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 commented 1 month ago

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

tcharding commented 1 month ago

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 commented 1 month ago

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.

tcharding commented 1 month ago

Can you wrap the columns in the git commit log to 72 characters please - sorry to be picky.

shinghim commented 1 month ago

no worries!