Skip to content
This repository has been archived by the owner on Mar 11, 2024. It is now read-only.

Remove bulletproofs #168

Open
dcmiddle opened this issue Nov 13, 2020 · 7 comments
Open

Remove bulletproofs #168

dcmiddle opened this issue Nov 13, 2020 · 7 comments

Comments

@dcmiddle
Copy link
Contributor

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.

@dcmiddle dcmiddle assigned hartm and unassigned hartm Nov 13, 2020
@hartm
Copy link
Contributor

hartm commented Nov 13, 2020

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
Copy link
Contributor

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
Copy link

madiazp commented Jan 2, 2021

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
Copy link
Contributor

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
Copy link
Contributor

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

@brentzundel
Copy link
Contributor

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
Copy link
Contributor

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

5 participants