Skip to content

BIP445: FROST Signing Protocol for BIP340 Signatures#2070

Open
siv2r wants to merge 7 commits intobitcoin:masterfrom
siv2r:bip-frost-signing
Open

BIP445: FROST Signing Protocol for BIP340 Signatures#2070
siv2r wants to merge 7 commits intobitcoin:masterfrom
siv2r:bip-frost-signing

Conversation

@siv2r
Copy link
Contributor

@siv2r siv2r commented Jan 3, 2026

This PR adds a BIP for the FROST (Flexible Round-Optimized Schnorr Threshold) signing protocol. The development repository is at https://github.com/siv2r/bip-frost-signing.

There already exists RFC 9591, which standardizes the two-round FROST signing protocol, but it is incompatible with Bitcoin's BIP340 X-only public keys. This BIP bridges that gap by providing a BIP340-compatible variant of FROST.

This BIP standardizes the FROST3 variant (Section 2.3 of the ROAST paper). This variant shares significant similarities with the MuSig2 signing protocol (BIP327). Accordingly, this BIP follows the core design principles of BIP327, and many sections have been directly adapted from it.

FROST key generation is out of scope for this BIP. There are sister BIPs such as ChillDKG and Trusted Dealer Generation that specify key generation mechanisms. This BIP must be used in conjunction with either of those for the full workflow from key generation to signature creation. Careful consideration has been taken to ensure the terminology in this BIP matches that of ChillDKG.

There are multiple (experimental) implementations of this specification:

  • The reference Python implementation included in this PR
  • secp256k1-zkp FROST module (yet to implement the test vectors)
  • FROST-BIP340 TODO: verify if this impl is compatible with our test vectors
  • secp256kfun (implements ChillDKG with FROST signing) TODO: verify if this impl is compatible with our test vectors

Disclosure: AI has been used to rephrase paragraphs for clarity, refactor certain sections of the reference code, and review pull requests made to the development repository.

Feedback is appreciated! Please comment on this pull request or open an issue at https://github.com/siv2r/bip-frost-signing for any feedback. Thank you!

cc @jonasnick @real-or-random @jesseposner

@siv2r
Copy link
Contributor Author

siv2r commented Jan 3, 2026

I'll fix the typos check soon

@siv2r
Copy link
Contributor Author

siv2r commented Jan 3, 2026

