-
Notifications
You must be signed in to change notification settings - Fork 10
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
Implementing HyperKZG verifier #60
Conversation
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.
Can't say much about the solidity portion, but the match checks out
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.
Thanks a lot for the thorough testing. I've left comments, but nothing blocking.
Do we want to open an issue for writing a slightly more efficient MSM, even if it's in Solidity?
} | ||
|
||
if (!pi_polys_are_correct(input.pi_evals_0, input.pi_evals_1, input.pi_evals_2, input.point, r, input.p_of_x)) { | ||
return false; |
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.
I wonder if using custom error types may not make this more maintainable, generally?
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.
For sure, it will make. Let's re-consider this in context of wrapping this building block into e2e contract, where whole set of errors will be defined. In assembly contract, every error should have own random 4 bytes identifier that allows tracing when contract is invoked via network.
assembly { | ||
let success := call(sub(gas(), 2000), 8, 0, add(input, 0x20), mul(inputSize, 0x20), out, 0x20) | ||
switch success | ||
case 0 { revert(0, 0) } |
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.
The original mentions returning invalid here to make gas estimation work. Does it perform well in the case of this PR?
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.
TBH, I don't completely understand this pairing precompile invocation code. With invalid()
it still produces OutOfGas
error in my current settings. Will need to spend some time to figure it out. Anyway, it is indeed true that we need to have some correct gas cost value on test failure
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.
Created and issue (#67) to address this later and unblock merging this PR
} | ||
} | ||
|
||
return absorb(keccak, label, input); |
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.
And about here I wish we had inline functions, but alas
@@ -1252,7 +1292,7 @@ library KeccakTranscriptLib { | |||
} | |||
|
|||
// write byte indicating whether point is at infinity | |||
if (Bn256.is_identity(point)) { | |||
if (!Bn256.is_identity(point)) { |
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.
😮
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.
Yeah - I was wondering too. The reason is that initial version of this absorb
wasn't used actually while computation. Now with HyperKZG it is 100% used and debugging transcription revealed this mistake.
More efficient - means either in Yul or in Huff, right? This actually lies in context of gas optimising meta-issue (and correspondent branch). We have some drafts already for Bn256 (link) and for Grumpkin (link) in particular. And I, personally, would postpone these further developments (in terms of any kinds of gas optimisation) until having more or less stable reference verifier (I think that naturally it can be SuperNova + light-client proof example) that we are committing to externally demonstrate in production. The obvious reason for this is that maintaining Yul / Huff code is much more expensive than Solidity one |
Fixes #59
This PR implements HyperKZG verifier.
Reference implementation: https://github.com/lurk-lab/arecibo/tree/ba341487d3f4a200bb624bb86aade4d9e63da22e