Skip to content
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

Fix linked proof commitments #342

Merged
merged 2 commits into from
Feb 7, 2024
Merged

Fix linked proof commitments #342

merged 2 commits into from
Feb 7, 2024

Conversation

samtay
Copy link
Contributor

@samtay samtay commented Feb 6, 2024

The current graph traversal logic is depth first. Thus on the ZKP side, the multiplier gates corresponding to the inputs will not be contiguous as they are allocated into the prover's Secret::{a_L,a_R}. In fact, determining the location of the input gates is not generally feasible (any multiplications performed with said inputs are going to be evaluated during the graph traversal as soon as those inputs are .. inputted).

This PR changes the graph traversal to be breadth first, so we allocate all the inputs first, in one contiguous block, and then we handle any downstream multiplications in the circuit.

Furthermore, since we're using a VecDeque to deque ready nodes, we may as well pop them from the front. This allows for a more intuitive traversal of the inputs in order, so we don't need to handle any reversal or odd pending final multiplier case when constructing the shared generators.

The tests pass, so it seems we didn't have any correctness assumptions on this traversal. But this change is a low level detail of the compiler to both FHE and ZKP, so we ought to think carefully about any ramifications. There could be performance impacts to FHE/ZKP program compilation. If desired we could diverge the FHE/ZKP compilation so that only ZKP uses the breadth first approach, but only if there is a good reason to do so.

The current graph traversal logic is depth first. Thus on the ZKP side,
the multiplier gates corresponding to the inputs will not be contiguous
as they are allocated into the prover's `Secret::{a_L,a_R}`.
In fact, determining the location of the input gates is not generally
feasible (any multiplications performed with said inputs are going to be
evaluated during the graph traversal as soon as those inputs are ..
inputted). If we instead change the graph traversal to be breadth first,
then we will allocate all the inputs first, in one contiguous block, and
_then_ we handle any downstream multiplications in the circuit.

Furthermore, since we're using a `VecDeque` to deque ready nodes, we may
as well pop them from the front. This allows for a more intuitive
traversal of the inputs in order, so we don't need to handle any
reversal or odd pending final multiplier case when constructing the
shared generators.

The tests pass, so it seems we didn't have any correctness assumptions
on this traversal.
Copy link
Contributor

@ryanorendorff ryanorendorff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll let Rick comment on the traversal change but otherwise looks good. Glad the generator insertion logic is simpler now too.

@samtay samtay merged commit c3cbf37 into main Feb 7, 2024
4 checks passed
@samtay samtay deleted the samtay/fix-shared-inputs branch February 7, 2024 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants