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

More concise share backups #182

Merged
merged 7 commits into from
Jul 4, 2024
Merged

Conversation

nickfarrow
Copy link
Collaborator

Exploring smaller backups by removing polynomial identifier and threshold (should be written in plaintext alongside the bech32 backup).

@nickfarrow nickfarrow marked this pull request as ready for review May 15, 2024 10:45
@LLFourn LLFourn force-pushed the small-share-backups branch 4 times, most recently from fb4502a to 1b6a720 Compare July 3, 2024 04:43
@LLFourn
Copy link
Owner

LLFourn commented Jul 3, 2024

@nickfarrow ready for your review. Review each commit independently is probably best.

) {
let b = bpoly.into_iter();
for (i, b) in b.enumerate() {
if i == apoly.len() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

at first glance i thought this was wrong because of the == for unequal length polys, but the following push makes it repeat this branch on the next iteration 👍

@nickfarrow
Copy link
Collaborator Author

frost shares should be Zero

Could you explain this one to me, why allow a frost secret share to be zero?

While zero isnt inherently forbidden for secret shares, it should never occur from a secure keygen, only if someone is cancelling out secret poly(s) and that might be worth panicking (perhaps not in this way!). I want to understand when to make things 'illegal', only when it's actually illegal within that context (like send_pubkey_to_bob example in readme)?

fn recover_secret(parties in 1usize..10, threshold in 1usize..5) {
use rand::seq::SliceRandom;
let frost = frost::new_with_deterministic_nonces::<sha2::Sha256>();
let parties = parties.max(threshold);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

could use (parties, threshold) in (1usize..=10).prop_flat_map(|n| (Just(n), 1usize..=n)) but it's also kind of confusing

nickfarrow and others added 7 commits July 3, 2024 20:31
- More understandable (and faster) internals
- Added `lagrange_basis_poly` function. Made it public because why not.
- Added `scalar::mul` function to multiply two polynomials
- Added `add_in_place` functions
- Fixed missing API surface area
Some funny expect was turning them into `NonZero`. It made no sense in
the current code and maybe never did!
Having a concept of a secret share seemed to be useful generally. This
makes the share backup feature less boxed off.

Also we now embed the share index in the human readable part when we can.
@nickfarrow
Copy link
Collaborator Author

minor fixups in 5d86d94, re-added and updated frost proptest. Everything LGTM!

@LLFourn
Copy link
Owner

LLFourn commented Jul 4, 2024

frost shares should be Zero

Could you explain this one to me, why allow a frost secret share to be zero?

While zero isnt inherently forbidden for secret shares, it should never occur from a secure keygen

This is true for every value that Scalar can take. The easy way to tell then is if your concern about 0 would equally apply to 42 or SHA256("malicious key"), then there's no problem with 0.

@LLFourn LLFourn merged commit c15a3e4 into LLFourn:master Jul 4, 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