I can see that GitHub's file changes view shows only one file at a time due to the large number of changes. This is because the reference implementation includes dependencies and auxiliary materials:

  • The reference code uses secp256k1lab python library (vendored as a git subtree, ~20 files) for scalar and group arithmetic. I can remove this from the PR when the library is integrated into this repository (RFC: Integrate secp256k1lab v1.0.0 as subtree, use it for BIP-374 #1855).
  • Auxiliary files include docs/partialsig_forgery.md (which I can move to a gist if preferred) and a test vector generation script (~1400 lines). I can exclude these if necessary.

@murchandamus murchandamus changed the title Add BIP: FROST Signing for BIP340-compatible Threshold Signatures BIP Draft: FROST Signing Protocol for BIP340 Schnorr Signatures Jan 6, 2026
Copy link
Member

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

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

This is just a first glance, but I noticed a few issues:

@murchandamus murchandamus changed the title BIP Draft: FROST Signing Protocol for BIP340 Schnorr Signatures BIP Draft: FROST Signing Protocol for BIP340 Signatures Jan 8, 2026
Copy link
Member

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

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

Thanks for the quick turn-around. It’s on my todo list to give this a more thorough look, but it might take a bit. If you can motivate some other reviewers meanwhile, that would also be welcome.

@siv2r
Copy link
Contributor Author

siv2r commented Jan 9, 2026

If you can motivate some other reviewers meanwhile, that would also be welcome.

I've shared it with most of the Bitcoin cryptographers I know and will post it on Twitter and the Bitcoin dev groups I'm part of. Hopefully that will bring in more reviewers!

Copy link

@DarkWindman DarkWindman left a comment

Choose a reason for hiding this comment

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

Hi! Quite a remarkable job! We found a few minor issues, and correcting them would improve the overall specification of the BIP.

Copy link
Contributor

@Christewart Christewart left a comment

Choose a reason for hiding this comment

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

As I mentioned on X i'm working on this, so you will likely see more comments in the future. Another nice-to-have would be a table of contents (example) as most other BIPs have this. Perhaps this is a limitation of the .md document vs .mediawiki. Not sure.

- Let *pubshare* = *empty_bytestring*
- If the optional argument *thresh_pk* is not present:
- Let *thresh_pk* = *empty_bytestring*
- If the optional argument *m* is not present:
Copy link
Contributor

Choose a reason for hiding this comment

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

How would you suggest handling this case in C?

We have a message m that equal to the empty bytestring? This is tested in the test vectors accompying this file here.

The python implmentation allows us to represent 2 different states that are (perhaps?) semantically mean the same thing is my understanding. Here are the 2 cases

  1. m is not present (encoded as 00) with the prefix
  2. m is present, but its the empty bytestring (encoded as 0100) with the prefix.

This can be represented in the type system of higher level languages like C++, Python, Rust, Scala etc.

From looking at the API on zkp, it seems like this wouldn't be possible to represent?

Copy link

@vitrixLab vitrixLab Jan 19, 2026

Choose a reason for hiding this comment

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

In C this must be modeled explicitly as an optional bytes type, otherwise we cannot distinguish
“m not present” (00) from “m present but empty(0100).

I recommend representing this as { uint8_t *ptr; size_t len; bool is_present; } and encoding based on is_present.

Collapsing these cases would break the test vectors and change the hash domain.

#sc

Copy link
Contributor Author

@siv2r siv2r Jan 21, 2026

Choose a reason for hiding this comment

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

We distinguish between a zeroed byte array and an empty byte string. Thus, m being absent (empty_bytestring) is different from m being equal to zero bytes (of any length), and we want to generate distinct nonces for these cases.

Yes, the current the zkp API doesn't add the message prefix correctly which needs to be fixed.

I agree with @vitrixLab, we can model this in C with an is_present variable, Musig2 does exactly this.

unsigned char msg_present;
msg_present = msg32 != NULL;
secp256k1_sha256_write(&sha, &msg_present, 1);
if (msg_present) {
    secp256k1_nonce_function_musig_helper(&sha, 8, msg32, 32);
}

@siv2r
Copy link
Contributor Author

siv2r commented Jan 21, 2026

Another nice-to-have would be a table of contents (example) as most other BIPs have this. Perhaps this is a limitation of the .md document vs .mediawiki. Not sure.

Yes, it's a .md issue, this bip initially had a manually written table of contents but was removed after #2070 (comment)

@murchandamus
Copy link
Member

As I mentioned on X i'm working on this, so you will likely see more comments in the future. Another nice-to-have would be a table of contents (example) as most other BIPs have this. Perhaps this is a limitation of the .md document vs .mediawiki. Not sure.

Click there. ;)

image

@Christewart
Copy link
Contributor

As I mentioned on X i'm working on this, so you will likely see more comments in the future. Another nice-to-have would be a table of contents (example) as most other BIPs have this. Perhaps this is a limitation of the .md document vs .mediawiki. Not sure.

Click there. ;)

Thank you! TIL :-)

Copy link

@DarkWindman DarkWindman left a comment

Choose a reason for hiding this comment

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

A few additional minor issues and questions.

- If the optional argument *extra_in* is not present:
- Let *extra_in = empty_bytestring*
- Let *k<sub>i</sub> = scalar_from_bytes_wrapping(hash<sub>FROST/nonce</sub>(rand || bytes(1, len(pubshare)) || pubshare || bytes(1, len(thresh_pk)) || thresh_pk || m_prefixed || bytes(4, len(extra_in)) || extra_in || bytes(1, i - 1)))* for *i = 1,2*
- Fail if *k<sub>1</sub> = Scalar(0)* or *k<sub>2</sub> = Scalar(0)*

Choose a reason for hiding this comment

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

