Negate groth16 b (depends on #283)

[dtebbs]

Change the groth16 proof protobuf data object to hold minus_b instead of b, so that this does not have to be done in the contract when evaluating the pairing.'
This makes it much easier to support groth16 verification over more pairings.

Depends on #283

[dtebbs]

Note a could also be chosen instead. Here I used b since the only G2 element, and because an alternative approach (negating multiple elements of the verification key) would also require negating G2 elements.

[AntoineRondelet]

I have mixed feelings about this one. Obtaining -P from P (P being a curve point), we just need to negate the y coordinate so that's a pretty cheap operation. When carrying out arithmetic over BW6-761, that's a bit annoying because the bit-length difference between curve points' coordinates and EVM-words is big, which means that even the simplest arithmetic operations on a field element require to pop and push 3 EVM words (at least 6 EVM operations for a simple negation...). Now, changing the protocol - especially changing the proof structure - for this reason seems quite a radical approach to me. The reasons I am doubtful are:

• This change is likely going to be translated over to Zecale (both the the I/O on nested proofs (extended_proof_to_proto was modified) and for the Zecale wrapping proof verification on chain). This means that, negating b in received nested proofs will become a requirement for people willing to build base application to be used with Zecale.
• Such divergence from Groth16 adds friction. What I mean by that is that it just breaks the conventions around the initial protocol and forces us to document this change very well (the prover server API needs to reflect this unambiguously) and/or remember that in some of our projects we negate a proof member for ease of manipulation on smart-contracts. That's adding friction - especially because the proof structure is not the same in most of the cpp code and in the solidity code.

Before proceeding, do you have an estimate on the benefits of this "optimization"? I'd be keen to understand how much speedup/gas is saved to "trade this off" with the points above. In other words, what was the cost of:

let q := 21888242871839275222246405745257275088696311157297823662689037894645226208583
let proof_A_y := mload(add(proof, 0x20))
mstore(add(pad, 0x1a0), sub(q, mod(proof_A_y, q)))

(and what would the cost of similar operations be with BW6 params?)

[dtebbs]

I have mixed feelings about this one. Obtaining -P from P (P being a curve point), we just need to negate the y coordinate so that's a pretty cheap operation. When carrying out arithmetic over BW6-761, that's a bit annoying because the bit-length difference between curve points' coordinates and EVM-words is big, which means that even the simplest arithmetic operations on a field element require to pop and push 3 EVM words (at least 6 EVM operations for a simple negation...). Now, changing the protocol - especially changing the proof structure - for this reason seems quite a radical approach to me.

I'd need to check around to see if there is a trick for efficient multi-word mod, but it seems to me it could be very complex. Note that saving gas is not really the primary motivation in my mind. By removing this operation from the contract we avoid a lot of complex operations which have to be completely re-implemented for each curve. All other operations are done in precompiled contracts, so there is also a certain tidiness to avoiding doing this in solidity.

The reasons I am doubtful are:

* This change is likely going to be translated over to Zecale (both the the I/O on nested proofs (`extended_proof_to_proto` was modified) and for the Zecale wrapping proof verification on chain). This means that, negating `b` in received nested proofs will become a requirement for people willing to build base application to be used with Zecale.
* Such divergence from Groth16 adds friction. What I mean by that is that it just breaks the conventions around the initial protocol and forces us to document this change **very well** (the prover server API needs to reflect this unambiguously) and/or remember that in some of our projects we negate a proof member for ease of manipulation on smart-contracts. That's adding friction - especially because the proof structure is not the same in most of the cpp code and in the solidity code.

I think I understand the concern. My thinking here is that it should be possible for this to be done without any impact on the applications. Note that the difference is only in how the data is passed around by zecale (and the corresponding zecale contract simplification) - there should be no difference in the nested or wrapping circuit. Applications are free to generate and verify their own proofs in whatever form they wish. In any case we will need to provide library function / tools to generate the zecale NestedTransaction objects, so this change could be viewed as just the internals of that.

In that sense, I was thinking of it as pretty much equivalent to changing whether we pass bytes or JSON strings in the protobuf obects, or whether we send arrays of uint256 or bytes. Obviously, it's not exactly equivalent, but the impact on applications and the protocol should be pretty much the same in the sense that, if someone is relying on whether we pass b or minus_b, then they are also relying on whether we encode as evm words, bytes, strings, etc. At this level, applications should not really be concerned.

Before proceeding, do you have an estimate on the benefits of this "optimization"? I'd be keen to understand how much speedup/gas is saved to "trade this off" with the points above. In other words, what was the cost of:

let q := 21888242871839275222246405745257275088696311157297823662689037894645226208583
let proof_A_y := mload(add(proof, 0x20))
mstore(add(pad, 0x1a0), sub(q, mod(proof_A_y, q)))

(and what would the cost of similar operations be with BW6 params?)

In the trivial case, I'm sure the cost is negligible. Since optimization wasn't really the primary concern I was less concerned about gas savings, but I can imagine that this could get very complex for the multi-word case.
However, the main justification for this in my mind is more in terms of the simplicity of the contract code and the need to create an implementation for every curve.

Given the points above about considering this as being similar to an internal encoding, let me know if this makes sense in terms of your points. (Obviously it would still make sense to mention this in the docs as an implementation detail - I'm not arguing for leaving it out completely.)

[AntoineRondelet]

However, the main justification for this in my mind is more in terms of the simplicity of the contract code and the need to create an implementation for every curve.

I see, I got you intention slightly wrong. Your focus really is on making things simpler - which weights a lot in the balance.

Note that the difference is only in how the data is passed around by zecale (and the corresponding zecale contract simplification) - there should be no difference in the nested or wrapping circuit. Applications are free to generate and verify their own proofs in whatever form they wish

They are indeed, but as per my initial comment, I saw that the extended_proof_to_proto function of the groth16 handler was modified (see: https://github.com/clearmatics/zeth/blob/negate-groth16-b/libzeth/snarks/groth16/groth16_api_handler.tcc#L70-L105), which means that if we re-use libzeth to implement the API of the zecale server -> this change automatically translates to Zecale wrapping proof I/O (and if we want to re-use libzeth for other projects -> same applies).

I'd need to check around to see if there is a trick for efficient multi-word mod, but it seems to me it could be very complex. Note that saving gas is not really the primary motivation in my mind. By removing this operation from the contract we avoid a lot of complex operations which have to be completely re-implemented for each curve. All other operations are done in precompiled contracts, so there is also a certain tidiness to avoiding doing this in solidity.

I see, why not do this as a new precompile? (as we discussed off-line in the past). I mean, if the point is to avoid any trouble with the Zecale proofs verif (and avoid to manipulate multiple EVM words for this step), this is already making the assumption that the BLS12/BW6 curves arithmetic is available in the client (which makes the assumption of extra pre-compiled). Now, I see that this method has a drawback, which is that it contrasts with the "family of ECC" available set of precompiled (which do not propose a way to negate a point) but we can surely add this and document the client API changes somewhere.
Did you bump into any issue while considering the use of precompiled here?

[dtebbs]

However, the main justification for this in my mind is more in terms of the simplicity of the contract code and the need to create an implementation for every curve.

I see, I got you intention slightly wrong. Your focus really is on making things simpler - which weights a lot in the balance.

I should have made that clear in the description. Sorry about that.

Note that the difference is only in how the data is passed around by zecale (and the corresponding zecale contract simplification) - there should be no difference in the nested or wrapping circuit. Applications are free to generate and verify their own proofs in whatever form they wish

They are indeed, but as per my initial comment, I saw that the extended_proof_to_proto function of the groth16 handler was modified (see: https://github.com/clearmatics/zeth/blob/negate-groth16-b/libzeth/snarks/groth16/groth16_api_handler.tcc#L70-L105), which means that if we re-use libzeth to implement the API of the zecale server -> this change automatically translates to Zecale wrapping proof I/O (and if we want to re-use libzeth for other projects -> same applies).

Yes. The motivation behind this was for zecale and eventually for using other curves with zeth.

What do you think about providing the contract code for verification (for BW6-761 and BLS12-377) as part of Zeth? In this case, applications should be essentially unaffected by this change. Unless I've missed something, everything else should be abstracted. For example, f you look at this commit in zecale clearmatics/zecale@ea4b989 which deals with this change, it only involves the proof verifier in the contract and related test code. Everything else should be hidden behind zeth calls and types (which we can tighten up to make more opaque).

Would this be a workable solution?

[AntoineRondelet]

Shall this one be rephrased to reflect that the proposed changes are mostly for simplicity matters?
Maybe something like: "// Note that the protobuf format exports -b (minus_b) to simplify (and slightly optimize) the on-chain verification code."

[AntoineRondelet]

Additionally, could you make sure to document this on top of the function declaration in the hpp file using Doxygen's syntax to make sure this shows up in the generated docs? (same for _from_proto to explain that minus_b is negated back to b)

[AntoineRondelet]

Here https://github.com/clearmatics/zeth/blob/negate-groth16-b/libzeth/snarks/groth16/groth16_api_handler.hpp#L29-L35

[AntoineRondelet]

BTW, why not doing the negation on the python client in the proof_to_contract_parameters? Wouldn't that save the need to modify the cpp API?
For now, the cpp API is modified because of something happening on-chain. That'll be quite odd for someone using libzeth outside of a blockchain context to exchange -b instead of b. I think that modifying proof_to_contract_parameters to:

1. Negate b in the proof structure
2. Convert to EVM words

would keep changes to the right scope. This needs some "curve specific" operations on the client - which are not supported though (for now the client pretty much "relays" information between components). Did you think about that @dtebbs ?

[dtebbs]

BTW, why not doing the negation on the python client in the proof_to_contract_parameters?

Yes, I did consider that. I can see the point - we limit the scope of this change to the encoding made by the client for the verification code on the contract. The main reason I didn't do that here is because the client would then need to know which curve is being used, and the value of q for that curve. Currently it does not have that information and just uses a single scheme that converts field elements and curve points to evm words.

We could do that. It would require some CURVE config in the client, and the curve constants (or maybe the client could query the server to get q, or to actually perform the negation) but it's definitely possible.

[dtebbs]

(This would involve negating vk.delta and one of vk.alpha or vk.beta. The contract would then contain a hard-coded $[-1]_2$ instead of $[1]_2$ in the verifier, or it could be passed a constructor parameter).

[dtebbs]

One other thought: if we add an RPC method it could be GetProcessedVerificaitonKey which returns the VK with extra data: -beta, -delta and -1 in G2.

[AntoineRondelet]

The main reason I didn't do that here is because the client would then need to know which curve is being used [...]

Exact. I think this would be quite a nice thing to have actually (some curve config on the client), here are the pros/cons I can think of (please add any that comes to mind)
Pros:

• By having access to some "remarkable params" (field characteristics etc), we can not only do the negation above, but also support some tests on the received arguments (type/"range" check received elements if needed - this btw needs to be systematically done on the contract to avoid any potential attacks leveraging the mismatch in the length of EVM words and field elements -> that's bad because this advocates to define partial functions (and an API with partial functions is.. well.. annoying to say the least), but that's a requirement for some minimal type safety on the contract)
• We keep consistent APIs and limit changes to the smallest scope possible

Cons:

• Needs to track the curve config on the client (which exacerbates the "configuration logistic" where several params need to be duplicated to cpp code, python code and solidity code -> this may be fixed later on by building some tooling/scripts (like a lightweight configure script to generate each components' config for us to lessen this issue - but out of scope now))

To this point btw, we can surely avoid such troubles by extending the server's API to have a "fetchServerConfig" endpoint that fetches the server configuration/protocol params from the server and configures the client as per the fetched data. That way the curve/pairing group name (i.e. ALT_BN128, BLS12_377 etc) can be part of the received message, and upon reception of the init request response the client can instantiate CurveParams by selecting the right local curve configuration and proceed (i.e. as soon as the connection is established with the server/as soon as the client is started -> it fetches the server config).
This seems to be the right tradeoff. It keeps the client generic (works with any pairing group), while keeping things fairly simple. I can't think about any meaningful use-case where the client keeps processing proofs coming from several servers configured using different curves (so adding the "curve" to the extended_proof object - which would solve against that - seems completely overkilled for instance).

[AntoineRondelet]

Oh, I just read your comment:

We rely on the prover server being available at deployment, so we could add an RPC method to get the curve parameters and the client could just use those for a one-shot transform. Proofs can then remain unchanged with b.

For some reason it didn't show up when I wrote my comment (maybe I forgot to refresh the page before commenting, not sure). Anyway, seems to align with the point I made at the end of my previous comment. That looks like a solution.

[dtebbs]

Explained minus_b in the doc comments for this class. Ideally it would be on the protobuf type declaration, but that may not be captured by doc generation so added on the class.

[AntoineRondelet]

Same as above. I'd be clear on the fact that these changes are mostly for simplicity matters (as per the main thread discussion)

[dtebbs]

Added a note on the doc comments for this function.

[AntoineRondelet]

As discussed off-line @dtebbs this PR LGTM overall (modulo the few comments above). I saw that you negate -b back to b in the extended_proof_from_proto to avoid any change propagation to the core cpp code (especially for zecale).
Some extra warnings would need to be written in the docs to document that -b needs to be used in the proto messages (as this breaks the proof structure in Groth's paper)

[dtebbs]

(After offline discussion).
Even we the abstraction proposed above, this does create a bit more of a "rough edge" for anyone creating proofs externally (e.g. creating nested proofs for zecale, say). Alternatives discussed include:

• negation in solidity
• precompiled contracts to support negations
• precompiled contracts to support groth16 verification directly
  and above we mention providing tools/libraries to convert from a more familiar a, b, c into our proof types.

We could also consider modifying this PR (or creating issues) in order to:

• negate some elements in the verification key instead of proofs (since this should be handled less frequently)
• keep proof / key types without negation and add new types such as ProcessedProof which include the negated b, specifically for use by contracts. In zecale, clients could submit regular proofs to the aggregator, but the aggregator would return ProcessedProof objects (which are opaque to the application and are just passed to the verification contract library).

[AntoineRondelet]

Thanks for the extra documentation comments @dtebbs
Discussion moved to #292
LGTM, merging this PR