Skip to content

Conversation

@dtebbs
Copy link
Contributor

@dtebbs dtebbs commented Oct 11, 2021

Moves many gadgets, including BLS12-377 pairing gadgets, from clearmatics/zecale, along with some accompanying interface changes.

@dtebbs dtebbs force-pushed the add-pairing-gadgets branch 17 times, most recently from 22227da to 44a4e8c Compare October 13, 2021 10:13
@dtebbs dtebbs force-pushed the add-pairing-gadgets branch 2 times, most recently from 314fa1a to 7ed8c1e Compare October 15, 2021 08:02
@dtebbs dtebbs marked this pull request as ready for review October 15, 2021 08:33
@dtebbs dtebbs force-pushed the add-pairing-gadgets branch from d5afd76 to ff116d0 Compare October 15, 2021 14:08
@dtebbs dtebbs force-pushed the add-pairing-gadgets branch from ff116d0 to 52d6074 Compare October 19, 2021 18:39
@AntoineRondelet
Copy link
Contributor

AntoineRondelet commented Oct 21, 2021

Moves many gadgets, including BLS12-377 pairing gadgets, from clearmatics/zecale, along with some accompanying interface changes.

@dtebbs Were the interfaces change needed for the code migration? If not, could we split this PR in 2 parts:

  1. One that does the code migration
  2. One that introduce code changes
    ?

Reviewing large chunks of code that are moved and mutated from one project to another is not the easiest, so I'd rather stick to 1 PR = 1 thing here and limit the scopes of the contributions to make it easier to review (and make the process less error prone).

@AntoineRondelet
Copy link
Contributor

Also, one of the commit in this PR, i.e. bb8c106, seems to tackle: #11.

@dtebbs
Copy link
Contributor Author

dtebbs commented Oct 21, 2021

Moves many gadgets, including BLS12-377 pairing gadgets, from clearmatics/zecale, along with some accompanying interface changes.

@dtebbs Were the interfaces change needed for the code migration? If not, could we split this PR in 2 parts:

  1. One that does the code migration
  2. One that introduce code changes
    ?

In most cases the interface changes are tied in with moving the code
The problem is that most of the interface changes were required in the original code in zecale. So if we had one PR which simply moves the code and doesn't make innterface changes, then we would have places where 2 equivalent schemes have slightly different interfaces (so the lib would not be self-consistent and many tests could not be applied to some classes), and places where there are duplicates (the original libsnark version and tests, along side the libzecale versions).

Potentially we could create those PRs in the reverse order, i.e.

  1. Apply the interface changes to all existing libsnark code
  2. Move all libzecale into libsnark

If you think that would make a big difference then let me know and I'll look into it.

const size_t num_primary_inputs,
const std::string &annotation_prefix);
void generate_r1cs_witness(
const r1cs_gg_ppzksnark_verification_key<other_curve<ppT>> &vk);
Copy link
Contributor

Choose a reason for hiding this comment

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

where's generate_r1cs_constraints?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok this interface has also been modified AFAICT, the _all_bits attribute isn't use anymore and the gen_constraints function seems to be missing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, ok. this class seems to be the equivalent of libzecale::r1cs_gg_ppzksnark_verification_key_scalar_variable

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added the empty functions for now so this PR doesn't get side-tracked. However, going forward I'd be keen to have a better distinction between these "variable" types (which should only have a "generate_r1cs_witness" method), and genuine "gadgets". Added #36 with some notes (TLDR: these objects should not really be expected to have a generate_constraints method).

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for opening this ticket. I'm in favor or rethinking the distinction between gadgets and variables, but to keep the code consistent before we do so, and stick to the gadget interface, it's good to have this function added back. Let's revisit this in another PR.

@AntoineRondelet
Copy link
Contributor

In most cases the interface changes are tied in with moving the code
The problem is that most of the interface changes were required in the original code in zecale

Ok I see. That's making this PR a little bit annoying to review since it moves quite a lot of code and the review is not simply about verifying the imports and namespaces etc but also tracking down the small interface changes spread across. That's fine, let's keep it that way.

dtebbs added 25 commits November 2, 2021 10:18
@dtebbs dtebbs force-pushed the add-pairing-gadgets branch from dbc3717 to 73a3cef Compare November 2, 2021 10:18
@dtebbs
Copy link
Contributor Author

dtebbs commented Nov 2, 2021

LGTM, thanks @dtebbs Happy for you to rebase if you want to clean the history, and I'll merge the PR

Thanks @AntoineRondelet . Rebased onto origin/develop so should be ready to merge once the CI passes.

@AntoineRondelet AntoineRondelet merged commit 9baa46f into develop Nov 2, 2021
@AntoineRondelet AntoineRondelet deleted the add-pairing-gadgets branch November 10, 2021 13:52
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