While reading the implementation, I noticed that it includes a check ensuring that k_1 != k_2. At first glance, omitting this check does not appear to introduce any vulnerabilities, and we have verified this. However, I would appreciate hearing your opinion on this point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Which function are you referring to? I don't see a k_1 != k_2 check in the nonce_gen_internal or deterministic_sign functions.

This is an interesting question though. I never considered adding this check, primarily because BIP327's reference implementation doesn't have it either.

Choose a reason for hiding this comment

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

Which function are you referring to? I don't see a k_1 != k_2 check in the nonce_gen_internal or deterministic_sign functions.

I apologize for not mentioning which implementation I was referring to. I meant the secp256k1-zkp FROST module, where the secp256k1_frost_nonce_gen() function performs this check. However, I think this verification is redundant, as I have not found any paper or specification that requires it. At first glance, it may seem that manipulation with Wagner attacks could apply, but I do not see any concrete attack vectors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, I was not aware of this. Thank you! I checked the Olaf paper as well, and it didn't have this requirement. I haven't thought about this from security proof perspective. Will keep this open till then :)

@siv2r
Copy link
Contributor Author

siv2r commented Jan 25, 2026

@DarkWindman thanks a lot for the review! I've addressed most of your review comments in a88f033.

Copy link
Member

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

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

From an editorial standpoint, it looks pretty good and like all the required sections are present. I have read the proposal only partially, and do not have the expertise to fully understand all aspects, so I cannot comment on the technical soundness and whether the Specification is complete and sufficient.

@murchandamus murchandamus added the PR Author action required Needs updates, has unaddressed review comments, or is otherwise waiting for PR author label Jan 27, 2026
],
"msg_index": 0,
"signer_index": 0,
"comment": "The signer's pubshare is not in the list of pubshares"
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this test should be moved into sign_error_test_cases since this would cause an error when deriving the threshold public key from the pubshares?

https://github.com/bitcoin/bips/blob/ec46a20323840b1a6aba83bc2d18b34dd0811245/bip-frost-signing.md#signers-context

Fail if DeriveThreshPubkey(id1..u, pubshare1..u) ≠ thresh_pk

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great observation! The DeriveThreshPubkey function would fail if we gave it a pubshare that's not part of the t-of-n FROST key (i.e., any pubshare other than the n generated during keygen). Here, we're providing a pubshare that was generated during keygen, so DeriveThreshPubkey passes. However, partial_sig_verify fails because we're not providing the correct pubshare whose secshare was used to generate psig.

TL;DR: We're generating psig with secshare, but using a different pubshare (one that exists in the t-of-n FROST key) for verification. That's why the pubshare indices list in this test vector looks like:

            "pubshare_indices": [
                2, # not signer's pubshare
                1
            ],

instead of

            "pubshare_indices": [
                0, # signer's pubshare
                1
            ],

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test vectors covers a lot of cases (some of which may not be useful) if you find something unncessary or if I missed an essential edge case, please let me know I'll add/remove them accordingly.

Copy link
Contributor

@Christewart Christewart Jan 29, 2026

Choose a reason for hiding this comment

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

Doesn't this compute the wrong pubkey because the lagrange coefficient ends up being incorrect - id should be 2 and ends it ends up being 0 because of the id_indices provided in this test case

"id_indices": [
    0,
    1
],

Bigger picture, I don't understand why we don't call ValidateSignersCtx in either PartialSignatureVerify or PartialSignatureVerifyInternal as the BIP is written.

It appears in the python implementation in this PR, we do call validate_signers_ctx. However in the test cases, you call partial_sig_verify_internal directly which circumvents the check. This is probably a signal that there should be some encapsulation of partial_sig_verify_internal - does this actually need to be in a separate method in the BIP? If so - and we allow it to be called directly - we need to redo any context validation in that function.

Is this an oversight or an intentional choice ? If the former, i think we should add validating the signer context inside of partial_sig_verify_internal, if the latter I would be interested in hearing the reasoning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, you're correct. I missed the incorrect id_indices. The DeriveThresPubkey should fail here.

Hmm, PartialSignatureVerify must call ValidateSignersCtx. The Python code does this, but it's not reflected in the BIP text. I'll update it. Thanks!

