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

implement chillidkg #192

Merged
merged 5 commits into from
Aug 29, 2024
Merged

implement chillidkg #192

merged 5 commits into from
Aug 29, 2024

Conversation

LLFourn
Copy link
Owner

@LLFourn LLFourn commented Aug 14, 2024

Implements the ideas from the bip-frost spec. This is a WIP. I think I like the new spec way better so I want to make proper errors for the error cases as well do documentation and renaming. Then delete the old key generation.

This deviates from the spec in several ways that I won't bother to list but they are mostly superficial things that I'll either fix or make an issue about.

@LLFourn LLFourn marked this pull request as draft August 14, 2024 06:54
@LLFourn LLFourn changed the title wip implement chillidkg Aug 14, 2024
We weren't following any of the FROST papers in how to compute the
binding coefficient. To follow [1] all we have to do is hash the
parties involved.

[1]: https://eprint.iacr.org/2023/899
@LLFourn LLFourn marked this pull request as ready for review August 26, 2024 02:01
@LLFourn
Copy link
Owner Author

LLFourn commented Aug 26, 2024

This is ready for review now.

  • implemented (my loose interpretation) of chilldkg
  • Remove all of our existing keygen stuff
  • Also fca38b5

let dh_key = g!(multi_nonce_keypair.secret_key() * encryption_key).normalize();
let pad = Scalar::from_hash(H::default().add(dh_key).add(encryption_key).add(dest));
let payload = s!(pad + share).public();
(*dest, payload)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't quite match the encryption of encpedpop here.

We calulate dh_key the same way.
Their hash is different to ours:

  • Hash( "encpedpop ecdh", dh_key || my_pubkey || their_pubkey || context_data )
  • I think their my_pubkey/pubnonce we have as multi_nonce_keypair.public_key(), not encryption_key?
  • Our hash is missing context data that sounds important? See this comment: This hashed ElGamal's "crucial feature is to feed the index of the enckey to the hash function"

Copy link
Owner Author

@LLFourn LLFourn Aug 28, 2024

Choose a reason for hiding this comment

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

I instead implemented it like in the paper referenced:

image

  1. Yes there a few instances of this. I am just putting in what I think is actually important because I'm lazy and I don't know how this will actually turn out byte-for-byte yet. It's a very good idea to look closely at this though and ask questions (as you've just done!).
  2. my_pubkey is encryption_key and pubnonce is multi_nonce_keypair.public_key(). I'm not sure what the right name. Maybe it shouldn't be called encryption_key because we later transform it into a signing key.
  3. The index is dest.

Copy link
Collaborator

@nickfarrow nickfarrow Aug 28, 2024

Choose a reason for hiding this comment

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

That makes sense.

Theyre passing their pubnonce into my_pubkey for their encryption hash, bit confusing. So they're actually including multi_nonce_keypair.public_key() in their hash

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yes. pubnonce is definitely superfluous since it's a function of dh_key and my_pubkey. There is an approach to these kinds of "what to hash" question which is just hash every single thing without giving a particular justification for it. That's a fine approach but it requires extra care when specifying the thing otherwise you end up with concatenating stuff like "context_data" everywhere and it gets unwieldy.

@LLFourn LLFourn merged commit 32eb006 into master Aug 29, 2024
15 checks passed
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.

2 participants