-
Notifications
You must be signed in to change notification settings - Fork 25
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 for default commitments #136
feat: use precomputation for default commitments #136
Conversation
One downside to this implementation is that it generates tables for all provided points, but only checks for the default generators when computing Pedersen commitments. Checking for any combination of the standard basepoint ( Should we only generate a table for the default |
7a03de0
to
1d40ca2
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.
LGTM - one minor comment
602b41f
to
1c897df
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.
ACK
Thanks for adding a bench :)
arr[i] = RISTRETTO_NUMS_POINTS_COMPRESSED[i].decompress().unwrap(); | ||
} | ||
arr | ||
}; | ||
|
||
/// Precomputation table for the first point, which is used as the default commitment generator | ||
pub static ref RISTRETTO_NUMS_TABLE_0: RistrettoBasepointTable = RistrettoBasepointTable::create(&RISTRETTO_NUMS_POINTS[0]); |
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.
If I understand correctly, we only need the first entry of RISTRETTO_NUMS_POINTS
to defined RISTRETTO_NUM_TABLE_0
, why do we need then an array RISTRETTO_NUMS_POINTS_COMPRESSED
of len 10
?
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.
The library already contained this set of points for implementers to use. The default Pedersen factory uses the standard base point G
(from the upstream curve library) and the first point in the set as its generators. The extended Pedersen factories use additional points from the set.
An earlier version of this PR generated precomputation tables for all the points in the set, but it was decided this was wasted computation at this point, since only the default Pedersen factory is used in the codebase (and the extended Pedersen factories use multiscalar multiplication for commitment evaluation, which would require different precomputation table structures and architecture).
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.
@CjS77 had mentioned elsewhere that a future strategy could be to keep some expensive curve point generation operations (like hash-based generation and precomputation) within the commitment factories, and then ensure that these factories are generated only once within the implementing codebase. This is already how the much larger set of range proof generators is handled. However, default Pedersen factory copies are currently generated on the fly more often, presumably because it was cheap to do so.
This work adds precomputation support for Pedersen commitments that use a provided generator. Basepoint tables are created via
lazy_static
alongside the generators themselves. When computing a commitment using the default generators, the corresponding tables are used. Commitments using any other generators are unchanged.Closes issue #135.