PartialSignatureVerifyInternal does call ValidateSignersCtx in both the BIP text and Python code. It's not apparent because it calls GetSessionValues, which internally calls ValidateSignersCtx. I think it's useful to explicitly call it in PartialSignatureVerifyInternal (even though it's redundant) to prevent implementers from making mistakes.

I was wondering why the test vector passes in the code even though both partial_sig_verify_internal and partial_sig_verify call validate_signers_ctx. Apparently, there's a bug in the test code. The verify_fail_test_cases reuses an old valid session_ctx (and fails to create a correct new one), so DeriveThresPubkey passes. It would have failed with the new session_ctx. I'll fix this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 87ff5bb. Will resolve this comment when the changes are pulled into this PR

@murchandamus
Copy link
Member

murchandamus commented Jan 30, 2026

Let’s call this BIP 445. Please add an entry for your proposal in the README table, in the preamble update the BIP header to 445 and Assigned header to 2026-01-30, and update your documents file name as well as the auxiliary file directory.

@murchandamus murchandamus changed the title BIP Draft: FROST Signing Protocol for BIP340 Signatures BIP445: FROST Signing Protocol for BIP340 Signatures Jan 30, 2026
Also, add minor fixes in the preamble of the BIP

[^t-edge-cases]: While *t = n* and *t = 1* are in principle supported, simpler alternatives are available in these cases. In the case *t = n*, using a dedicated *n-of-n* multi-signature scheme such as MuSig2 (see [BIP327][bip327]) instead of FROST avoids the need for an interactive DKG. The case *t = 1* can be realized by letting one signer generate an ordinary [BIP340][bip340] key pair and transmitting the key pair to every other signer, who can check its consistency and then simply use the ordinary [BIP340][bip340] signing algorithm. Signers still need to ensure that they agree on a key pair.

The IRTF has published [RFC 9591][rfc9591], which specifies the FROST signing protocol for several elliptic curve and hash function combinations, including secp256k1 with SHA-256, the cryptographic primitives used in Bitcoin. However, the signatures produced by RFC 9591 are incompatible with BIP340 Schnorr signatures due to the X-only public keys introduced in BIP340. Additionally, RFC 9591 does not specify key tweaking mechanisms, which are essential for Bitcoin applications such as [BIP32][bip32] key derivation and [BIP341][bip341] Taproot. This document addresses these limitations by specifying a BIP340-compatible variant of FROST signing protocol that supports key tweaking.
Copy link
Contributor

@Christewart Christewart Feb 2, 2026

Choose a reason for hiding this comment

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

I'm not sure where to best recommend putting this (here, or the tweaking section?) but I think it could be a good idea to mention that this BIP (AFAICT) follows the tweak handling of BIP327.

This would give people a heads up that you may be able to reuse code from your existing MuSig implementation if you have one.

We call it the "Tweak Context" in this BIP. In BIP327, they call it the "KeyAgg Context". At minimum, it seems preferable to use the same nomenclature.

I would guess that this data structure is gonna become more popular as taproot native protocols will have to handle x-only keys - perhaps its worth having a BIP of its own 🤷‍♂️ .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The BIP has the following paragraph in the motivation section:

The FROST3 signing protocol shares substantial similarities with the MuSig2 signing protocol specified in [BIP327][bip327]. Accordingly, this specification adopts several design principles from BIP327, including support for key tweaking, partial signature verification, and identifiable abort mechanisms.

It specifies the key tweaking is adopted from BIP327. I'll now add a note in the algorithms section, about the potential to reuse code from exisiting MuSig2 implementation. This should suffice, right?

We call it the "Tweak Context" in this BIP. In BIP327, they call it the "KeyAgg Context". At minimum, it seems preferable to use the same nomenclature.

Yes, TweakContext is basically the renamed version of KeyAgg Context. It would be incorrect to use the same name here because in BIP327, this struct was the output of the KeyAgg function (a key generation protocol). However, key generation for this BIP is out of scope, and TweakContext is only needed when the user wants to tweak their threshold public key. It's not needed otherwise. Hence, I prefer the TweakContext name.

I would guess that this data structure is gonna become more popular as taproot native protocols will have to handle x-only keys - perhaps its worth having a BIP of its own 🤷‍♂️ .

I agree this structure will become popular. It's already used in BIP 89. As for standardizing it in a BIP, I think it could be done. Whenever this struct appears, it's usually accompanied by Tweak Context, TweakCtxInit, and ApplyTweak. However, I'm not sure every BIP would follow this exact structure, they may modify it depending on their needs. For instance, this BIP renamed KeyAggContext. There's also the possibility of "agnostic tweaking" in FROST, which if adopted would completely eliminate this data structure and would follow a different tweaking approach.

Copy link
Contributor

Choose a reason for hiding this comment

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

The "agnostic tweaking" approach is essentially what is described in Re-Randomized FROST. You can see in the game in Figure 4 that the attacker provides tweak alpha to the signing oracle, which then tweaks the secret share and public key ("randomize" with alpha) and runs the normal FROST signing algorithm with the tweaked keys.

Btw, this approach does not work with MuSig2. In FROST where ell_i and x_i denote the lagrange coefficient and share of signer i and t denote the tweak, tweaking-agnostic signing relies on

sum(ell_i(x_i + t)) = sum(ell_i x_i) + t

which follows from sum(ell_i) = 1. In MuSig2 we'd multiply the tweaked secret key with key aggregation coefficient a_i:

sum(a_i(x_i + t))

which is generally not identical to sum(a_i x_i) + t.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes, this sums up agnostic tweaking accurately. I only knew this as a trick: that shifting every shamir secshare by t would result in the threshold public key being shifted by g^t, when reviewing this ChillDKG function. I wasn't aware of the paper. Just went through it and it describes the algorithm exactly. Thanks!

| *scalar_from_bytes_checked(b)* | *Scalar.from_bytes_checked(b)* | Deserializes a 32-byte array *b* to a scalar, fails if the value is ≥ *ord* |
| *scalar_from_bytes<br>_nonzero_checked(b)* | *Scalar.from_bytes<br>_nonzero_checked(b)* | Deserializes a 32-byte array *b* to a scalar, fails if the value is zero or ≥ *ord* |
| *scalar_from_bytes_wrapping(b)* | *Scalar.from_bytes_wrapping(b)* | Deserializes a 32-byte array *b* to a scalar, reducing the value modulo *ord* |
| *hash<sub>tag</sub>(x)* | *tagged_hash(x)* | Computes a 32-byte domain-separated hash of the byte array *x*. The output is *SHA256(SHA256(tag) \|\| SHA256(tag) \|\| x)*, where *tag* is UTF-8 encoded string unique to the context |
Copy link
Contributor

Choose a reason for hiding this comment

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

secp256k1lab table nit:

Suggested change
| *hash<sub>tag</sub>(x)* | *tagged_hash(x)* | Computes a 32-byte domain-separated hash of the byte array *x*. The output is *SHA256(SHA256(tag) \|\| SHA256(tag) \|\| x)*, where *tag* is UTF-8 encoded string unique to the context |
| *hash<sub>tag</sub>(x)* | *tagged_hash(tag, x)* | Computes a 32-byte domain-separated hash of the byte array *x*. The output is *SHA256(SHA256(tag) \|\| SHA256(tag) \|\| x)*, where *tag* is UTF-8 encoded string unique to the context |

@Christewart
Copy link
Contributor

tACK test vectors on 289286c

@murchandamus
Copy link
Member

murchandamus commented Feb 3, 2026

As I’m not involved in the technical review here, I’m gonna tune out here for the time being.
Please feel free to mention me by nickname when you reach a point where you’d like an editor to take a look again, or when you think you are reaching a point where your document should be published.

@jonasnick
Copy link
Contributor

There are a few REVIEW comments in the python implementation that would need to be addressed because they appear to be relevant to security.

Copy link

@disnocen disnocen left a comment

Choose a reason for hiding this comment

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

Nonce generation review

b = Scalar.from_bytes_wrapping(
tagged_hash("FROST/noncecoef", ser_ids + aggnonce + Q.to_bytes_xonly() + msg)
)
assert b != 0

Choose a reason for hiding this comment

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

I reviewed the nonce generation functions (NonceGen, CounterNonceGen,DeterministicSign) and the nonce coefficient computation.

  1. Nonce reuse is prevented by secnonce zeroing and defense-in-depth hash inputs
  2. RNG failures are mitigated by secret share blinding and alternative generation modes
  3. Nonce bias is negligible due to proper modular reduction of hash outputs
  4. Domain separation is thorough via tagged hashing
  5. Multi-user concerns are addressed by including signer-specific inputs in the nonce hash
  6. Nonce aggregation integrity is ensured by binding the nonce coefficient b to all session parameters (signer set, aggregate nonce, public key, message), preventing algebraic attacks across signing sessions

- The list of tweaks *tweak<sub>1..v</sub>*: *v* 32-byte arrays, each a serialized scalar
- The list of tweak modes *is_xonly_t<sub>1..v</sub>* : *v* booleans
- The message *m*: a byte array[^max-msg-len]
- The index *i* of the signer in the list of public nonces where *0 < i ≤ u*
Copy link
Contributor

Choose a reason for hiding this comment

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

Why pass the index here and not the signer_id itself? It seems like the index is only used to find the signer_id

Also since we start coutning at 0 here, I believe this is off by one (i.e u+1 elements)?

0 < i ≤ u

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to extract the tuple (id[i], pubnonce[i], pubshare[i]) for the given psig, and we need the index i for this.

0 < i ≤ u

Yes, this is a mistake. Will fix it. Thanks!


# think: can msg & extra_in be of any length here?
# think: why doesn't musig2 ref code check for `pk` length here?
# REVIEW: Why should thresh_pk be XOnlyPk here? Shouldn't it be PlainPk?

Choose a reason for hiding this comment

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

I think that using XOnlyPk (32 bytes) is acceptable. It is consistent with BIP 327 (MuSig2), whose NonceGen algorithm passes the aggregate public key as XOnlyPk via GetXonlyPubkey() (see BIP 327, line aggpk: "a 32-byte array").

The purpose of thresh_pk in nonce_gen is defense-in-depth entropy diversification: if the RNG fails and produces the same rand' twice, having different thresh_pk values would still produce different nonces. Security of nonce generation fundamentally relies on the 32 bytes of randomness from secrets.token_bytes(32), not on thresh_pk.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. Sounds good. Thanks!

Comment on lines +336 to +339
# REVIEW: do we actually need this check? Musig2 embeds pk in secnonce to prevent
# the wagner's attack related to tweaked pubkeys, but here we don't have that issue.
# If we don't need to worry about that attack, we remove pubshare from get_session_values
# return values

Choose a reason for hiding this comment

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

While the Wagner attack that motivates the check in BIP 327 does NOT directly apply to FROST in the same way, I think it should be kept for defense in depth

raise ValueError(
"The signer's pubshare must be included in the list of pubshares."
)
# REVIEW: do we actually need this check?
Copy link

@disnocen disnocen Feb 19, 2026

Choose a reason for hiding this comment

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

Note: The comment is for line 344, and the subsequent check, not the previous one. Sorry, from the wrong highlight. I can not change that.

I do think we need this check. Without this check, a malicious coordinator could achieve signature disruption and make signing session fail by manipulating the Lagrange interpolation process. This in turn breaks the identifiable aborts property of ROAST/FROST3, since now the signer is apparently responsible for the failure, while in reality the problem lies in the wrong data fed by the coordinator

Note that the assert in derive_interpolating_value is not sufficient, since Python assert statements are removed entirely when Python is run with the -O (optimize) flag or PYTHONOPTIMIZE=1is set

Copy link
Contributor

Choose a reason for hiding this comment

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

Should you also check that the ids are unique for identifiable aborts?

Choose a reason for hiding this comment

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

I see that get_session_values (line 321) calls validate_signers_ctx, which already checks uniqueness of ids at line 103-104. It raises a generic ValueError though: I don't know if this kind of error is sufficient for identifiable aborts, or if aInvalidContributionError would be better

Copy link
Contributor Author

@siv2r siv2r Feb 27, 2026

Choose a reason for hiding this comment

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

I agree. I'd like to keep this check as well.

I don't know if this kind of error is sufficient for identifiable aborts, or if aInvalidContributionError would be better

SignersContext is external, pre-protocol setup data, agreed upon by all parties before signing begins. It is not a runtime protocol contribution from any single signer or coordinator. InvalidContributionError is semantically for in-protocol misbehavior, so I don't think it's appropriate here. Maybe we could define a new error for invalid protocol input data like ProtocolInputError or ProtocolSetupError.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should you also check that the ids are unique for identifiable aborts?

Duplicate ids in SignersContext means the protocol setup is broken and should be rejected upfront. Now that you mention it, the BIP says validate_signers_context is an "optional" API that implementations can run to check the compatibility of keygen and signing. We should mandate it since we obviously want to avoid duplicate IDs during signing.

Side note: For identifiable abort to work, it requires the coordinator to be trusted (peform honest aggregation): siv2r/bip-frost-signing#42

return psig


# REVIEW should we hash the signer set (or pubshares) too? Otherwise same nonce will be generate even if the signer set changes
Copy link

@disnocen disnocen Feb 19, 2026

Choose a reason for hiding this comment

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

Note: The comment is for line 364, and the subsequent function. Sorry, from the wrong highlight. I can not change that.

I think we should include serialize_ids(ids) in the det_nonce_hash input.

The current implementation hashes secshare, aggothernonce, tweaked_tpk, msg, and the index i, but not the signer set (ids). Since deterministic_sign is used by the last participant (the one who waits for everyone else's nonces), a malicious coordinator can replay the same aggothernonce across multiple sessions with different signer subsets while keeping everything else constant. This produces identical nonces for the victim across sessions, but the Lagrange interpolation coefficients differ because the signer set changed. Three such sessions give three linear equations in three unknowns (k_1, k_2, d'), which is enough to extract the victim's secret share.

MuSig2's DeterministicSign doesn't need this because it's always n-of-n (the signer set never varies), but in FROST the variable signer subsets make it necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about adding my_id as well? I don't think it's required (if key generation was done correctly) but it doesn't hurt.

s = Scalar(0)
for my_id, psig in zip(ids, psigs):
try:
s_i = Scalar.from_bytes_checked(psig)
Copy link
Contributor

Choose a reason for hiding this comment

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

To be consistent with the pseudocode and partial_sig_verify, this should be Scalar.from_bytes_checked_nonzero. However, is there a reason to enforce nonzero in the first place? BIP MuSig allows 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, I misunderstood the s >= n check in MuSig code:

def partial_sig_verify_internal(psig: bytes, pubnonce: bytes, pk: bytes, session_ctx: SessionContext) -> bool:
    (Q, gacc, _, b, R, e) = get_session_values(session_ctx)
    s = int_from_bytes(psig)
    if s >= n:
        return False

Thinking $\text{s} = n \cong 0 \mod n$ should fail. I'll change the bip spect and code to allow 0. Thanks!

return secnonce, pubnonce


# think: can msg & extra_in be of any length here?
Copy link
Contributor

Choose a reason for hiding this comment

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

yes



# think: can msg & extra_in be of any length here?
# think: why doesn't musig2 ref code check for `pk` length here?
Copy link
Contributor

Choose a reason for hiding this comment

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

No deep reason. It was simply missed in BIP MuSig2.

return psig


# REVIEW should we hash the signer set (or pubshares) too? Otherwise same nonce will be generate even if the signer set changes
Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about adding my_id as well? I don't think it's required (if key generation was done correctly) but it doesn't hurt.

siv2r added a commit to siv2r/bip-frost-signing that referenced this pull request Mar 2, 2026
It misclassifed a "verify error" vector as "verify fail" vector.
context: bitcoin/bips#2070 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

New BIP PR Author action required Needs updates, has unaddressed review comments, or is otherwise waiting for PR author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants