search

filecoin-project / blstrs

Other
58 stars 37 forks source link

Changes to match other bls12_381 libraries #56

Closed mikelodder7 closed 1 year ago

mikelodder7 commented 1 year ago

Added methods to match other bls12-381 libraries (like bls12_381_plus) such as consistent serialization, convenience methods for hex and byte ordering and use const values which is more consistent with Rust-crypto.

vmx commented 1 year ago

Thanks for the PR, it contains a lot of good changes.

Currently blstrs tries to be a drop-in replacement for the bls12_381 crate. Some of your changes are currently incompatible with that.

You maintain the blst12_381_plus fork. For me, one of the key things about open source is collaboration and trying to do what's best for the community/ecosystem. While sometimes forks are needed, I really try to keep them to a minimum. Have you tried to upstream your changes to bls12_381? I think it would be great if we could get those forks together and then making blstrs compatible to that.

mikelodder7 commented 1 year ago

Thanks for the PR, it contains a lot of good changes.

Currently blstrs tries to be a drop-in replacement for the bls12_381 crate. Some of your changes are currently incompatible with that.

You maintain the blst12_381_plus fork. For me, one of the key things about open source is collaboration and trying to do what's best for the community/ecosystem. While sometimes forks are needed, I really try to keep them to a minimum. Have you tried to upstream your changes to bls12_381? I think it would be great if we could get those forks together and then making blstrs compatible to that.

Yes I have in the past but they are locked on version 1.56 and in order to incorporate the new hash_to_curve interfaces in elliptic-curve 0.13, this restriction has to be lifted. Until they do, I can't raise a PR to them. Happy to keep a fork of this repo until then.

mikelodder7 commented 1 year ago

That being said, bls12_381_plus is 98% compatible with bls12_381.

mikelodder7 commented 1 year ago

Going to close for now. Perhaps once bls12_381_plus is merged then I can raise a PR again.

vmx commented 1 year ago

Thanks for the information. I had another look. It's really mostly just additions, so it should be possible to keep compatibility where needed. In regards to the hex encoding/decoding functionality, I'd probably leave those outside of the library, it should be easy enough to keep them outside (I prefer keeping the API surface minimal).

So I'd prefer if you could get changes merged upstream first, but if that turns out to not be possible in the nearer term, feel free to re-open this PR and I'll have yet another look.

daira commented 1 year ago

Yes I have in the past but they are locked on version 1.56 and in order to incorporate the new hash_to_curve interfaces in elliptic-curve 0.13, this restriction has to be lifted. Until they do, I can't raise a PR to them. Happy to keep a fork of this repo until then.

Um, just ask. We only raise the MSRV if there is a reason to, but this would be a reason.

mikelodder7 commented 1 year ago

@daira Done https://github.com/zkcrypto/bls12_381/pull/107/files