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

feat: Support public inputs in Ultra Honk #581

Merged
merged 13 commits into from
Jul 18, 2023
Merged

Conversation

ledwards2225
Copy link
Collaborator

@ledwards2225 ledwards2225 commented Jul 5, 2023

Description

The primary goal of this PR is to support public inputs in Ultra Honk. This was previously not supported because adding a non-zero public input to an UH circuit would result in wires with a non-zero first entry, making them not shiftable. The solution is to add a "zero row" at the start of the execution trace for all UH circuits. The ordering of the execution trace is thus: zero row, public inputs, conventional gates.

A portion of the work in this PR is simply cleanup that was necessary to limit the number of places in the code where we need to specify the "zero row offset". For example, this offset has to be taken into account in the computation of the dyadic circuit size, which was previously computed independently in 3 separate locations within the Honk composers.

Note about implementation: In general there are many "zero rows" in every circuit, i.e. the padding between the "filled" rows of the execution trace and the dyadic (power of 2) circuit size. For this reason, 0 is added as a variable in the circuit builders by default. To add a zero row at the start of the execution trace, I simply add the first index of each wire to the copy cycle associated with the variable 0. Whether or not this zero row is utilized is specified in the Flavor, since, for example, it is not needed for plonk variants or even Standard Honk.

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • The branch has been merged with/rebased against the head of its merge target.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • There are no circuit changes, OR a cryptographer has been assigned for review.
  • New functions, classes, etc. have been documented according to the doxygen comment format. Classes and structs must have @brief describing the intended functionality.
  • If existing code has been modified, such documentation has been added or updated.
  • No superfluous include directives have been added.
  • I have linked to any issue(s) it resolves.
  • I'm happy for the PR to be merged at the reviewer's next convenience.

@ledwards2225 ledwards2225 changed the title Cleaning up circuit size params in honk composers chore: Cleaning up circuit size params in honk composers Jul 5, 2023
@kevaundray kevaundray changed the title chore: Cleaning up circuit size params in honk composers chore Cleaning up circuit size params in honk composers Jul 7, 2023
@kevaundray kevaundray changed the title chore Cleaning up circuit size params in honk composers chore: Cleaning up circuit size params in honk composers Jul 7, 2023
@kevaundray kevaundray changed the title chore: Cleaning up circuit size params in honk composers chore: Cleaning up circuit size parameters in honk composers Jul 10, 2023
@ledwards2225 ledwards2225 changed the title chore: Cleaning up circuit size parameters in honk composers chore: Support public inputs in Ultra Honk Jul 17, 2023
@@ -87,7 +89,7 @@ template <typename Flavor> class TranscriptTests : public testing::Test {
round++;
// TODO(Mara): Make testing more flavor agnostic so we can test this with all flavors
if constexpr (IsGrumpkinFlavor<Flavor>) {
manifest_expected.add_entry(round, "IPA:poly_degree", circuit_size);
manifest_expected.add_entry(round, "IPA:poly_degree", size_uint64);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This was a bug in the test that was revealed by the work in this PR but is otherwise unrelated.

@@ -9,6 +9,57 @@ struct SelectorProperties {
bool requires_lagrange_base_polynomial = false;
};

/**
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These two functions were formerly in the shared composer lib but they are used only for plonk so they've been moved to the plonk specific composer lib

@@ -1,179 +0,0 @@
#pragma once
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This file was an exact copy of composer_lib and was not used at all so I've deleted it entirely.

@ledwards2225 ledwards2225 changed the title chore: Support public inputs in Ultra Honk feat: Support public inputs in Ultra Honk Jul 17, 2023
@ledwards2225 ledwards2225 marked this pull request as ready for review July 17, 2023 16:25
@@ -251,33 +249,6 @@ std::shared_ptr<typename Flavor::ProvingKey> UltraComposer_<Flavor>::compute_pro

// Polynomial memory is zeroed out when constructed with size hint, so we don't have to initialize trailing space

// // In the case of using UltraPlonkComposer for a circuit which does _not_ make use of any lookup tables, all
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the Plonk strategy for ensuring non-zero polys. We wont ever use this for Honk.


construct_selector_polynomials<Flavor>(circuit_constructor, proving_key.get());

// TODO(#217)(luke): Naively enforcing non-zero selectors for Honk will result in some relations not being
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is the Plonk strategy for ensuring non-zero polys. We wont ever use this for Honk.

@ledwards2225 ledwards2225 linked an issue Jul 17, 2023 that may be closed by this pull request
@@ -36,6 +38,11 @@ template <UltraFlavor Flavor> class UltraComposer_ {
std::vector<uint32_t> recursive_proof_public_input_indices;
bool contains_recursive_proof = false;
bool computed_witness = false;
size_t total_num_gates = 0; // num_gates + num_pub_inputs + tables + zero_row_offset (used to compute dyadic size)
size_t dyadic_circuit_size = 0; // final dyadic circuit size
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend adding (next power of two) in the comments. As a variable name dyadic is ok, but for me it wasn't clear from the start if this is supposed to be the logarithm or the power of 2

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good call - done

@@ -66,6 +66,9 @@ class Standard {
using RelationUnivariates = decltype(create_relation_univariates_container<FF, Relations>());
using RelationValues = decltype(create_relation_values_container<FF, Relations>());

// Whether or not the first row of the execution trace is reserved for 0s to enable shifts
static constexpr bool has_zero_row = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need it for all honk flavors?

// define utilities to extend univarates from RELATION_LENGTH to MAX_RELATION_LENGTH for each Relation
// using BarycentricUtils = decltype(create_barycentric_utils<FF, Relations, MAX_RELATION_LENGTH>());
// Whether or not the first row of the execution trace is reserved for 0s to enable shifts
static constexpr bool has_zero_row = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need it for all honk flavors?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No need for the zero row for Standard flavors since in this case the only shifted poly is z_perm which is constructed explicitly with a constant coeff equal to 0. The problem arises for UltraHonk since we need to shift the wire polys themselves

@ledwards2225 ledwards2225 merged commit 9cd0a06 into master Jul 18, 2023
2 checks passed
@ledwards2225 ledwards2225 deleted the lde/composer_cleanup branch July 18, 2023 20:18
ludamad pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jul 22, 2023
ludamad pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jul 24, 2023
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.

Allow UltraHonk to handle public inputs
2 participants