-
Notifications
You must be signed in to change notification settings - Fork 358
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
MPC Generated Pedersen Keys #1180
base: master
Are you sure you want to change the base?
Conversation
Hi @ewoolsey apologies for the delay it's been very busy recently. Will review asap. |
All good, thanks so much for taking a look! I don't see any reason why this shouldn't be the default behaviour, but if you see any issues let me know! |
// Update BasisExpSigma with δ | ||
// TODO: Is it sound to use the same δ for all basis? | ||
for i := 0; i < len(c.Parameters.G1.BasisExpSigma); i++ { | ||
for j := 0; j < len(c.Parameters.G1.BasisExpSigma[i]); j++ { | ||
c.Parameters.G1.BasisExpSigma[i][j].ScalarMultiplication(&c.Parameters.G1.BasisExpSigma[i][j], &deltaBI) | ||
} | ||
} | ||
|
||
// Update GRootSigmaNeg using δ⁻¹ | ||
c.Parameters.G2.GRootSigmaNeg.ScalarMultiplication(&c.Parameters.G2.GRootSigmaNeg, &deltaInvBI) | ||
|
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.
aren't we supposed to verify the ratio of this in verifyPhase2
function?
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.
@hussein-aitlahcen yep that's correct! Sorry for the slow reply, I've been away for the week! I'll push an update for this in an hour or so!
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 actually patched my fork with your change, rebased on top of main and added the verification if you want to absorb it hussein-aitlahcen@9d859af. I also fixed the cloning of the phase2 which was missing the pedersen key/marshalling and was able to do a full mpc round using this.
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.
@hussein-aitlahcen Thanks for doing that! I've just committed your changes to this PR. Although it might be nice to eventually upstream the marshaling stuff.
I'm getting some issues with verification though it seems.
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.
What error are you getting? I'm able to do phase2 init + contrib + verification. Did it a couple of time. I think I also inverted GRootSigmaNeg to be mul by deltaBI
instead of deltaInvBI
. Also if you didn't fix the cloning it won't work.
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.
Ah I manually recreated your fix but copy and pasted wrong. Just cherry picked and now things seem to be working!
@hussein-aitlahcen thanks so much for the initial review. If you think this is good to go ahead now then I'll go ahead and translate these changes into the templates. |
Maybe @Tabaie could double check what we did please? I'm not a cryptographer :). |
@Tabaie just bumping this one more time 👍 |
@Tabaie sorry just bumping this again ahah |
Description
Some circuits require that the pedersen key is MPC generated.
I'm opening this up as a draft PR as I'd like to gather some feedback on it! Additionally, it will require that encoding for
[][]curve.G1Affine
is supported ingnark-crypto
. Once I've got the go ahead from one of the maintainers I'll proceed with the encoding work upstream and properly updating the templates.This change involves adding a couple fields to both
Phase2Evaluations
andPhase2
which represent the G1 points of the Pedersen keys.Phase2.Contribute()
then handles the MPC contributions like you'd expect.Breaking changes include the addition of the new struct fields, as well as the change of arguments in
ExtractKeys
Fixes
#1046
#1136
Type of change
How has this been tested?
Checklist:
golangci-lint
does not output errors locally