-
Notifications
You must be signed in to change notification settings - Fork 208
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
FROST Trusted Dealer #278
base: master
Are you sure you want to change the base?
FROST Trusted Dealer #278
Conversation
6078d4b
to
ab3c7a6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some initial comments, more tomorrow
include/secp256k1_frost.h
Outdated
/** Serialize a FROST share | ||
* | ||
* Returns: 1 when the share could be serialized, 0 otherwise | ||
* Args: ctx: a secp256k1 context object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: see bitcoin-core/secp256k1@aa3dd52 for consistent wording here
include/secp256k1_frost.h
Outdated
* Each participant _must_ have a secure channel with the trusted dealer with | ||
* which they can transmit shares to each other. | ||
* | ||
* A new seed32 _must_ be used for each key generation session. The trusted |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/new/fresh
By the way, we need to say similar things for musig sessions. Feel free to steal some sentences/phrasing ideas from bitcoin-core/secp256k1#1479.
include/secp256k1_frost.h
Outdated
* start a new session to generate a new key, they must NOT REUSE their | ||
* respective seed32 again, but instead generate a new one. It is recommended |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/again// ("reuse again" is saying the same thing twice)
include/secp256k1_frost.h
Outdated
const unsigned char *in32 | ||
) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3); | ||
|
||
/** Creates key generation shares |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"key shares" or "key generation" shares?
include/secp256k1_frost.h
Outdated
* Each participant _must_ have a secure channel with the trusted dealer with | ||
* which they can transmit shares to each other. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, this may be slightly confusing because one could read it as "must have established a secure channel already", but that's of course not necessary. Maybe: "The trusted dealer must transmit shares over secure channels to participants."
src/modules/frost/keygen_impl.h
Outdated
secp256k1_sha256_write(&sha, polygen, 16); | ||
secp256k1_sha256_finalize(&sha, polygen); | ||
|
||
/* Derive share */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/share/shares
src/modules/frost/keygen_impl.h
Outdated
for (i = 0; i < n_participants; i++) { | ||
secp256k1_scalar share_i, idx; | ||
|
||
secp256k1_scalar_clear(&share_i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is just for nuking memory. Use _scalar_set_int(0)
to set to zero. (We should probably have a dedicated function, but we don't have one currently.)
include/secp256k1_frost.h
Outdated
* Args: ctx: pointer to a context object | ||
* Out: shares: pointer to the key generation shares | ||
* pubshares: pointer to the public verification shares | ||
* pk: pointer to the x-only public key |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be an x-only key? I'm not entirely sure, I would need to think about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function conditionally inverts the shares to guarantee that the private key will match the x-only key.
Since the primary use-case will be on-chain signing, I think it's a useful simplification to take care of the negation during share generation and makes it easier to reason about in secp256k1_frost_pubkey_get and secp256k1_frost_pubkey_tweak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Upon further reflection, I'm realizing it's not a useful simplification because in practice the output of the DKG will typically be further tweaked by a BIP32 tweak and then a taproot tweak, rather than used directly.
So I now think it would be better to have the DKG output a 33-byte key rather than an x-only key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've taken a stab at implementing this change, however, it requires tracking more state during signing.
Currently, nonce_process
takes an xonly agg_pk
and optionally a tweak_cache
and returns a session
. partial_sign
takes a session
and optionally a tweak_cache
.
partial_sign
only needs to handle negation logic if a tweak_cache
is present because the DKG outputs an x-only key. If we change the DKG to output a 33-byte key, then nonce_process
will need to take a 33-byte agg_pk
and it will need to save a parity bit, serialized as a byte, in the session
, so that partial_sign
can handle the negation logic for when a tweak_cache
is not present.
So while I don't think there's any reason for the protocol to output a 33-byte key, it seems useful to have the implementation do so, because it avoids a serialized byte in the session
and I don't see a downside.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to compare this approach to the one taken in the signing BIP. The BIP proposes that the pubshares be saved to the session
and the 33 byte public key is derived from the pubshares.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps the best reason to not output an xonly key from the DKG is that we don't want to encourage FROST DKG outputs being used directly on-chain without an unspendable script path because unlike MuSig, FROST does not randomize the group public key: https://github.com/bitcoin/bips/blob/master/bip-0341.mediawiki#cite_note-23
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The signing BIP now uses signer public shares to initialize the tweak context (name to be updated) rather than the group public key. Hence, it no longer depends on whether the group pubkey (produced by keygen) is plain or x-only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative design choice here is for trusted_gen
to always output tweak_cache
and optionally group_pk
. This would be similar to how musig_pubkey_agg
outputs keyagg_cache
and optionally agg_pk
.
In general, I think it would be nice to get this rebase this. (Should be mostly trivial.) Also, CMake support is missing because we have this now for all other -zkp modules (#288 ), but that's currently not essential right now. |
src/modules/frost/session_impl.h
Outdated
ARG_CHECK(n_pubnonces > 1); | ||
ARG_CHECK(my_id != 0); | ||
for (i = 0; i < n_pubnonces; i++) { | ||
ARG_CHECK(ids[i] != 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we also ARG_CHECK
for 1 <= ids[i] <= max_participant
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function doesn't currently take max_participant
as an input, and I'm not sure if it's worth requiring the extra state to perform this check.
const unsigned char *msg32, | ||
const secp256k1_xonly_pubkey *agg_pk, | ||
const unsigned char *extra_input32 | ||
) SECP256K1_ARG_NONNULL(1) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(3) SECP256K1_ARG_NONNULL(4); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can also add pubshare
as an input argument. MuSig2 hashes the individual pubkey when generating the nonces, but we don't need to make pubshare
a mandatory argument like MuSig2 does (more info).
How do we decide the args supplied to the hash function for generating a nonce? Should we throw in all available signer information into the hash function? For instance, can we include the participant identifier too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related issue: siv2r/bip-frost-signing#15, which suggests using tweak_cache
instead of agg_pk
as the input arg.
ab3c7a6
to
e94367c
Compare
configure.ac
Outdated
@@ -470,6 +475,14 @@ if test x"$enable_module_ecdsa_adaptor" = x"yes"; then | |||
SECP_CONFIG_DEFINES="$SECP_CONFIG_DEFINES -DENABLE_MODULE_ECDSA_ADAPTOR=1" | |||
fi | |||
|
|||
if test x"$enable_module_frost" = x"yes"; then | |||
if test x"$enable_module_schnorrsig" = x"no"; then | |||
AC_MSG_ERROR([Module dependency error: You have disabled the schnorrsig module explicitly, but it is required by the musig module.]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
%s/musig/frost
configure.ac
Outdated
if test x"$enable_module_frost" = x"yes"; then | ||
AC_MSG_ERROR([FROST module is experimental. Use --enable-experimental to allow.]) | ||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: when we execute ./configure --enable-module-frost
, the following error appears:
configure: error: MuSig module is experimental. Use --enable-experimental to allow.
instead of:
configure: error: FROST module is experimental. Use --enable-experimental to allow.
To resolve this issue, we should reposition the FROST module check before the MuSig experimental module check (lines 486-488).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you try again? I wasn't able to reproduce the issue.
When I run ./configure --enable-module-frost
I get configure: error: FROST module is experimental. Use --enable-experimental to allow.
.
src/modules/frost/keygen_impl.h
Outdated
secp256k1_sha256_finalize(&sha, polygen); | ||
|
||
/* Derive share */ | ||
/* See draft-irtf-cfrg-frost-08#appendix-C.1 */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: we can update this to refer to the completed RFC: https://datatracker.ietf.org/doc/rfc9591/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/modules/frost/session_impl.h
Outdated
/* TODO: this doesn't have the same sidechannel resistance as the BIP340 | ||
* nonce function because the seckey feeds directly into SHA. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. We must XOR key32
and session_id
(in const-time) before feeding them to SHA256
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to bring these changes over from MuSig: 206017d
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/group.h
Outdated
static void secp256k1_point_save_ext(unsigned char *data, secp256k1_ge *ge); | ||
|
||
static void secp256k1_point_load_ext(secp256k1_ge *ge, const unsigned char *data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The libsecp repo defines these functions using different names:
point_save_ext
->ge_to_bytes_ext
point_load_ext
->ge_from_bytes_ext
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
include/secp256k1_frost.h
Outdated
* 3. Avoid copying (or serializing) the secnonce. This reduces the possibility | ||
* that it is used more than once for signing. | ||
* | ||
* Remember that nonce reuse will leak the secret key! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be more precise to say "will leak the agg_share" instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/modules/frost/session_impl.h
Outdated
secp256k1_sha256 sha; | ||
unsigned char seed[32]; | ||
unsigned char i; | ||
enum { n_extra_in = 4 }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
enum { n_extra_in = 4 }; | |
enum { N_EXTRA_IN = 4 }; |
src/modules/frost/session_impl.h
Outdated
for (i = 0; i < n_extra_in; i++) { | ||
unsigned char len; | ||
if (extra_in[i] != NULL) { | ||
len = 32; | ||
secp256k1_sha256_write(&sha, &len, 1); | ||
secp256k1_sha256_write(&sha, extra_in[i], 32); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this approach to hashing optional arguments—it’s cleaner than musig’s approach of using a helper function. However, there's a minor issue: the signing BIP encodes the message length as 8 bytes, but here we use 1 byte.
As stated in the BIP's noncegen:
- If the optional argument m is not present:
- Let m_prefixed = bytes(1, 0)
- Else:
- Let m_prefixed = bytes(1, 1) || bytes(8, len(m)) || m
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I switched over to using the MuSig approach: 35f453d
return 1; | ||
} | ||
|
||
static void secp256k1_nonce_function_frost(secp256k1_scalar *k, const unsigned char *session_id, const unsigned char *msg32, const unsigned char *key32, const unsigned char *pk32, const unsigned char *extra_input32) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note for the future: when writing tests, we should skip two noncegen test vectors—(1) empty message and (2) 38-byte message—since our nonce generation currently only supports 32-byte messages.
src/modules/frost/session_impl.h
Outdated
secp256k1_sha256_finalize(&sha, seed); | ||
|
||
for (i = 0; i < 2; i++) { | ||
unsigned char buf[32]; | ||
secp256k1_sha256_initialize(&sha); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The method of hashing arguments in noncegen deviates from the BIP. I believe it currently computes the nonces as
but the BIP computes them as
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/modules/frost/session_impl.h
Outdated
return 1; | ||
} | ||
|
||
int secp256k1_frost_nonce_process(const secp256k1_context* ctx, secp256k1_frost_session *session, const secp256k1_frost_pubnonce * const* pubnonces, size_t n_pubnonces, const unsigned char *msg32, const secp256k1_xonly_pubkey *pk, size_t my_id, const size_t *ids, const secp256k1_frost_tweak_cache *tweak_cache, const secp256k1_pubkey *adaptor) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function follows the nonce aggregation technique from FROST1, but the BIP uses FROST3. We may need to split this function into two:
-
frost_nonce_agg
- computes the aggnonce:$\widetilde{R_1} || \widetilde{R_2}$ where,
$\widetilde{R_1} = \sum \limits_{i=1}^{t} R_{i, 1}$ and$\widetilde{R_2} = \sum \limits_{i=1}^{t} R_{i, 2}$ -
frost_nonce_process
- computes the final nonce:$R_{\text{fin}} = \widetilde{R_1} + b \cdot \widetilde{R_2}$
Musig’s implementation follows the same technique
src/modules/frost/session_impl.h
Outdated
the generator (by simply adding the generator to one of the | ||
malicious nonces), and this does not change the winning condition | ||
of the EUF-CMA game. */ | ||
aggnonce_pt[i] = secp256k1_ge_const_g; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe aggnonce_pt[i]
is allowed to be infinity. The NonceAgg specification serializes them using cybtes_ext
, so we can use ge_to_bytes_ext
here.
Setting the nonce to the generator point only occurs with the final nonce.
src/modules/frost/session_impl.h
Outdated
|
||
/* TODO: consider updating to frost-08 to address maleability at the cost of performance */ | ||
/* See https://github.com/cfrg/draft-irtf-cfrg-frost/pull/217 */ | ||
static int secp256k1_frost_compute_noncehash(const secp256k1_context* ctx, unsigned char *noncehash, const unsigned char *msg, const secp256k1_frost_pubnonce * const* pubnonces, size_t n_pubnonces, const unsigned char *pk32, const size_t *ids) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FROST3 computes the nonce coefficient as
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove the ctx
object from the function args.
src/modules/frost/session_impl.h
Outdated
return 1; | ||
} | ||
|
||
static int secp256k1_frost_nonce_process_internal(const secp256k1_context* ctx, int *fin_nonce_parity, unsigned char *fin_nonce, secp256k1_scalar *b, secp256k1_gej *aggnoncej, const unsigned char *msg, const secp256k1_frost_pubnonce * const* pubnonces, size_t n_pubnonces, const unsigned char *pk32, const size_t *ids) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can remove the ctx
object from the function args
secp256k1_gej_add_ge_var(&fin_nonce_ptj, &fin_nonce_ptj, &aggnonce[0], NULL); | ||
secp256k1_ge_set_gej(&fin_nonce_pt, &fin_nonce_ptj); | ||
|
||
if (secp256k1_ge_is_infinity(&fin_nonce_pt)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The BIP sets the final nonce to the generator point if it’s infinity.
secp256k1_scalar_add(&session_i.s_part, &session_i.s_part, &e_tmp); | ||
} | ||
} | ||
/* Update the challenge by multiplying the Lagrange coefficient to prepare |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hadn’t considered this option when writing the BIP. So, right now, it computes
include/secp256k1_frost.h
Outdated
size_t my_id, | ||
const size_t *ids, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the documentation, we should probably mention that [1] the ids
array must have unique elements, and [2] my_id
must be included in ids
. Otherwise, Lagrange interpolation will return incorrect values.
We could also implement ARG_CHECK
s for these, but I'm not sure if it's worth the effort.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In BIP, the DeriveInterpolatingValue function fails if both these conditions aren't met.
* Args: ctx: pointer to a context object | ||
* In: pre_sig64: 64-byte pre-signature | ||
* msg32: the 32-byte message being verified | ||
* pubkey: pointer to an x-only public key to verify with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* pubkey: pointer to an x-only public key to verify with | |
* agg_pk: pointer to an x-only public key to verify with |
* nonce_parity: the output of `frost_nonce_parity` called with the | ||
* session used for producing the pre-signature | ||
*/ | ||
SECP256K1_API int secp256k1_frost_verify_adaptor( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just pointing out that there's another possible approach here. In single signer Schnorr adaptor, we use extract_adaptor_point
API instead of verify_adaptor
—more details. Interestingly, MuSig doesn’t include either extract_adaptor_point
or verify_adaptor
(not sure why).
src/modules/frost/session_impl.h
Outdated
if (secp256k1_fe_is_odd(&cache_i.pk.y) != cache_i.parity_acc) { | ||
secp256k1_scalar_negate(&sk, &sk); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use this comment here (taken from musig)
/* Negate sk if secp256k1_fe_is_odd(&cache_i.pk.y)) XOR cache_i.parity_acc.
* This corresponds to the line "Let d = g⋅gacc⋅d' mod n" in the
* specification. */
ARG_CHECK(sig64 != NULL); | ||
ARG_CHECK(session != NULL); | ||
ARG_CHECK(partial_sigs != NULL); | ||
ARG_CHECK(n_sigs > 0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ARG_CHECK(n_sigs > 0); | |
ARG_CHECK(n_sigs > 1); |
Should we do this instead?
The frost_nonce_process
currently has ARG_CHECK(n_pubnonce > 1)
. We might want to use the same value in both functions, either 1 or 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In musig's example, every function (except main
) is static
examples/frost.c
Outdated
} | ||
/* Mark signer as assigned */ | ||
pubnonces[i] = &signer[signer_id].pubnonce; | ||
/* pubkeys[i] = &signer[signer_id].pubkey; */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this comment was meant to be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#278 (comment) applies here too.
for (i = 0; i < COUNT; i++) { | ||
frost_multi_hop_lock_tests(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can move this function call to the previous loop
CHECK(secp256k1_gej_is_infinity(&summed_nonces[1])); | ||
} | ||
|
||
int frost_memcmp_and_randomize(unsigned char *value, const unsigned char *expected, size_t len) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: same function is also used in MuSig's tests, so can we make it a helper function in src/testutil.h
?
/* Compute "effective" nonce rj = aggnonce[0] + b*aggnonce[1] */ | ||
/* TODO: use multiexp to compute -s*G + e*pubshare + aggnonce[0] + b*aggnonce[1] */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to use pubnonce
instead of aggnonce
: #296
Responds to: BlockstreamResearch#278 (comment)
Based on: BlockstreamResearch@206017d Responds to: BlockstreamResearch#278 (comment)
Based on: 57de68994ae18d20b0b6e1a9e4531c3d88b5ec81 and 3f9bb4d868a2a27caacdaf19b08ce91ce73c1fb4 Responds to: BlockstreamResearch#278 (comment)
Responds to: BlockstreamResearch#278 (comment)
This commit adds the foundational configuration and building scripts and an initial structure for the project.
This commit adds trusted share generation, as well as share serialization and parsing.
This commit adds share verification, as well as computation of public verification shares.
This commits add BIP-341 ("Taproot") and BIP-32 ("ordinary") public key tweaking.
This commits adds nonce generation, as well as serialization and parsing.
This commit adds nonce aggregation, as well as adaptor signatures.
This commit adds signature generation and aggregation, as well as partial signature serialization and parsing.
d53f97e
to
abb2ee3
Compare
This commit adds an example file to demonstrate how to use the module.
Add api tests, nonce tests, tweak tests, sha256 tag tests, and constant time tests.
This commit adds a documentation file with detailed instructions for how to use the module properly.
abb2ee3
to
f41560c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only looked at share generation/verification so far, left a few comments below. Fwiw, I've added CMake support, feel free to pull it in: theStack@3b72fbd
src/modules/frost/keygen_impl.h
Outdated
/* The x-coordinate must not be zero (see | ||
* draft-irtf-cfrg-frost-08#section-4.2.2) */ | ||
if (secp256k1_scalar_is_zero(indexhash)) { | ||
return 0; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As the chance of this happening is negligible, a VERIFY_CHECK
might be sufficient? Then the function could be void
, also simplifying the call-sites.
src/modules/frost/keygen_impl.h
Outdated
secp256k1_gej rj; | ||
secp256k1_ge rp; | ||
size_t i; | ||
int ret = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in secp256k1_frost_vss_gen
: this value is never changed, so could remove ret
and declare the function as void
instead, also simplifying the call-site below.
include/secp256k1_frost.h
Outdated
*/ | ||
typedef struct { | ||
unsigned char data[36]; | ||
} secp256k1_frost_share; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
naming nit: maybe call it secshare
(both for the type and its instances and (de)ser function names in the API), in order to underline that this should be kept in secret? also since the public counterpart is called pubshare
.
include/secp256k1_frost.h
Outdated
* workflow. See | ||
* https://blockstream.com/2019/02/18/musig-a-new-multisignature-standard/ for | ||
* more details about the risks associated with serializing or deserializing | ||
* this structure. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that this blog post link has been removed in the MuSig2 PR in the secp256k1 main repo (see bitcoin-core/secp256k1#1479 (comment)), I guess the same should be done here.
src/modules/frost/keygen_impl.h
Outdated
if (secp256k1_scalar_eq(&mul, &party_idx)) { | ||
continue; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe more a matter of taste, but the "skip for our own id" logic could be implemented before computing the _i_th indexhash by simply comparing the ids, e.g.
if (secp256k1_memcmp_var(ids33[i], my_id33, 33) == 0) {
return 0;
}
(untested)
The x-coordinate for the participant is a hash output. This x-coordinate must not be zero, however, the probability of a sha256 output being zero is 1 in 2^256. Thus, it is sufficient to use VERIFY_CHECK, which allows the function to be void, which simplifies the call-sites, as suggested by @theStack.
Suggested by @theStack.
Suggested by theStack.
Suggested by @theStack.
As suggested by @theStack.
Co-authored-by: Sebastian Falbesoner <[email protected]>
Co-authored-by: Sebastian Falbesoner <[email protected]>
This PR implements a BIP-340 compatible threshold signature system based on FROST (Flexible Round-Optimized Schnorr Threshold Signatures) with a trusted dealer. This PR does not implement distributed key generation (DKG).
FROST Paper
This PR is based upon the FROST paper by Chelsea Komlo and Ian Goldberg and the draft RFC.
FROST Signing BIP
This PR implements the FROST signing BIP.
Prior Work
This PR is patterned on the APIs, and in many instances duplicates code, from the MuSig2 implementation due to their similarities in nonce generation and signing.