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

Public input permutation #62

Merged
merged 4 commits into from
Jan 24, 2023
Merged

Public input permutation #62

merged 4 commits into from
Jan 24, 2023

Conversation

adr1anh
Copy link
Contributor

@adr1anh adr1anh commented Jan 13, 2023

Fixes #28, #22

  • comment-out work queue
  • change sumcheck initialization round to check that the last element is 0 (not necessary now since we don't do ZK, but creates the least amount of change)
  • reinstore gamma challenge

Clang-tidy related

There are many changes that silence warnings from clang-tidy.

  • Initializing class members when possible
  • Explicitly deleting copy constructors/assignment operators for non-copyable classes
  • Using = default for trivial destructors
  • Adding noexcept to move constructors/assignment
  • Using auto after static_cast to avoid duplication.
  • Using std::move when copying an argument like a string or vector to help the compiler (references may alias, so passing by value tells the compiler that this value is really constant)
  • Removing unused headers, and adding #pragma once to prevent import clashes.
  • Reorder some imports, and use the module-relative paths
  • Replace some typedef with using
  • Use const auto& element : container to prevent unnecessary copies.

Circuit Constructor

  • Replace n by num_gates to prevent clashes with n we use to refer to the subgroup_size
  • Added comments about things that seemed fishy, (all marked with TODO(Adrian)
  • Removed NUM_RESERVED_GATES and WireType since they are not used/duplicated
  • Simplified set_public_input, though not sure if the removal of the ASSERT is correct.

Composer

  • Refactor compute_proving_key_base
    • Take the CircuitConstructor by const reference and modify the behavior so we only modify the proving_key. We no longer add any padding or dummy gates to the constructor, and instead do the padding entirely over the polynomials.
    • Remove unnecessary zero-ing out of selector values by using the fact that polynomials are initialized to 0.
    • Explicitly state where the public inputs are stored.
  • Refactor compute_witness_base
    • Use explicit types when referencing the circuit_constructor and ensure we only get objects that we don't modify.
    • Create array of wires to handle the program_width more generally.
    • No longer modify the circuit_constructor wires to add padding, and instead add 0-padding to the wire polynomials.
    • Remove fr::__copy
  • Ensure all calls to circuit_constructor are const
  • PermutationHelper: Big refactor to clarify handling of public inputs.
    • Simplify cycle_node behavior, and more generically handle different number of columns, and remove confusing WireType enum.
    • Make compute_wire_copy_cycles return a vector of CyclicPermutation for clarity.
    • Remove resize by noting that the number of cycles is equal to the number of variables
    • Reverse the order in which we were applying each CyclicPermutation to the sigma polynomials. Now, each cycle_node will map to the next one in the list.
    • Add comments to explain what parts of the function are necessary for our public input handling.
    • Changed the composer_test to also test for public inputs.
      • Add many more tests to ensure the permutation polynomials that we create have the expected form.
      • Compare results with the public_input_delta

Prover & Verifier

  • Comment-out the work_queue related members.
  • Restore gamma challenge that was removed due to a misunderstanding
  • Remove default argument beta = 1 for the grand_product computation.

Sumcheck

  • Add #pragma once to headers
  • Modify the GrandProductComputationRelation to work with Z_perm that has the first coefficient equal to 0.
  • Handle public_input_delta in GrandProductComputationRelation
  • Modify GrandProductInitializationRelation to instead check that the last "real" value of Z_perm_shift is 0. In the current ZK-less situation, this is not necessary since it will be guaranteed by Gemini shift opening. But leaving it here for later.
  • Renamed LAGRANGE_1 to LAGRANGE_FIRST to be consistent with other parts of the code.

@adr1anh adr1anh linked an issue Jan 13, 2023 that may be closed by this pull request
@adr1anh adr1anh force-pushed the ah/perm branch 2 times, most recently from 4413a8b to e3e1dc6 Compare January 20, 2023 13:06
Copy link
Contributor Author

@adr1anh adr1anh left a comment

Choose a reason for hiding this comment

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

Explanations for some changes

@adr1anh adr1anh marked this pull request as ready for review January 20, 2023 14:17
@adr1anh adr1anh requested a review from Rumata888 January 20, 2023 14:17
Copy link
Contributor

@Rumata888 Rumata888 left a comment

Choose a reason for hiding this comment

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

Everything seems great

@adr1anh adr1anh merged commit 124cec0 into master Jan 24, 2023
@adr1anh adr1anh deleted the ah/perm branch January 24, 2023 09:03
dbanks12 pushed a commit that referenced this pull request Jan 26, 2023
## Clang-tidy related

There are many changes that silence warnings from `clang-tidy`. 
- Initializing class members when possible 
- Explicitly deleting copy constructors/assignment operators for non-copyable classes
- Using `= default` for trivial destructors
- Adding `noexcept` to move constructors/assignment
- Using `auto` after `static_cast` to avoid duplication.
- Using `std::move` when copying an argument like a `string` or `vector` to help the compiler (references may alias, so passing by value tells the compiler that this value is really constant)
- Removing unused headers, and adding `#pragma once` to prevent import clashes. 
- Reorder some imports, and use the module-relative paths
- Replace some `typedef` with `using` 
- Use `const auto& element : container` to prevent unnecessary copies.


## Circuit Constructor

-  Replace `n` by `num_gates` to prevent clashes with `n` we use to refer to the `subgroup_size` 
- Added comments about things that seemed fishy, (all marked with `TODO(Adrian)` 
- Removed `NUM_RESERVED_GATES` and `WireType` since they are not used/duplicated
- Simplified `set_public_input`, though not sure if the removal of the `ASSERT` is correct. 

## Composer 

- Refactor `compute_proving_key_base`
  - Take the `CircuitConstructor` by `const` reference and modify the behavior so we only modify the `proving_key`. We no longer add any padding or dummy gates to the constructor, and instead do the padding entirely over the polynomials. 
  - Remove unnecessary zero-ing out of selector values by using the fact that polynomials are initialized to 0. 
  - Explicitly state where the public inputs are stored. 
- Refactor `compute_witness_base`
  - Use explicit types when referencing the `circuit_constructor` and ensure we only get objects that we don't modify. 
  - Create `array` of wires to handle the `program_width` more generally. 
  - No longer modify the `circuit_constructor` wires to add padding, and instead add 0-padding to the wire polynomials. 
  - Remove `fr::__copy` 
- Ensure all calls to `circuit_constructor` are `const` 
- PermutationHelper: Big refactor to clarify handling of public inputs. 
  - Simplify `cycle_node` behavior, and more generically handle different number of columns, and remove confusing `WireType` enum. 
  - Make `compute_wire_copy_cycles` return a vector of `CyclicPermutation` for clarity. 
  - Remove `resize` by noting that the number of cycles is equal to the number of `variables`
  - Reverse the order in which we were applying each `CyclicPermutation` to the `sigma` polynomials. Now, each `cycle_node` will map to the next one in the list. 
  - Add comments to explain what parts of the function are necessary for our public input handling. 
  - Changed the `composer_test` to also test for public inputs. 
    - Add many more tests to ensure the permutation polynomials that we create have the expected form. 
    - Compare results with the `public_input_delta`

## Prover & Verifier

- Comment-out the `work_queue` related members. 
- Restore `gamma` challenge that was removed due to a misunderstanding
- Remove default argument `beta = 1` for the `grand_product` computation.

## Sumcheck 

- Add `#pragma once` to headers
- Modify the `GrandProductComputationRelation` to work with `Z_perm` that has the first coefficient equal to 0. 
- Handle `public_input_delta` in `GrandProductComputationRelation` 
- Modify `GrandProductInitializationRelation` to instead check that the last "real" value of `Z_perm_shift` is 0. In the current ZK-less situation, this is not necessary since it will be guaranteed by Gemini shift opening. But leaving it here for later. 
- Renamed `LAGRANGE_1` to `LAGRANGE_FIRST` to be consistent with other parts of the code.
dbanks12 pushed a commit that referenced this pull request Jan 27, 2023
## Clang-tidy related

There are many changes that silence warnings from `clang-tidy`. 
- Initializing class members when possible 
- Explicitly deleting copy constructors/assignment operators for non-copyable classes
- Using `= default` for trivial destructors
- Adding `noexcept` to move constructors/assignment
- Using `auto` after `static_cast` to avoid duplication.
- Using `std::move` when copying an argument like a `string` or `vector` to help the compiler (references may alias, so passing by value tells the compiler that this value is really constant)
- Removing unused headers, and adding `#pragma once` to prevent import clashes. 
- Reorder some imports, and use the module-relative paths
- Replace some `typedef` with `using` 
- Use `const auto& element : container` to prevent unnecessary copies.


## Circuit Constructor

-  Replace `n` by `num_gates` to prevent clashes with `n` we use to refer to the `subgroup_size` 
- Added comments about things that seemed fishy, (all marked with `TODO(Adrian)` 
- Removed `NUM_RESERVED_GATES` and `WireType` since they are not used/duplicated
- Simplified `set_public_input`, though not sure if the removal of the `ASSERT` is correct. 

## Composer 

- Refactor `compute_proving_key_base`
  - Take the `CircuitConstructor` by `const` reference and modify the behavior so we only modify the `proving_key`. We no longer add any padding or dummy gates to the constructor, and instead do the padding entirely over the polynomials. 
  - Remove unnecessary zero-ing out of selector values by using the fact that polynomials are initialized to 0. 
  - Explicitly state where the public inputs are stored. 
- Refactor `compute_witness_base`
  - Use explicit types when referencing the `circuit_constructor` and ensure we only get objects that we don't modify. 
  - Create `array` of wires to handle the `program_width` more generally. 
  - No longer modify the `circuit_constructor` wires to add padding, and instead add 0-padding to the wire polynomials. 
  - Remove `fr::__copy` 
- Ensure all calls to `circuit_constructor` are `const` 
- PermutationHelper: Big refactor to clarify handling of public inputs. 
  - Simplify `cycle_node` behavior, and more generically handle different number of columns, and remove confusing `WireType` enum. 
  - Make `compute_wire_copy_cycles` return a vector of `CyclicPermutation` for clarity. 
  - Remove `resize` by noting that the number of cycles is equal to the number of `variables`
  - Reverse the order in which we were applying each `CyclicPermutation` to the `sigma` polynomials. Now, each `cycle_node` will map to the next one in the list. 
  - Add comments to explain what parts of the function are necessary for our public input handling. 
  - Changed the `composer_test` to also test for public inputs. 
    - Add many more tests to ensure the permutation polynomials that we create have the expected form. 
    - Compare results with the `public_input_delta`

## Prover & Verifier

- Comment-out the `work_queue` related members. 
- Restore `gamma` challenge that was removed due to a misunderstanding
- Remove default argument `beta = 1` for the `grand_product` computation.

## Sumcheck 

- Add `#pragma once` to headers
- Modify the `GrandProductComputationRelation` to work with `Z_perm` that has the first coefficient equal to 0. 
- Handle `public_input_delta` in `GrandProductComputationRelation` 
- Modify `GrandProductInitializationRelation` to instead check that the last "real" value of `Z_perm_shift` is 0. In the current ZK-less situation, this is not necessary since it will be guaranteed by Gemini shift opening. But leaving it here for later. 
- Renamed `LAGRANGE_1` to `LAGRANGE_FIRST` to be consistent with other parts of the code.
ludamad pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jul 22, 2023
## Clang-tidy related

There are many changes that silence warnings from `clang-tidy`. 
- Initializing class members when possible 
- Explicitly deleting copy constructors/assignment operators for non-copyable classes
- Using `= default` for trivial destructors
- Adding `noexcept` to move constructors/assignment
- Using `auto` after `static_cast` to avoid duplication.
- Using `std::move` when copying an argument like a `string` or `vector` to help the compiler (references may alias, so passing by value tells the compiler that this value is really constant)
- Removing unused headers, and adding `#pragma once` to prevent import clashes. 
- Reorder some imports, and use the module-relative paths
- Replace some `typedef` with `using` 
- Use `const auto& element : container` to prevent unnecessary copies.


## Circuit Constructor

-  Replace `n` by `num_gates` to prevent clashes with `n` we use to refer to the `subgroup_size` 
- Added comments about things that seemed fishy, (all marked with `TODO(Adrian)` 
- Removed `NUM_RESERVED_GATES` and `WireType` since they are not used/duplicated
- Simplified `set_public_input`, though not sure if the removal of the `ASSERT` is correct. 

## Composer 

- Refactor `compute_proving_key_base`
  - Take the `CircuitConstructor` by `const` reference and modify the behavior so we only modify the `proving_key`. We no longer add any padding or dummy gates to the constructor, and instead do the padding entirely over the polynomials. 
  - Remove unnecessary zero-ing out of selector values by using the fact that polynomials are initialized to 0. 
  - Explicitly state where the public inputs are stored. 
- Refactor `compute_witness_base`
  - Use explicit types when referencing the `circuit_constructor` and ensure we only get objects that we don't modify. 
  - Create `array` of wires to handle the `program_width` more generally. 
  - No longer modify the `circuit_constructor` wires to add padding, and instead add 0-padding to the wire polynomials. 
  - Remove `fr::__copy` 
- Ensure all calls to `circuit_constructor` are `const` 
- PermutationHelper: Big refactor to clarify handling of public inputs. 
  - Simplify `cycle_node` behavior, and more generically handle different number of columns, and remove confusing `WireType` enum. 
  - Make `compute_wire_copy_cycles` return a vector of `CyclicPermutation` for clarity. 
  - Remove `resize` by noting that the number of cycles is equal to the number of `variables`
  - Reverse the order in which we were applying each `CyclicPermutation` to the `sigma` polynomials. Now, each `cycle_node` will map to the next one in the list. 
  - Add comments to explain what parts of the function are necessary for our public input handling. 
  - Changed the `composer_test` to also test for public inputs. 
    - Add many more tests to ensure the permutation polynomials that we create have the expected form. 
    - Compare results with the `public_input_delta`

## Prover & Verifier

- Comment-out the `work_queue` related members. 
- Restore `gamma` challenge that was removed due to a misunderstanding
- Remove default argument `beta = 1` for the `grand_product` computation.

## Sumcheck 

- Add `#pragma once` to headers
- Modify the `GrandProductComputationRelation` to work with `Z_perm` that has the first coefficient equal to 0. 
- Handle `public_input_delta` in `GrandProductComputationRelation` 
- Modify `GrandProductInitializationRelation` to instead check that the last "real" value of `Z_perm_shift` is 0. In the current ZK-less situation, this is not necessary since it will be guaranteed by Gemini shift opening. But leaving it here for later. 
- Renamed `LAGRANGE_1` to `LAGRANGE_FIRST` to be consistent with other parts of the code.
ludamad pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jul 24, 2023
## Clang-tidy related

There are many changes that silence warnings from `clang-tidy`. 
- Initializing class members when possible 
- Explicitly deleting copy constructors/assignment operators for non-copyable classes
- Using `= default` for trivial destructors
- Adding `noexcept` to move constructors/assignment
- Using `auto` after `static_cast` to avoid duplication.
- Using `std::move` when copying an argument like a `string` or `vector` to help the compiler (references may alias, so passing by value tells the compiler that this value is really constant)
- Removing unused headers, and adding `#pragma once` to prevent import clashes. 
- Reorder some imports, and use the module-relative paths
- Replace some `typedef` with `using` 
- Use `const auto& element : container` to prevent unnecessary copies.


## Circuit Constructor

-  Replace `n` by `num_gates` to prevent clashes with `n` we use to refer to the `subgroup_size` 
- Added comments about things that seemed fishy, (all marked with `TODO(Adrian)` 
- Removed `NUM_RESERVED_GATES` and `WireType` since they are not used/duplicated
- Simplified `set_public_input`, though not sure if the removal of the `ASSERT` is correct. 

## Composer 

- Refactor `compute_proving_key_base`
  - Take the `CircuitConstructor` by `const` reference and modify the behavior so we only modify the `proving_key`. We no longer add any padding or dummy gates to the constructor, and instead do the padding entirely over the polynomials. 
  - Remove unnecessary zero-ing out of selector values by using the fact that polynomials are initialized to 0. 
  - Explicitly state where the public inputs are stored. 
- Refactor `compute_witness_base`
  - Use explicit types when referencing the `circuit_constructor` and ensure we only get objects that we don't modify. 
  - Create `array` of wires to handle the `program_width` more generally. 
  - No longer modify the `circuit_constructor` wires to add padding, and instead add 0-padding to the wire polynomials. 
  - Remove `fr::__copy` 
- Ensure all calls to `circuit_constructor` are `const` 
- PermutationHelper: Big refactor to clarify handling of public inputs. 
  - Simplify `cycle_node` behavior, and more generically handle different number of columns, and remove confusing `WireType` enum. 
  - Make `compute_wire_copy_cycles` return a vector of `CyclicPermutation` for clarity. 
  - Remove `resize` by noting that the number of cycles is equal to the number of `variables`
  - Reverse the order in which we were applying each `CyclicPermutation` to the `sigma` polynomials. Now, each `cycle_node` will map to the next one in the list. 
  - Add comments to explain what parts of the function are necessary for our public input handling. 
  - Changed the `composer_test` to also test for public inputs. 
    - Add many more tests to ensure the permutation polynomials that we create have the expected form. 
    - Compare results with the `public_input_delta`

## Prover & Verifier

- Comment-out the `work_queue` related members. 
- Restore `gamma` challenge that was removed due to a misunderstanding
- Remove default argument `beta = 1` for the `grand_product` computation.

## Sumcheck 

- Add `#pragma once` to headers
- Modify the `GrandProductComputationRelation` to work with `Z_perm` that has the first coefficient equal to 0. 
- Handle `public_input_delta` in `GrandProductComputationRelation` 
- Modify `GrandProductInitializationRelation` to instead check that the last "real" value of `Z_perm_shift` is 0. In the current ZK-less situation, this is not necessary since it will be guaranteed by Gemini shift opening. But leaving it here for later. 
- Renamed `LAGRANGE_1` to `LAGRANGE_FIRST` to be consistent with other parts of the code.
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.

Update grand product relation Handle public inputs in grand product in Honk PoC
2 participants