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

Dependency refactor #110

Merged
merged 11 commits into from
Aug 24, 2022
Merged

Dependency refactor #110

merged 11 commits into from
Aug 24, 2022

Conversation

mrain
Copy link
Contributor

@mrain mrain commented Aug 17, 2022

Description

closes: #109

  • Remove jf-rescue crate, rescue hash function now resides in jf-primitives/rescue.
  • Plonk constraint system definition and concrete constructions now live in a standalone crate jf-relation.
    • Basic and customized circuit gates are defined in jf-relation.
    • Customized/advanced circuit implementations are located in their own crates.
      • Plonk verifier related gadgets, transcript and plonk-verifier are now in jf-plonk/circuit.
      • Primitive gadgets, including commitment, el gamal etc. remains in jf-primitives/circuit.
      • Circuit for rescue hash function is now in jf-primitives/circuit/rescue.
  • par-utils is moved to jf-utils.
  • New dependency graph
    • jf-relation depends on jf-utils.
    • jf-primitives depends on jf-relation and jf-utils.
    • jf-plonk depends on all other crates.

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

  • Targeted PR against correct branch (main)
  • Linked to GitHub issue with discussion and accepted design OR have an explanation in the PR that describes this work.
  • Wrote unit tests
  • Updated relevant documentation in the code
  • Added a relevant changelog entry to the Pending section in CHANGELOG.md
  • Re-reviewed Files changed in the GitHub PR explorer

@mrain mrain linked an issue Aug 17, 2022 that may be closed by this pull request
@mrain mrain changed the title Refactor Dependency refactor Aug 17, 2022
Copy link
Contributor

@alxiong alxiong 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.

here's some comments:

  • plonk/src/circuit/* should be moved to jf-relation, those are not related to Plonk proof system, they are gadgets for plonk verifier (when you try to do proof recursion and prove that a Plonk proof is correct in circuit, therefore they belong with all the other gadgets)
  • within jf-relation:
    • no need to have customized/ folder,
    • instead, put all gate impl (including basic.rs into relation/src/gadgets/*
    • merged relation/src/customized/gates.rs into relation/src/gates.rs
    • rename relation/src/customized/mod.rs to customized.rs ? (@chancharles92, @zhenfeizhang any better idea?) or we can also split them up and group them like arkwork or like plonky2
    • gadgets moved from plonk/ should be under /gadgets/transcript/ and gadgets/plonk_verifier/, just like gadgets/ecc/

@mrain
Copy link
Contributor Author

mrain commented Aug 18, 2022

👍 nice work.

here's some comments:

  • plonk/src/circuit/* should be moved to jf-relation, those are not related to Plonk proof system, they are gadgets for plonk verifier (when you try to do proof recursion and prove that a Plonk proof is correct in circuit, therefore they belong with all the other gadgets)

Currently, circuit implementation of plonk verifier depends on struct VerifyingKey provided by jf-plonk. Moving it to jf-relation will cause circular dependency.

  • within jf-relation:

    • no need to have customized/ folder,
    • instead, put all gate impl (including basic.rs into relation/src/gadgets/*
    • merged relation/src/customized/gates.rs into relation/src/gates.rs
    • rename relation/src/customized/mod.rs to customized.rs ? (@chancharles92, @zhenfeizhang any better idea?) or we can also split them up and group them like arkwork or like plonky2
    • gadgets moved from plonk/ should be under /gadgets/transcript/ and gadgets/plonk_verifier/, just like gadgets/ecc/

Good suggestion. Will do.

plonk/Cargo.toml Outdated Show resolved Hide resolved
plonk/src/constants.rs Outdated Show resolved Hide resolved
primitives/Cargo.toml Outdated Show resolved Hide resolved
relation/Cargo.toml Outdated Show resolved Hide resolved
relation/src/constants.rs Outdated Show resolved Hide resolved
relation/src/customized/ecc/mod.rs Outdated Show resolved Hide resolved
@zhenfeizhang
Copy link
Contributor

Good idea... now the customized is a bit large and it makes sense to group its components

@chancharles92
Copy link
Contributor

chancharles92 commented Aug 18, 2022

Agree with @zhenfeizhang , I prefer the approach used by arkworks and plonky2

@alxiong
Copy link
Contributor

alxiong commented Aug 18, 2022

Currently, circuit implementation of plonk verifier depends on struct VerifyingKey provided by jf-plonk. Moving it to jf-relation will cause circular dependency.

Ah, I see, then just keep them where there are.

@mrain mrain requested review from zhenfeizhang and alxiong August 22, 2022 19:06
Copy link
Contributor

@alxiong alxiong left a comment

Choose a reason for hiding this comment

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

I really like your reorg! great work! 🎉
Thank you for the big migration!

Some minor comments:

  • relation/gadgets/basic.rs seems a bit strange, I would merge this file with relation/circuit.rs and call it relation/constraint_system.rs (which contains both the trait Circuit and struct PlonkCircuit) <-- I agree our usage of circuit/constraint_system/gadget/gates need to be re-checked later.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
relation/src/gadgets/ecc/mod.rs Outdated Show resolved Hide resolved
relation/src/gadgets/logic.rs Show resolved Hide resolved
alxiong
alxiong previously approved these changes Aug 23, 2022
Copy link
Contributor

@alxiong alxiong left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@chancharles92 chancharles92 left a comment

Choose a reason for hiding this comment

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

Good job! Only 1 minor comment

plonk/src/circuit/plonk_verifier/mod.rs Outdated Show resolved Hide resolved
relation/Cargo.toml Outdated Show resolved Hide resolved
…circuit`. Removed unnecessary `test_apis` feature
@mrain mrain merged commit f74a685 into main Aug 24, 2022
@mrain mrain deleted the dependency-refactor branch August 24, 2022 03:48
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.

[Dependency] Remove dependency of jf-plonk from jf-primitives
4 participants