-
Notifications
You must be signed in to change notification settings - Fork 166
ZIP 312: change randomizer handling #895
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
base: main
Are you sure you want to change the base?
Conversation
zips/zip-0312.rst
Outdated
| value commitments, decryption of note ciphertexts, etc.; and the signers MUST check | ||
| that the given SIGHASH matches the data sent from the Coordinator, or compute the | ||
| SIGHASH themselves from that data. However, the specific mechanism for that process | ||
| is outside the scope of this ZIP. |
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 think the mechanism for using FROST for transaction spend auth should not be outside the scope of this ZIP, given that this ZIP is "FROST for Spend Authorization Multisignatures" (rather than the smaller-scoped "Re-randomized FROST"). However, I agree that most of the content that mechanism would entail is what would go into creating and checking PCZTs, so what we should do here is incorporate by reference the ZIP that eventually contains that specification (#693), and then either use it as appropriate directly in this section, or divide the specification into "Re-randomized FROST" (for more general consumption) and "Using Re-randomized FROST for Zcash transactions" (which then augments it further with the Zcash-specific logic like PCZT usage).
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 agree, let's leave this pending until we get more concrete on PCZTs
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.
OK that's annoying, I'm certain that I made a suggestion here, and GitHub's newly enshittified changed-files view has eaten it.
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 it's even worse. That comment is only visible on the "new experience" view.
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.
ACK with suggestions.
conradoplg
left a comment
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.
Addressed comments in did some additional improvements/cleanups
42b086e to
0d08f3f
Compare
|
force-pushed to resolve conflicts with |
aec05a4 to
ba47eb0
Compare
| supported by the Zcash Foundation's reference implementation of FROST | ||
| [#zf-frost-dkg]_. |
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.
Reference https://c2sp.org/cocktail-dkg here (currently C2SP/C2SP#164)
| Since Sapling will not be modified to support Quantum Recoverability | ||
| [#draft-ecc-quantum-recoverability]_, Orchard is RECOMMENDED for new FROST keys. |
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.
Note that #draft-ecc-quantum-recoverability provides the rationale for this choice.
ba47eb0 to
5100d80
Compare
| can be sent along with the FROST shares during key generation. This option MAY | ||
| be used if Distributed Key Generation was used, if participants find it | ||
| acceptable to trust one participant to correctly generate $\mathsf{sk}$. | ||
| - Participants generate a $\mathsf{sk}$ in distributed manner using TODO. |
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 should also reference https://c2sp.org/cocktail-dkg .
Co-authored-by: Daira-Emma Hopwood <daira@jacaranda.org>
Co-authored-by: Jack Grigg <jack@electriccoin.co>
Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>
Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>
ZIP 0: Document that the Pull-Request header uses the singular spelling even if there are multiple PRs (there are several existing cases of this). Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>
Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>
update references. Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>
as a prerequisite for unlinkability of Sapling and Orchard spend authorization signatures. Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>
(In my opinion it completely defeats the point of a threshold signature scheme.) Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>
(This might be moved into another PR.) Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>
Signed-off-by: Daira-Emma Hopwood <daira@jacaranda.org>
5100d80 to
9c24f21
Compare
|
Recent changes all look good to me. What are you thinking for next steps? I think we could merge this and work on the rest in other PRs? What I think is missing:
|
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.
Flushing my review comment from a while back on 9c24f21.
| The "Owners", "Credits", and "Original-Authors" headers always use | ||
| these plural spellings even if there is only one Owner, one person to | ||
| be credited, or one original author. The "Pull-Request" header always | ||
| uses this singular spelling even there are multiple Pull Requests. |
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 confirmed that these changes to ZIP 0 are either clarifications, or straightforward improvements to the ZIP metadata by a ZIP Editor, and I do not think these require full consensus from the ZIP Editors. But also, ACK on these changes with my ZIP Editor hat on.
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 hadn't finished reviewing the changes in this file, and didn't have any comments indicating where I'd reached in my review, so assume I didn't review this one file.
Changes to randomizer handling after I realized that it couldn't possibly be generated using the message as an input. It does simplify things too.
Some additional fixes (using "Re-Randomized FROST" consistently; updating Owners)
fixes #1051