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.

Sign up for GitHub

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

Jump to bottom

fix: check_circuit bug fix #510

Merged
merged 3 commits into from
Jun 20, 2023
Merged

fix: check_circuit bug fix #510

merged 3 commits into from
Jun 20, 2023

Conversation

Rumata888
Copy link
Contributor

@Rumata888 Rumata888 commented Jun 6, 2023 •
edited
Loading

Description

Check circuit had an off-by-one error

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.

ludamad
ludamad reviewed Jun 6, 2023
View reviewed changes
cpp/src/barretenberg/ecc/scalar_multiplication/scalar_multiplication.test.cpp Outdated
@@ -917,7 +917,7 @@ TYPED_TEST(SRSIO, PippengerUnsafeShortInputs)

Fr* scalars = (Fr*)aligned_alloc(32, sizeof(Fr) * num_points);

AffineElement* points = (AffineElement*)aligned_alloc(32, sizeof(AffineElement) * num_points * 2 + 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason this can't just be a points = new AffineElement[num_points * 2 + 1]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need them aligned. From what I understand by default new doesn't do that and we absolutely need that for pippenger

ledwards2225
ledwards2225 approved these changes Jun 12, 2023
View reviewed changes
Copy link
Collaborator

@ledwards2225 ledwards2225 left a comment

Choose a reason for hiding this comment

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

LGTM

cpp/src/barretenberg/plonk/proof_system/widgets/random_widgets/plookup_widget_impl.hpp Outdated
@@ -334,6 +334,9 @@ void ProverPlookupWidget<num_roots_cut_out_of_vanishing_polynomial>::compute_gra
// Compute and add monomial form of z_lookup to the polynomial store
z_lookup.ifft(key->small_domain);
key->polynomial_store.put("z_lookup", std::move(z_lookup));
for (size_t k = 1; k < 4; ++k) {
free(accumulators[k]);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Good catch

@kevaundray kevaundray changed the title Stupid check_circuit bug fix and memory leak in widget fix fix: Stupid check_circuit bug fix and memory leak in widget Jun 12, 2023
@Rumata888 Rumata888 changed the title fix: Stupid check_circuit bug fix and memory leak in widget fix: check_circuit bug fix and memory leak in widget Jun 20, 2023
@Rumata888 Rumata888 changed the title fix: check_circuit bug fix and memory leak in widget fix: check_circuit bug fix Jun 20, 2023
@Rumata888 Rumata888 marked this pull request as ready for review June 20, 2023 17:37
@Rumata888 Rumata888 merged commit 4b156a3 into master Jun 20, 2023
@Rumata888 Rumata888 deleted the is/memory_fixes branch June 20, 2023 17:37
@github-actions github-actions bot mentioned this pull request Jun 20, 2023
Merged
@codygunton codygunton linked an issue Jun 21, 2023 that may be closed by this pull request
check_circuit should handle recursion #465
Closed
@codygunton codygunton mentioned this pull request Jun 21, 2023
Closed
@github-actions github-actions bot mentioned this pull request Jul 11, 2023
Merged
ludamad pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jul 22, 2023
@Rumata888
fix: check_circuit bug fix (AztecProtocol/barretenberg#510)
432427f
ludamad pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jul 24, 2023
@Rumata888
fix: check_circuit bug fix (AztecProtocol/barretenberg#510)
119bc2c
This was referenced Jul 25, 2023
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ludamad ludamad ludamad left review comments

@ledwards2225 ledwards2225 ledwards2225 approved these changes

Assignees

@Rumata888 Rumata888

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

check_circuit should handle recursion
3 participants
@Rumata888 @ludamad @ledwards2225