[frost] Make "tweaks" mutate #190

LLFourn · 2024-07-25T09:27:54Z

Prior to this PR tweaks were being stored in a separate field inside FrostKey and were applied at signature combination time. In frostsnap we actually want to be able to do "one-way" tweaks where you forget the original polynomial (at least the first coefficient). This is not an API safety concern but we strictly don't want the original to exist anymore in memory.

The next problem we address is that we don't want you to need the full polynomial for signing. Instead we make the user provide a "paired" secret share which is the secret scalar and index paired with the shared key (first coefficient of polynomial). This is a convenient combination since it allows you to hash the shared key to figure out a tweak and then apply it to both the shared key and the shared secret.

See commits individually. eb6dc14 is the frost only one.

I haven't updated docs yet because I intend to integrate some bip-frost stuff which will change the API again. I haven't updated the examples yet so this is not really mergable.

In case you want a non-zero point generically

Prior to this commit tweaks were being stored in a separate field inside `FrostKey` and were applied at signature combination time. In frostsnap we actually want to be able to do "one-way" tweaks where you forget the original polynomial (at least the first coefficient). This is not an API safety concern but we strictly don't want the original to exist anymore. The next problem we address is that we don't want you to need the full polynomial for signing. Instead we make the user provide a "paired" secret share which is the secret scalar and index paired with the shared key (first coefficient of polynomial). This is a convenient combination since it allows you to hash the shared key to figure out a tweak and then apply it to both the shared key and the shared secret.

See: #189

So it doesn't start with zero points.

LLFourn · 2024-07-26T07:20:40Z

This is reviewable now. Please excuse poor documentation. I think I'll do a full sweep after I've attempted implementing the bip.

See #189 for why I did multiplicative tweaking.

irriden · 2024-07-26T23:50:28Z

Thank you for the PR !

I have rebased my work on top of this PR here, everything ok.

I also tested add and mul tweaks on signet, separately and together, both worked.

The awkward parts were 1) applying tweaks to both SharedKey and PairedSecretShare 2) the function signature difference between homomorphic_add and homomorphic_mul.

Consider these minor nits :)

Thank you again ! Also particularly appreciate recover_secret, this is useful in a LN context.

LLFourn · 2024-07-28T07:49:43Z

The awkward parts were 1) applying tweaks to both SharedKey and PairedSecretShare

Hmm I was actually thinking you wouldn't have to do this unless in your protocol signers are also verifying signature shares (which I guess in LN context is likely).

The function signature difference between homomorphic_add and homomorphic_mul

Good point I think homomorphic_mul should just own self as well.

irriden · 2024-07-29T05:53:21Z

Hmm I was actually thinking you wouldn't have to do this unless in your protocol signers are also verifying signature shares (which I guess in LN context is likely).

What drives this for me at the moment: coordinator_sign_session requires the SharedKey, and I assume that it needs the tweaked SharedKey. Let me know if this assumption is wrong thank you !

EDIT: and the CoordinatorSignSession returned is needed to combine signature shares.

LLFourn · 2024-07-29T06:47:13Z

@irriden I can relax this a bit but you will need CoordinatorSignSession to verify the signature shares anyway or are you not doing that in this case?

To make arithmetic API simpler at the cost of making the caller call `.non_zero()` etc

LLFourn · 2024-07-29T07:56:27Z

@irriden see the two last commits. Hopefully the tweaking is more straightforward now. There is no longer two variants of tweakings "xonly" and normal tweaks. To do an xonly tweak you just do a normal homomorphic_add then call non_zero and then into_xonly. This is more steps for the caller but I think more natural and means less code.

I've made it so you don't need a CoordinatorSignSession to create the final signature. I'm guessing you are just checking the final signature for validity?

irriden · 2024-07-29T23:48:47Z

I can relax this a bit but you will need CoordinatorSignSession to verify the signature shares anyway or are you not doing that in this case?

Right now I am not verifying signature shares, I am leaving identifiable aborts for later :)

