-
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
Compact groth16 arguments #288
Conversation
6635dee
to
e10f7bc
Compare
d20b7e4
to
f7f1bff
Compare
e5c9c2b
to
7594488
Compare
f7f1bff
to
e812c74
Compare
7594488
to
618b918
Compare
e812c74
to
a3184f9
Compare
abd9b93
to
e00294f
Compare
65e912f
to
13f5d4f
Compare
e00294f
to
ecabe3e
Compare
ecabe3e
to
b9d5569
Compare
// uint256[2] Alpha, | ||
// uint256[4] Beta, | ||
// uint256[4] Delta, | ||
// uint256[] ABC_coords |
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.
Side note: Now that we support several curves (i.e. bn/bls) may be worth to clarify these comments. This contract is tailored for bn254, and won't work with any other groups. So May be worth specifying under with circumstances your comment above holds (i.e. ALT_BN128
set in cpp code config) or simply rephrase to make this generic, e.g.:
// Constructor. The form of vk is:
// alpha = vk[:2*N]
// beta = vk[2*N:6*N]
// delta = vk[6*N:10*N]
// ABC_coords = vk[10*N:]
// where:
// - q is the characteristic of the base field of the pairing group used
// - N = 2*\ceil{bitLen(q)/256} (# EVM words necessary to represent elements of Fq)
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.
Side note2: Making the constructor below generic shouldn't be really hard, but troubles may start with functions below, so that may be better to have different contracts to avoid any complexity related to generalization (and so my comment above may not apply anymore if the group name appears in the contract name)
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've added a comment at the top to explain that this all assumes alt-bn128.
Making the constructor below generic shouldn't be really hard, but troubles may start with functions below, so that may be better to have different contracts to avoid any complexity related to generalization
Yes, good idea to rename, especially once we support another curve. It will be interesting to see just how much of the contract can be made generic. But to make it properly generic without some kind of templating I think we'd probably need to pass several extra parameters (to specify the size of field and curve elements, the value of `g2, etc) and extra logic at runtime to deal with proofs and verification keys in dynamic arrays.
It would be nicer to split the verification part into it's own library
. I seem to remember hit issues with this in the past, but it's working in zecale so we may be able to replicate what happens there.
uint256[2] memory c_p, | ||
uint256[2] memory h, | ||
uint256[2] memory k, | ||
uint256[18] memory proof_data, |
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.
Name is inconsistent with argument name in verifyTx
for Groth16
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.
Renamed the groth16 version from proof
to proof_data
. (In the pghr13 case this function fills out an actual Proof structure, which is naturally named proof
. The groth16 version is optimized to pull directy from the memory buffer.)
b9d5569
to
6c9b078
Compare
LGTM |
Compact the proof and vk into single arrays of evm words. Saves gas and reduces the number of parameters required in applictions where vk and proof must be passed as arguments.
Depends on #287