maidsafe / blsttc

poanetwork/threshold_crypto using supranational/blst for sign+verify
Other
28 stars 20 forks source link

Blstrs #42

Closed iancoleman closed 2 years ago

iancoleman commented 2 years ago

Changes from pairing::bls12_381 0.16.0 crate to blstrs crate.

The benefit is mostly performance improvements.

There are also improvements from changing to standard algorithms, namely hash-to-curve for hash_g2 is IETF standard, and hash-to-field when converting arbitrary bytes to field is IETF standard.

There are two main backwards incompatible changes.

Trivia: bls12_381 crate was previously part of pairing 0.16.0 but was moved out of pairing into a separate crate as of pairing 0.17.0.

The main impact of this change on other code is moving to group 0.11.0 which forces an update from rand 0.7.3 -> 0.8.5. This often has a flow on effect to other dependencies. The last version of group to use rand 0.7 was group 0.8, which is still quite an old version. It seems best to use the latest versions and update rand elsewhere instead of keeping everything at old version for the sake of not updating rand.

dan-da commented 2 years ago

It seems best to use the latest versions and update rand elsewhere instead of keeping everything at old version for the sake of not updating rand

yes! We have been wanting to upgrade to rand 0.8+ in blsttc and dkg. blst_ringct already uses it, and sn_dbc thus has had to use both, which is awkward.

iancoleman commented 2 years ago

Two minor things before this looks good to merge

@davidrusu do you want me to add a note about "no constant time guarantees" in the readme for this PR? I don't think we need it since the original library had no guarantees and neither does this. It'll be good to have it later but for this PR it's not needed imo.

@joshuef should we include the original authors or remove them as per comment at https://github.com/maidsafe/blsttc/pull/42#discussion_r816279714

davidrusu commented 2 years ago

do you want me to add a note about "no constant time guarantees" in the readme for this PR?

@iancoleman I think it's worth calling out in the readme, at the very least it'll make users think about timing side-channel attacks. Just a small note like:

note: this library makes no attempt at ensuring constant time operations and as such may leak information that could be used in a timing side-channel attack.

Let's add it in this PR

dan-da commented 2 years ago

lgtm. We will merge this now, and as I understand it unresolved comments have been made into issues already, so they can be addressed in future PRs.