To do an xonly tweak you just do a normal homomorphic_add then call non_zero and then into_xonly. This is more steps for the caller but I think more natural and means less code.

All sounds great thank you !

I've made it so you don't need a CoordinatorSignSession to create the final signature.

Seems to me PartySignSession is created from fn party_sign_session, which is a function that requires the agg_binonce as a parameter, and this agg_binonce can only be produced by a CoordinatorSignSession. Looks like this is also the case for the parties parameter of fn party_sign_session, it can only be produced by CoordinatorSignSession.

So CoordinatorSignSession is still needed to create the final signature isn't it?

EDIT: I do see that these two params could be created explicitly without creating a CoordinatorSignSession.

Do not worry too much about this, I'm happy to ensure tweaks are applied to both SharedKey and PairedSecretShare simultaneously.

I'm guessing you are just checking the final signature for validity?

Yes, just the final signature, not the partial signatures.

So it's clearer how you can avoid a coordinator

LLFourn · 2024-07-30T03:18:38Z

See 4e22279 where I make a method to make it more discoverable how this is meant to work.

irriden · 2024-07-30T17:15:24Z

See 4e22279 where I make a method to make it more discoverable how this is meant to work.

Thank you !

I am going to keep using the CoordinatorSignSession route. My coordinator for now keeps a copy of SharedKey in order to check the aggregate signature at the last step, and I'm happy to provide a tweaked SharedKey to coordinator_sign_session.

Nonetheless seems to me this would be particularly useful in the case where one of the parties is used as the coordinator.

LLFourn · 2024-07-30T23:31:58Z

schnorr_fun/src/frost/shared_key.rs

+    /// Constructor to create a from a vector of points where each item represent a polynomial
+    /// coefficient.
+    ///
+    /// Returns `None` if the first coefficient is [`Point::zero`].


This is not true anymore. It can be None is the poly is empty. I think I might make two different from_polys one for Zero and another for NonZero. The Zero one will be infallible.

[edit] maybe just the Zero one and call non_zero to get the non_zero one.

Pair the secret shares using the shared key so you know it's correct. Also cleaned up the accessing the public key in order to do the implementation. ..and some other misc changes 🤗

No need to clone here

As well as make more generic the bincode/serde impls

nickfarrow · 2024-08-06T20:25:32Z

schnorr_fun/src/frost/shared_key.rs

