search

hyperledger-archives / ursa

Hyperledger Ursa (a shared cryptographic library) has moved to end-of-life status, with the components of Ursa still in use moved to their relevant Hyperledger projects (AnonCreds, Indy, Aries and Iroha).
https://wiki.hyperledger.org/display/ursa
Apache License 2.0
321 stars 142 forks source link

Remove bulletproofs #168

Open dcmiddle opened 3 years ago

dcmiddle commented 3 years ago

I'm uncomfortable with the bulletproofs module for both subjective and objective reasons. When it was originally authored there were issues with the licensing (i.e. it was copied from the Dalek project) which I think were remedied. However I'm not 100% confident. That's not a concrete criticism but I don't know how to make it concrete without revisiting the full review which seems to have allowed commits like this through: Dont push yet. Some comments, refactorings and TODOs. Some refactoings

Moreover since it copies rather than imports/links to Dalek it is not actively incorporating changes from Dalek.

More concretely the feature also relies on a defunct crypto project, Apache Milagro Crypto Library: https://github.com/milagro-crypto/amcl That library seems to have changed hands or location and may only have a single maintainer. Relying (for security in particular) on single maintainer projects is contrary to best practices.

At the very least this module should be marked as experimental. That was sort of implicitly the case under the zmix code structure, but as we restructure the code this and other modules will move to a different structure that won't implicitly reflect the maturity of the feature. I would prefer however that it be removed until it can be actively maintained to address the concerns above.

hartm commented 3 years ago

Do we know who is using this? That should probably be a starting point.

We are already working on replacing Milagro in many pairing-based primitives with pairing-plus (https://github.com/algorand/pairing-plus), although it looks like there may be licensing issues (they are MIT). The main reason for this is performance, though--pairing-plus is much faster.

I agree that this feature should be marked as experimental--and so should others in Ursa. This is something we should discuss further as a part of the refactor.

brentzundel commented 3 years ago

I was thinking I would use it as part of the Merkle tree revocation implementation, but if there are concerns about it, then maybe we can discuss option on the next call.

madiazp commented 3 years ago

So what's the status on this? I'm interested in using this lib specifically for the gadgets and it's seems more maintained that the dalek's one. This concern is also because there are dependencies that needs to be updated. For example the current version of amcl_wrapper(0.4.0) is not compatible with this repo.

brentzundel commented 3 years ago

The main problem (as I understand it) is that this library isn't as well-maintained as Dalek's. It was written as a near duplicate of Dalek, but for a different curve. Then Dalek continued to be developed where this one hasn't, so they have diverged.

brentzundel commented 3 years ago

FWIW I don't think we should remove bulletproofs from Ursa, but I do think we should label them as experimental.

brentzundel commented 1 year ago

After conversation in 26 Oct meeting, the consensus is to remove the current bulletproofs code from Ursa and look into the possibility of contributing the Ursa BLS 12-381 version of bulletproofs to the Dalek library.

mikelodder7 commented 1 year ago

I’ve tried. Here’s the repo https://github.com/cryptidtech/bulletproofs