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: Split plonk and honk tests #529

Merged
merged 20 commits into from
Jun 20, 2023
Merged

feat: Split plonk and honk tests #529

merged 20 commits into from
Jun 20, 2023

Conversation

codygunton
Copy link
Collaborator

@codygunton codygunton commented Jun 13, 2023

Description

With this PR, honk_tests and plonk_tests targets build without a composer wrapping a circuit constructor and composer helper. I verified this by temporarily deleting the composer files and rebuilding, but the composer files are still present place because other parts of Barretenberg still depend on them. Next step is to split the stdlib tests and then delete the old composers.

Along the way I deleted some unused (or declared but not defined) functions, did some renaming (some composers become circuit_constructor or builder; I leave a mix since a big renaming PR will have to be made later anyway), removed the honk dependency on plonk (very easy), and did other nice small bits of cleanup.

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • There are no circuit changes, OR specifications in /markdown/specs have been updated.
  • There are no circuit changes, OR a cryptographer has been assigned for review.
  • I've updated any terraform that needs updating (e.g. environment variables) for deployment.
  • The branch has been rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.
  • 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.

@codygunton codygunton self-assigned this Jun 14, 2023
/* Generates a monomial basis Grumpkin SRS for testing purposes.
We only provide functionality create a single transcript file.
The SRS has the form [1]_1, [x]_1, [x^2]_1, ... where x = 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.

This was out of date.

@@ -159,7 +159,8 @@ void UltraHonkComposerHelper_<Flavor>::compute_witness(CircuitConstructor& circu
template <UltraFlavor Flavor>
UltraProver_<Flavor> UltraHonkComposerHelper_<Flavor>::create_prover(CircuitConstructor& circuit_constructor)
{
finalize_circuit(circuit_constructor);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I didn't see a reason for composer helpers to have a function that proxies immediately to circuit_constructor.finalize_circuit so I made the change.

@@ -1,19 +1,18 @@
#include "standard_honk_composer.hpp"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The tests that were refactors have enormous diffs. I tried hard to keep the work to a totally formulaic refactor.

@@ -1,274 +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 is not in use anywhere.

@@ -1179,7 +1179,6 @@ class UltraCircuitConstructor : public CircuitConstructorBase<arithmetization::U
fr alpha_base,
fr alpha) const;

void update_circuit_in_the_head();
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 function is declared but not defined.

@@ -136,7 +136,7 @@ template <typename OuterComposer> class stdlib_verifier : public testing::Test {
verification_key_pt::from_witness(&outer_composer, verification_key_native);

info("Constructing the ultra (inner) proof ...");
plonk::proof recursive_proof = prover.construct_proof();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Name was vague. If anything, I'd call the outer proof, the one constructed by the recursive verifier, the recursive proof.

@@ -1,5 +1,4 @@
# TODO(Cody): Remove plonk dependency
Copy link
Collaborator Author

@codygunton codygunton Jun 14, 2023

Choose a reason for hiding this comment

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

I had already observed with Mara that this was easy to do, and doing so here let me work on this PR more incrementally.

@codygunton codygunton marked this pull request as ready for review June 14, 2023 19:49
@codygunton codygunton requested a review from Rumata888 June 14, 2023 19:49
@@ -70,6 +70,8 @@ template <typename Curve> struct aggregation_state {

auto* context = P0.get_context();

context->check_circuit();
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to leave this in? Currently it will only print information about failed gates

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whoops, no, that should go

@codygunton codygunton changed the title feat: Split plonk and honk tests. feat: Split plonk and honk tests Jun 15, 2023
@codygunton
Copy link
Collaborator Author

I don't understand why it's complaining about the conventional commit title.

@Rumata888
Copy link
Contributor

I don't understand why it's complaining about the conventional commit title.

I force re-ran it and everything seems ok

@ludamad
Copy link
Collaborator

ludamad commented Jun 15, 2023

I don't understand why it's complaining about the conventional commit title.

I swear the action/github actions is just a bit buggy - I see PR's close and then suddenly their bad status propagates to another PR

@Rumata888 Rumata888 merged commit ba583ff into master Jun 20, 2023
@Rumata888 Rumata888 deleted the cg/split-fooonk-tests branch June 20, 2023 16:10
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.

3 participants