Skip to content
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

FROST Implementation #84

Merged
merged 38 commits into from
Sep 6, 2022
Merged

FROST Implementation #84

merged 38 commits into from
Sep 6, 2022

Conversation

nickfarrow
Copy link
Collaborator

@nickfarrow nickfarrow commented Apr 4, 2022

FROST (Flexible Round-Optimized Schnorr Threshold) implementation

  • Only handles x-only tweaking (no ordinary tweaks)
  • t-of-n keygen and signing proptest with random parties, threshold, and signers. Proptest carries out keygen and then uses a random mask to create a set of signers who create a combined signature.
  • Docs mostly in synopsis, do we want to add usage docs to particular functions also?
  • End-to-end test

This implementation has not yet been made compatible with other FROST implementations or forthcoming specifications.
BlockstreamResearch/secp256k1-zkp#138
https://github.com/isislovecruft/frost-dalek
https://github.com/cfrg/draft-irtf-cfrg-frost

@nickfarrow nickfarrow force-pushed the frost branch 8 times, most recently from 84ed1d6 to 2198d9d Compare April 11, 2022 07:09
@sanket1729
Copy link

Thanks for looking at this. Looking forward to the PR.

  • Would this be BIP 340 compatible?

  • Can this support BIP 32 derivations?

@nickfarrow
Copy link
Collaborator Author

nickfarrow commented Apr 12, 2022

  • Would this be BIP 340 compatible?

We're making sure joint public keys have even Y coordinates, and are using FROST specific key-prefixing by tending to hash all public information where available. As well as tagged hashes. So i believe so!

  • Can this support BIP 32 derivations?

Currently this code produces a single joint public key for the multisignature (which can be tweaked). I'm actually not too sure how exactly address derivation will work, i think same as MuSig. But my naive understanding would be that the joint public key could somehow be used for the BIP32 parent public key, maybe @LLFourn has some ideas here

Edit: We've implemented both FrostKey and XOnlyFrostKey allowing for plain tweaking (bip32) and xonly tweaking (taproot commitments)

@nickfarrow nickfarrow force-pushed the frost branch 3 times, most recently from d99a86e to 2df81f4 Compare April 20, 2022 04:50
@nickfarrow nickfarrow changed the title WIP frost docs WIP FROST Implementation Apr 28, 2022
@nickfarrow nickfarrow force-pushed the frost branch 4 times, most recently from 8572171 to 9543b20 Compare May 6, 2022 06:53
@nickfarrow nickfarrow changed the base branch from frost to master May 9, 2022 05:30
@nickfarrow nickfarrow force-pushed the frost branch 5 times, most recently from 6349a67 to d942bf5 Compare May 16, 2022 04:53
@nickfarrow nickfarrow changed the title WIP FROST Implementation FROST Implementation May 16, 2022
@nickfarrow nickfarrow marked this pull request as ready for review May 16, 2022 08:17
@nickfarrow nickfarrow mentioned this pull request May 16, 2022
let mask_bytes = signers_mask_seed.to_be_bytes();
for i in 0..n_parties {
let rand_pos = mask_bytes[(i % mask_bytes.len() as u32) as usize] % n_parties as u8;
signer_mask.swap(i as usize, rand_pos as usize);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is a shuffle function in rand.

Copy link
Collaborator Author

@nickfarrow nickfarrow May 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok cool, was just about to ask this. So OK to use rand in proptest? So long as it is deterministic using signers_mask_seed as seed for randomness

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh good point. It's ok to use something implementing Rng to call shuffle as long as it's deterministic. I wonder if proptest has an easy way to get the rng it uses internally? See: https://docs.rs/proptest/latest/proptest/test_runner/struct.TestRunner.html#method.rng

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldnt figure out how to use that .rng method without getting rid of the convenient proptest! macro. Instead i've switched to:

signer_mask.shuffle(&mut TestRng::deterministic_rng(RngAlgorithm::ChaCha);

where TestRng::deterministic_rng(RngAlgorithm::ChaCha) seems to provide the necessary deterministic randomness

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I don't think this is doing quite what we want, it doesn't vary as the proptest runs. Trying again

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't figure out a nice way to get the current run context's RNG without completely scrapping the macro and doing everything manually.

Leaning towards returning to using a proptest argument for the signer mask seed. I am thinking of using a rand integer that is then used as a seed for RNG.

See this branch & line:
nickfarrow@7d95699#diff-65e82911697a71b2673ac046e6851aa3cfa3296ad1735eb1c283f381d131fe1fR898
Random 32 bytes being fed into ChaCha seed
image
(will force push here unless we can come up with a better idea!)

@nickfarrow
Copy link
Collaborator Author

doc test now seems to be failing because of some unrelated file

 Documenting ecdsa_fun v0.7.1 (/home/runner/work/secp256kfun/secp256kfun/ecdsa_fun)
warning: unresolved link to `derive_nonce`
   --> secp256kfun/src/nonce.rs:148:41
    |
148 | /// In general it's better to use the [`derive_nonce`] macro than to call
    |                                         ^^^^^^^^^^^^ no item named `derive_nonce` in scope

Signed-off-by: nickfarrow <[email protected]>
Signed-off-by: nickfarrow <[email protected]>
Signed-off-by: nickfarrow <[email protected]>
Signed-off-by: nickfarrow <[email protected]>
* docs updates
* use message::raw
* rename joint_public_key public_key
* remove unecessary AddTag typing
* add Frost.binding_hash
* handle zero aggregate nonce like musig
Copy link
Owner

@LLFourn LLFourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I rebased and made one small fixup. Let's merge this. Unfinished business:

  1. Nonce generation API can use some refinement
  2. verification shares should be allowed to be zero
  3. nonce generation should be more like ietf spec. (probably more things should be -- we should read through it and see what's different).

@LLFourn LLFourn merged commit 3c54923 into LLFourn:master Sep 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants