-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Use a single committer_key and fft_precomputation for batch of proofs #1955
base: testnet3
Are you sure you want to change the base?
Conversation
530607d
to
1eec428
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.
I think we should change the API of to_universal_prover
to take in a struct (maybe DegreeInfo
?) containing the relevant information; as it stands we're just extracting some usize
s from other structs and then passing these into to_universal_prover
, which is creating a lot of repetition and room for errors stemming from accidentally switching the order of arguments.
Agreed, in the past I also lost half an hour testing a bug caused by false ordering. 3383c94 Also worth noting: I renamed |
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.
Made some comments about the web/wasm targets. The tl;dr of it is that I conjecture that web & wasm targets will make up an outsized portion of users proving with Aleo's implementation of Varuna (people like to use JavaScript, Rust is difficult for most application developers).
Therefore that there should be some asynchronous parameter download methods baked into enclosing structs which construct the UniversalProver & Universal verifiers. An example method for which to fetch these parameters is proposed here: https://github.com/AleoHQ/snarkVM/pull/1987/files#diff-d907035b6432d9b2ef95015b23710393694a19b111dfffc195499ff3da02c3bcR112-R126
This PR provides good decoupling and actually will make it easier to get parameters on the web, but we should ensure we include it so that web developers are fully supported prior to mainnet.
For other reviewers reading this, we talked about it elsewhere and for now this PR can stay as is - PR #1987 will be rebased on this one. |
9415643
to
3b3ae6f
Compare
I think we should roll supported Lagrange sizes and hiding_bound also into DegreeInfo |
I would vote against that because we have tests which generate lagrange_sizes/hiding_bound independently from CircuitInfo. Both ways of doing does not seem obviously better and does not seem worth the (minor) refactor:
|
3b3ae6f
to
e3976b0
Compare
In any real world use case, Also, the maximum supported degree of the SRS can depend on whether or not the committed polynomials are hiding. So logically |
I would also be fine if |
That makes sense, looks a lot cleaner now, also removed the |
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 modulo couple of small changes
.to_universal_prover() | ||
.expect("Failed to convert universal SRS (KZG10) to the prover.") | ||
}) | ||
fn varuna_universal_prover(degree_info: DegreeInfo) -> UniversalProver<Self::PairingCurve> { |
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.
I see that we can no longer keep UniversalProver
as a 'static
, but what about the UniversalParams
? those could likely be made 'static
so that we don't need to load them for every call to ProvingKey::{prove, prove_batch}
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.
Good idea! This would be a somewhat involved change so not sure I can get it in before the code freeze, will tackle in a new PR.
Currently, every time we create a proof, we indeed create UniversalParams
and grow this according to the circuit size. For a simple transfer it grows to ~ 50MB. But it could grow to gigabytes. If we were to make the object static, we'll also need to add a method to shrink the parameters to our minimum parameter size (~ 10MB) so they don't take up memory forever.
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 with a suggestion regarding UniversalParams
Reduce duplication of universal provers
5e816a2
to
ebcb62a
Compare
Motivation
To reduce memory and computation for batch proofs, we should only have to use a single
committer_key
andfft_precomputation
for the biggest circuit of a batch proof. Previously we were computing these for every circuit. For a large batch this reduces the size of theProvingKey
by around 40%. We also don't needCommitterUnionKey
anymore which was collecting the union of the committer keys.Test Plan
The existing test suite seems to suffice.