-
Notifications
You must be signed in to change notification settings - Fork 26
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
Negate groth16 b (depends on #283) #287
Changes from 4 commits
5c98f7b
dafa607
291254f
13f5d4f
85e0578
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -132,8 +132,10 @@ template<typename ppT> | |
std::ostream &groth16_snark<ppT>::proof_write_json( | ||
const typename groth16_snark<ppT>::proof &proof, std::ostream &out_s) | ||
{ | ||
// JSON matches the protobuf format, where we export -b instead of b, to | ||
// support efficient verification in contracts. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. I'd be clear on the fact that these changes are mostly for simplicity matters (as per the main thread discussion) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a note on the doc comments for this function. |
||
out_s << "{\n \"a\": " << point_affine_to_json(proof.g_A) | ||
<< ",\n \"b\": " << point_affine_to_json(proof.g_B) | ||
<< ",\n \"minus_b\": " << point_affine_to_json(-proof.g_B) | ||
<< ",\n \"c\": " << point_affine_to_json(proof.g_C) << "\n}"; | ||
return out_s; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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."There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thatminus_b
is negated back tob
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here https://github.com/clearmatics/zeth/blob/negate-groth16-b/libzeth/snarks/groth16/groth16_api_handler.hpp#L29-L35
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ofb
. I think that modifyingproof_to_contract_parameters
to: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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 getq
, or to actually perform the negation) but it's definitely possible.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(This would involve negating
vk.delta
and one ofvk.alpha
orvk.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).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
Cons:
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 theinit request
response the client can instantiateCurveParams
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).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I just read your comment:
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.