Plonk Anchor Vanchor

[GhostOfGauss]

This adds the anchor and vanchor plonk circuits to the protocol, so it closes #86 and closes #87.  

Aside from the anchor and vanchor there are a few new things:

• I wrote a new testing helper function to replace gadget_tester (see here). The reason is that gadget_tester has issues with failure tests.  One thing I didn't like about gadget_tester was that it lacks functionality for failure tests where the prover/verifier disagree on what the public input values are.  The new prove_then_verify function improves on this by adding an optional verifier_public_inputs argument.  This is useful for testing things like should_fail_to_verify_wrong_chain_id, for example. 
• Added a check_membership_is_enabled function to use for dummy inputs to the vanchor circuit (see here). 

To-Do: the vanchor tests are still ugly and I would like to clean them up.  My main question regarding these tests is 

• Initializing the array of input_paths in the obvious way would be to define some default_path and initialize the array as [default_path; INS].  But PathGadgets don't implement Copy and deriving Copy doesn't seem to work.  So what I'm doing now is ugly, see here. 
• At the moment I'm letting INS = BRIDGE_SIZE so that the root set is the same size as the number of inputs.  I don't really know what I'd do to make the tests allow for those two parameters to be unequal.  (Maybe it's not worth doing?)

[drewstone]

I'd say its worth making a test where BRIDGE_SIZE and INS are different. As they are likely to be different in practice since we're using 2-2 and 16-2 join/split txes in Solidity.

We can have a bridge where the root set is of length 3 and the inputs are size 2. This shouldn't be tough to implement right?

[codecov-commenter]

Codecov Report

❗ No coverage uploaded for pull request base (master@b3a8f9f). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master     #169   +/-   ##
=========================================
  Coverage          ?   66.49%           
=========================================
  Files             ?       66           
  Lines             ?     4539           
  Branches          ?        0           
=========================================
  Hits              ?     3018           
  Misses            ?     1521           
  Partials          ?        0           

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b3a8f9f...2e0ca85. Read the comment docs.

[GhostOfGauss]

I've changed the Vanchor tests to have a single 16-2 success test and I assume that all remaining tests are 2-2 transactions.  This solves the issues that I was concerned with above.  Please take a look at the 2 success tests (here and here). If there are no objections to the way they are written then I will change the failure tests to reflect the changes I made to those success tests.

[GhostOfGauss]

A question on this TODO : we can get the field size in bits as F::size_in_bits().  What should the bound on transaction size be? If there are only 2 outputs then having each amount < (field size)/2 is sufficient, but if you wanted to make sure that no overflow could occur when someone combines 16 inputs then you should enforce outputs to be < (field size)/16.  Maybe it's a good idea to use F::size_in_bits() - 10 just to be conservative?  That's still allowing for a huge transaction size, since it's something like a 244 bit number.

[drewstone]

I believe 248 bits is what is used in other projects.

…

On Fri, Feb 25, 2022 at 12:46 PM Todd Norton ***@***.***> wrote: A question on this TODO <https://github.com/webb-tools/arkworks-gadgets/blob/7f1e465feb3f03afa1c90ce6579a7580635e6777/arkworks-plonk-circuits/src/vanchor/mod.rs#L282> : we can get the field size in bits as F::size_in_bits(). What should the bound on transaction size be? If there are only 2 outputs then having each amount < (field size)/2 is sufficient, but if you wanted to make sure that no overflow could occur when someone combines 16 inputs then you should enforce outputs to be < (field size)/16. Maybe it's a good idea to use F::size_in_bits() - 10 just to be conservative? That's still allowing for a huge transaction size, since it's something like a 244 bit number. — Reply to this email directly, view it on GitHub <#169 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADELLF6I3AFUZGHY5NIUBFLU466AFANCNFSM5PFG2OEA> . You are receiving this because your review was requested.Message ID: ***@***.***>

[lazovicff]

This order of public inputs is very different from R1CS circuit. We should aim to make them equal. @drewstone What approach should we take here. This is how R1CS looks like: https://github.com/webb-tools/protocol-substrate/blob/main/pallets/vanchor/src/lib.rs#L394-L405

[lazovicff]

The Anchor PI are also not matching the R1CS. For comparison: https://github.com/webb-tools/arkworks-gadgets/blob/drew/anchor-vanchor/arkworks-plonk-circuits/src/anchor/mod.rs#L629-L633
Old ones: https://github.com/webb-tools/protocol-substrate/blob/main/pallets/anchor/src/lib.rs#L407-L416

I propose a separate task should be made for this, since neither of these circuits is setting up the PI in a most intuitive way.

[lazovicff]

#180

[drewstone]

Sounds good, we can triage separately.