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

Improve testability regarding randomness #427

Closed
conradoplg opened this issue Apr 19, 2023 · 1 comment
Closed

Improve testability regarding randomness #427

conradoplg opened this issue Apr 19, 2023 · 1 comment

Comments

@conradoplg
Copy link
Collaborator

@FiloSottile reached out to me to convince us of adjusting the FROST spec to make it easier to test regarding randomness, re: https://words.filippo.io/dispatches/avoid-the-randomness-from-the-sky/

We already do a good job at it, though some parts are kind of implicit. For example, nonce_generate(secret) samples randomness so in principle it can't be tested, but we do provide test vectors for a specific random_bytes so there as an implicit "nonce_generate_for_random_bytes(secret, random_bytes)" function that could be tested instead. Note that this is kinda part of #420; the reference implementation does have a nonce_generate(H, secret, random_bytes)

The other place that involves randomness is trusted_dealer_keygen(secret_key) which samples a bunch of random scalars. We again provide test vectors for a given list of scalars for an implicit "trusted_dealer_keygen_for_coefficients(secret_key, coefficients)" function.

That leaves the G.RandomScalar() function which is what generates those coefficients (interestingly, it's only used in trusted_dealer_keygen). Currently we don't specify it and refer to the appendix for how to implement it with wide reduction or rejection sampling. But we could have simply defined it with:

def random_scalar():
  random_bytes = random_bytes(64)
  return H3(random_bytes)

(though I guess we can't use H3 and we'd need Yet Another Hash Function, but the idea is the same. Though would it be fine in this case, since the input is random?)

That would allow implementers to test RandomScalar() and would also make it harder for implementers to botch its implementation. And would also allow us to remove that Appendix.

I appreciate that we are very close to get the RFC published so we probably won't want to change this, but here are some suggestions:

  1. Make the spec a bit more explicit about what can be tested by refactoring nonce_generate() into nonce_generate_for_random_bytes() and making nonce_generate() sample random_bytes and call it (or anything similar, I'm bad at naming). Same applies for trusted_dealer_keygen. This is closely related to Make spec and reference implementation identical #420.
  2. Remove G.RandomScalar() and the appendix about randomness, and specify random_scalar() as above instead. Then change the test vectors to provide the random_bytes inputs to random_scalar instead of the random coefficients.
  3. If we don't want to change the RFC, we could do those just in the PoC code and in the Rust code
@chelseakomlo
Copy link
Collaborator

This is a useful goal to aim for in general but this feedback/effort would have been more helpful earlier in the process. Closing because we are (hopefully) close to completion.

@mpguerra mpguerra added this to FROST Sep 6, 2023
@mpguerra mpguerra moved this to Done in FROST Sep 6, 2023
@mpguerra mpguerra removed this from FROST Sep 27, 2023
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

No branches or pull requests

2 participants