LLFourn / secp256kfun

A pure-rust secp256k1 library optimised for fun
BSD Zero Clause License
101 stars 29 forks source link

[frost] Make "tweaks" mutate #190

Closed LLFourn closed 1 month ago

LLFourn commented 1 month ago

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. eb6dc14994d0dc99e1b1118d58d7128aff386ca5 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.

LLFourn commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

@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?

LLFourn commented 1 month ago

@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 commented 1 month ago

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.

LLFourn commented 1 month ago

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

irriden commented 1 month ago

See https://github.com/LLFourn/secp256kfun/commit/4e222791dad3cde339aa65d9b5f52929b96f74f4 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 commented 1 month ago

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!

LLFourn commented 1 month ago

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

irriden commented 2 weeks ago

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.