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

feat: Make the circuit constructors field agnostic so we can check circuits on grumpkin #534

Merged
merged 18 commits into from
Jul 5, 2023
Merged

feat: Make the circuit constructors field agnostic so we can check circuits on grumpkin #534

merged 18 commits into from
Jul 5, 2023

Conversation

maramihali
Copy link
Contributor

@maramihali maramihali commented Jun 15, 2023 •
edited
Loading

This PR continues the work of getting Honk over Grumpkin by making the circuit constructors and gates field agnostic and ensuring the StandardCircuitConstructor is able to verify basic circuits over Grumpkin.

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.

@kevaundray kevaundray changed the title Mm/honk on grumpkin feat: Extends the implementation of Honk over the Grumpkin curve Jun 15, 2023
@maramihali maramihali changed the title feat: Extends the implementation of Honk over the Grumpkin curve feat: make the circuit constructors field agnostic so we can check circuits on grumpkin Jun 20, 2023
@maramihali maramihali marked this pull request as ready for review June 20, 2023 12:39
@maramihali
Copy link
Contributor Author

I reduced scope to only having standard circuits on Grumpkin because ultra circuits require field-agnostic plookup components and I want to prioritise getting Standard Honk on Grumpkin before moving to Ultra (which should be overall similar but would likely require more components in bberg to be templated)

codygunton
codygunton requested changes Jun 21, 2023
View reviewed changes
Copy link
Collaborator

@codygunton codygunton left a comment

Choose a reason for hiding this comment

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

Nice work, thanks. I'm only asking for cleanup and conversations.

cpp/src/barretenberg/ecc/curves/bn254/bn254.hpp Outdated
@@ -15,5 +16,6 @@ class BN254 {
using AffineElement = typename Group::affine_element;
using G2AffineElement = typename barretenberg::g2::affine_element;
using G2BaseField = typename barretenberg::fq2;
using Fq12 = barretenberg::fq12;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you give this a more generic name like PairingTargetField or something like this? That way, if we have end up implementing another pairing-friendly curve with possibly a different pairing extension degree (not 12), we will not have to change aliases. It's also more readable for someone looking at the library for the first time. It might also more future-proof to not refer to the name of the prime in the alias.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dyt TargetField is enough? You won't have a target field unless the curve supports pairings.

cpp/src/barretenberg/proof_system/arithmetization/gate_data.hpp Outdated
};

using poly_triple = poly_triple_<barretenberg::fr>;

// TODO: figure out what to do with this...
Copy link
Collaborator

Choose a reason for hiding this comment

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

Unresolved TODO. If you conclude this does not need to be handled now, please make a github issue and update the comment to read // TODO(#<issue number>): ....

Copy link
Contributor Author

@maramihali maramihali Jun 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I cleaned some boilerplatecode as you suggested, thanks for that - my idea is that this is sort of okay to get to MVP with Grumpkin on StandardHonk without much hassle in the rest of the codebase but then should be cleaned up (opened issue 557 for this file); the nicest thing to do imo is have no alias and just make the type for each gate explicit when we refer to them in the codebase. They don't appear in that many places after i did a bit of cleanup.

Copy link
Collaborator

Choose a reason for hiding this comment

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

#557

cpp/src/barretenberg/proof_system/arithmetization/gate_data.hpp Outdated Show resolved Hide resolved
cpp/src/barretenberg/proof_system/arithmetization/gate_data.hpp Show resolved Hide resolved
cpp/src/barretenberg/proof_system/circuit_constructors/turbo_circuit_constructor.test.cpp Outdated Show resolved Hide resolved
cpp/src/barretenberg/proof_system/circuit_constructors/ultra_circuit_constructor.test.cpp Outdated
@@ -15,7 +15,7 @@ TEST(ultra_circuit_constructor, create_gates_from_plookup_accumulators)
{
UltraCircuitConstructor circuit_constructor = UltraCircuitConstructor();

barretenberg::fr input_value = fr::random_element();
fr input_value = fr::random_element();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't this make the file a little less clear? We'll want to find and replace barretenberg::fr to bn254::fr at some point in the future, and so I'd rather leave this as it was.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense, this change was trigger by the fact that there's fr almost everywhere and then four places with barretenberg::fr and we are using the namespace. Should I change all places to barretenberg::fr ?

cpp/src/barretenberg/proof_system/circuit_constructors/ultra_circuit_constructor.cpp Outdated Show resolved Hide resolved
cpp/src/barretenberg/proof_system/circuit_constructors/ultra_circuit_constructor.cpp Outdated Show resolved Hide resolved
@maramihali maramihali force-pushed the mm/honk-on-grumpkin branch 3 times, most recently from ea67137 to 37c4f7d Compare June 23, 2023 12:11
@maramihali
Copy link
Contributor Author

Opens issue #557 for addressing TODOs with templating gates.

@maramihali maramihali force-pushed the mm/honk-on-grumpkin branch 2 times, most recently from d4a8ca6 to babec93 Compare June 23, 2023 17:39
@kevaundray kevaundray changed the title feat: make the circuit constructors field agnostic so we can check circuits on grumpkin feat: Make the circuit constructors field agnostic so we can check circuits on grumpkin Jun 27, 2023
@codygunton codygunton merged commit 656d794 into master Jul 5, 2023
@codygunton codygunton deleted the mm/honk-on-grumpkin branch July 5, 2023 09:11
This was referenced Jul 5, 2023
Merged
Merged
ludamad pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jul 22, 2023
@maramihali @codygunton
feat: Make the circuit constructors field agnostic so we can check ci…
2abb7c4 
…rcuits on grumpkin (AztecProtocol/barretenberg#534)

Co-authored-by: maramihali <mara@aztecprotocol.com>
Co-authored-by: codygunton <codygunton@gmail.com>
ludamad pushed a commit to AztecProtocol/aztec-packages that referenced this pull request Jul 24, 2023
@maramihali @codygunton
feat: Make the circuit constructors field agnostic so we can check ci…
54a8c54 
…rcuits on grumpkin (AztecProtocol/barretenberg#534)

Co-authored-by: maramihali <mara@aztecprotocol.com>
Co-authored-by: codygunton <codygunton@gmail.com>
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

@kevaundray kevaundray kevaundray left review comments

@codygunton codygunton codygunton approved these changes

Assignees

@maramihali maramihali

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@maramihali @codygunton @kevaundray