-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: use precomputation on (most) fixed generators #19
Conversation
594be43
to
8283627
Compare
I think you will struggle to get that PR into dalek, but worth a try. Looks ok at first glance otherwise |
Agreed that it seems very unlikely for an upstream PR to be accepted, and I didn't plan on doing one. I think it would need to remain a fork. However, keeping a maintained fork would also allow for more flexibility on future changes to the curve library. |
8283627
to
d8e7506
Compare
d8e7506
to
815264a
Compare
This will be ready to review once a curve library PR is merged and tagged, at which point this PR can use it as a dependency. |
@AaronFeickert I've merged the curve library PR and created a tag |
f7e26fb
to
8ab22bd
Compare
8ab22bd
to
bdd9b9b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good.
I am mostly happy with it except for the copy. Expensive types should not have copy as an implementation.
The reason is two-fold:
- Types that impl copy are not moved when used in a function, they are copied
- If a type impl copy it hides expensive copying by always doing mem copy and not doing a move which is more desirable and or pointing out to the dev that a clone is happening here,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is more going in the right direction
cda39f3
to
737d665
Compare
Currently, bit vector commitments are computed using [multiscalar multiplication](https://github.com/tari-project/bulletproofs-plus/blob/cd7588ee8eaebe862fe9cf5d7c3fd92981703e87/src/range_proof.rs#L265-L273) against the inner-product generators. However, this can be simplified by using the shiny new [generator precomputation](#19) feature.
This is now an upstream PR. |
Update the version ## [0.3.0](v0.2.3...v0.3.0) (2023-07-13) ## ⚠ BREAKING CHANGES * Changes the way that seed nonces are used in mask recovery. Existing range proofs will verify, but will fail to recover the correct mask. ## Features * simplify bit vector commitment ([35](#35)) ([f831d64](f831d64)), closes [/github.com/tari-project/bulletproofs-plus/blob/cd7588ee8eaebe862fe9cf5d7c3fd92981703e87/src/range_proof.rs#L265-L273](https://github.com/tari-project//github.com/tari-project/bulletproofs-plus/blob/cd7588ee8eaebe862fe9cf5d7c3fd92981703e87/src/range_proof.rs/issues/L265-L273) * use precomputation on (most) fixed generators ([19](#19)) ([cd7588e](cd7588e)), closes [#18](#18) ## Bug Fixes * nonce index encoding ([31](#31)) ([394843f](394843f))
Uses precomputation of fixed generator vectors to speed up verification, at the cost of storing precomputation tables between verification operations. This doesn't use precomputation on the Pedersen generators, since those can be set independently of the others, and we can't mix-and-match precomputation tables due to upstream limitations.
Note that this requires and uses a custom curve library fork. The fork supports partial precomputation by removing an existing restriction about matching the number of static points and scalars used for precomputation evaluation. It also implements
Clone
on the underlying types used for precomputation. This is unfortunate, since due to their size (several megabytes in total) such tables should almost certainly never be cloned. However, it's done for the reason explained below.The generator tables are wrapped in an
Arc
for shared ownership. This is done because precomputation evaluation is an instance method on a precomputation type, not a static method that takes a reference to the underlying tables. I have no idea why this design was chosen (static methods are used for other types of multiscalar multiplication), especially because there's no mutation involved. But because of this, the verifier needs to own the precomputation structure containing the tables, even though those tables are expected to be reused (that's the entire point of precomputation). Using anArc
takes care of this nicely, and avoids cloning.However, apparently
#[derive(Clone)]
only plays nicely with structs if all included generic types implementClone
, which means even though cloning the tableArc
isn't any kind of deep copy, we can't use that attribute unless the precomputation tables implementClone
. Manually implementingClone
on the containing struct is a headache, so it seemed easier just to add#[derive(Clone)]
at the curve library level.This means it's probably very important to ensure that precomputation tables are used very carefully to avoid unintended cloning. I did some testing and confirmed that the current implementation handles this as expected, and won't clone any of the tables, despite the compiler requiring they implement
Clone
.Closes issue #18.