+    /// The resulting shared key will be `SharedKey<Normal, Zero>`. It's up to the caller to do the zero check with [`non_zero`]
+    ///
+    /// [`non_zero`]: Self::non_zero
+    pub fn from_poly(poly: Vec<Point<Normal, Public, Zero>>) -> Self {


Is this again because there's nothing special about preventing Zero for a shared key?

I always feel weird seeing it allowed. Why avoid panics when the user passes an empty poly for the shared key? (maybe they just don't know it but have a secret share?)

nickfarrow · 2024-08-06T20:27:46Z

schnorr_fun/src/frost/session.rs

+    }
+
+    /// The public key the session was started under
+    pub fn public_key(&self) -> Point<EvenY> {


I liked how you re-allowed SharedKey to be Normal, but here the signing session is forced into a BIP340 context. No problem for us but maybe other people would want signing for Normal keys?

There is no way to sign with Normal keys though.

schnorr_fun/src/frost/share.rs

nickfarrow · 2024-08-06T20:38:24Z

schnorr_fun/src/frost/shared_key.rs

+    /// encoded as 33 bytes.
+    ///
+    /// ⚠ Unlike other secp256kfun things this doesn't exactly match the serde/bincode
+    /// implemenations which will length prefix the list of points.


I am curious as to why? Or is this just a note that serde/bincode prefix the length so you'll have to keep that in mind if you mix & match?

This is the first time I'm creating a to_bytes on something that is variable length and I just felt like noting the difference. Maybe the comment is more trouble than it's worth. You definitely shouldn't mix and match.

nickfarrow

I noticed you made all the public keys shared_key and then changed back. Looks like they are no longer going with shared_key on the BIP also!

Very much like 🐙 = coordinator

Proptest with all the variations of tweaks looks very robust.

One completely unrelated thing I just noticed is that create_shares_and_pop is kind of confusing, makes it sound like something is getting popped off a vec.

I didn't look too deeply into the secp256kfun casting between point types etc. but the idea of checking the types like this makes sense.

There's quite a few spelling mistakes, not sure whether you care much for that, happy to fix up in commit or another PR. Everything else LGTM!

signature share verification was unnecessarily slow because you get to regenerate their whole verification share each time. Now you can cache it. Also verification shares are only for EvenY keys. We now have a concept of "share image" which is more general I'm not entirely sure I like but I'll leave it in for now.

LLFourn · 2024-08-08T03:38:35Z

I just pushed a commit to make "verification shares" a first class type. Now you use the verification shares to verify signature shares. @irriden you can now create a polynomial from "share images" rather than "verification shares" because verification shares are strictly something you can verify a signature share against (i.e. homomorphically linked to a Point<EvenY>.

One completely unrelated thing I just noticed is that create_shares_and_pop is kind of confusing, makes it sound like something is getting popped off a vec.

heh it will be gone soon I hope.

There's quite a few spelling mistakes, not sure whether you care much for that, happy to fix up in commit or another PR. Everything else LGTM!

Feel free to push doc fixes you catch at anytime. I fixed up a few given that you made me self-conscious about it!

There's no need for the `Frost` type to do signing/verification. It's needed to start the session but not after.

LLFourn · 2024-08-08T07:31:08Z

Added 84ac38b which made things make a bit more sense.

irriden · 2024-08-23T17:26:35Z

Hi @LLFourn,

Thank you for the ping.

For now I remain on 151c6d6.

bip-frost-dkg outputs this:

class DKGOutput(NamedTuple):
    secshare: Optional[bytes]
    threshold_pubkey: bytes
    pubshares: List[bytes]

where pubshares are verification shares.

That's all the info that an individual signer gets. The individual signer doesn't get the "share images" of the other signers.

My solution will be to get rid of the bip-frost-dkg chilldkg reference implementation, and replace it with the secp256kfun version.

LLFourn added 3 commits July 25, 2024 19:23

[core] Allow Point::is_zero on Z: ZeroChoice

0f403a6

In case you want a non-zero point generically

[musig] Make musig mod a bit more like frost mod

bbaf252

LLFourn requested a review from nickfarrow July 25, 2024 09:28

LLFourn force-pushed the tweak-mutate branch 2 times, most recently from ec8a662 to 5bea2bd Compare July 26, 2024 07:18

LLFourn added 2 commits July 26, 2024 17:19

[frost] Add multiplicative tweaking by demand

0e4ba72

See: #189

[poly] Reduce interpolated polynomial

b331864

So it doesn't start with zero points.

LLFourn force-pushed the tweak-mutate branch from 5bea2bd to 40191a9 Compare July 26, 2024 07:19

[frost] Renaming and docs

8a80c80

LLFourn force-pushed the tweak-mutate branch from 4e1d33e to 8a80c80 Compare July 26, 2024 07:27

[frost] Make SharedKey and PairedSecretShare zeroable

2f00a45

To make arithmetic API simpler at the cost of making the caller call `.non_zero()` etc

LLFourn force-pushed the tweak-mutate branch 2 times, most recently from 15677b5 to 29962aa Compare July 29, 2024 07:51

[frost] make verifying and combining signature shares better

dda6cdf

LLFourn force-pushed the tweak-mutate branch from 29962aa to dda6cdf Compare July 29, 2024 07:55

[frost] add Frost::aggregate_binonces

4e22279

So it's clearer how you can avoid a coordinator

LLFourn commented Jul 30, 2024

View reviewed changes

LLFourn force-pushed the tweak-mutate branch from e2cffda to a2a54ec Compare August 1, 2024 01:03

LLFourn added 2 commits August 1, 2024 11:05

[❄] PairedSecretShare construction safety

cd1847d

Pair the secret shares using the shared key so you know it's correct. Also cleaned up the accessing the public key in order to do the implementation. ..and some other misc changes 🤗

[schnorr] Make Signature Copy

0a6b572

[❄] Make point_polynomial return a slice

6c37a37

No need to clone here

LLFourn force-pushed the tweak-mutate branch from a2a54ec to e461fee Compare August 1, 2024 01:05

[❄] Implement SharedKey::to_bytes/from_slice

151c6d6

As well as make more generic the bincode/serde impls

LLFourn force-pushed the tweak-mutate branch from e461fee to 151c6d6 Compare August 1, 2024 01:10

nickfarrow reviewed Aug 6, 2024

View reviewed changes

schnorr_fun/src/frost/share.rs Show resolved Hide resolved

nickfarrow reviewed Aug 6, 2024

View reviewed changes

nickfarrow approved these changes Aug 6, 2024

View reviewed changes

[❄] Move signature methods onto the sessions

84ac38b

There's no need for the `Frost` type to do signing/verification. It's needed to start the session but not after.

LLFourn merged commit 646b969 into master Aug 9, 2024
15 checks passed

LLFourn deleted the tweak-mutate branch August 9, 2024 01:00

LLFourn mentioned this pull request Aug 9, 2024

Clean up frost #191

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[frost] Make "tweaks" mutate #190

[frost] Make "tweaks" mutate #190

LLFourn commented Jul 25, 2024 •

edited

Loading

LLFourn commented Jul 26, 2024 •

edited

Loading

irriden commented Jul 26, 2024

LLFourn commented Jul 28, 2024

irriden commented Jul 29, 2024 •

edited

Loading

LLFourn commented Jul 29, 2024

LLFourn commented Jul 29, 2024

irriden commented Jul 29, 2024 •

edited

Loading

LLFourn commented Jul 30, 2024

irriden commented Jul 30, 2024

LLFourn Jul 30, 2024 •

edited

Loading

nickfarrow Aug 6, 2024

nickfarrow Aug 6, 2024

LLFourn Aug 7, 2024

nickfarrow Aug 6, 2024

LLFourn Aug 7, 2024

nickfarrow left a comment

LLFourn commented Aug 8, 2024

LLFourn commented Aug 8, 2024

irriden commented Aug 23, 2024

[frost] Make "tweaks" mutate #190

[frost] Make "tweaks" mutate #190

Conversation

LLFourn commented Jul 25, 2024 • edited Loading

LLFourn commented Jul 26, 2024 • edited Loading

irriden commented Jul 26, 2024

LLFourn commented Jul 28, 2024

irriden commented Jul 29, 2024 • edited Loading

LLFourn commented Jul 29, 2024

LLFourn commented Jul 29, 2024

irriden commented Jul 29, 2024 • edited Loading

LLFourn commented Jul 30, 2024

irriden commented Jul 30, 2024

LLFourn Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

nickfarrow Aug 6, 2024

Choose a reason for hiding this comment

nickfarrow Aug 6, 2024

Choose a reason for hiding this comment

LLFourn Aug 7, 2024

Choose a reason for hiding this comment

nickfarrow Aug 6, 2024

Choose a reason for hiding this comment

LLFourn Aug 7, 2024

Choose a reason for hiding this comment

nickfarrow left a comment

Choose a reason for hiding this comment

LLFourn commented Aug 8, 2024

LLFourn commented Aug 8, 2024

irriden commented Aug 23, 2024

LLFourn commented Jul 25, 2024 •

edited

Loading

LLFourn commented Jul 26, 2024 •

edited

Loading

irriden commented Jul 29, 2024 •

edited

Loading

irriden commented Jul 29, 2024 •

edited

Loading

LLFourn Jul 30, 2024 •

edited

Loading