Add frost-secp256k1-tr crate (BIP340/BIP341) [moved]

[conduition]

This PR is a re-opening of #584 to resolve PR ownership problems. I'm opening this so zebra-lucky (who is short on time) doesn't need to be the middle-man merging my commits into their PR anymore.

---

This PR adds a new ciphersuite crate frost-secp256k1-tr, which complies with BIP340 Schnorr Signatures on Bitcoin and is compatible with BIP341 tweaks. This crate can be used to produce valid signatures on Bitcoin transactions.

We handle the complex x-only negation conditions of BIP340 by introducing new methods on the Ciphersuite trait, which are generally identity functions in the case of other ciphersuites, but which in the taproot suite will conditionally negate certain values at critical times to maintain compatibility with BIP340's x-only public keys. These methods are usually denoted C::effective_xyz(...).

To support BIP341 taptree tweaking, we add two new types, SigningTarget and SigningParameters, and modify the signature package to accept any type that transforms Into SigningTarget. For backwards compatibility, this includes any type which implements AsRef<[u8]>. These convert into SigningTarget paired with SigningParameters::default(). The SigningParameters can be modified to commit signatures to a specific tapscript merkle root tweak, allowing FROST groups to modify (tweak) their group keys with taproot script-spending paths, while retaining the use of the key-spending path.

Some additional features we can consider adding:

• Adaptor signature support
• BIP32 key derivation
• Higher-level (nested) multisig support

i have a separate feature branch, taproot-bip32, based off of this branch which adds BIP32 support to the frost-secp256k1-tr crate. See this discussion for more background. It would be nice to be able to PR that branch into this repo as well, so that FROST keys can be used in hierarchical and descriptor wallets. For now I am leaving that separate to reduce conflation of features, but they can be merged together if desired.

[mergify]

⚠️ The sha of the head commit of this PR conflicts with #584. Mergify cannot evaluate rules on this PR. ⚠️

[conduition]

This PR clearly needs to be resynced with main, I will have a go at that soon after conferring with my clients

[conduition]

As noted by @jesseposner during the round table, the raw DKG output is not safe to use in a BIP341 context. I'm going to update this PR to reflect that, which might entail some API changes.

[conradoplg]

As noted by @jesseposner during the round table, the raw DKG output is not safe to use in a BIP341 context. I'm going to update this PR to reflect that, which might entail some API changes.

I had the impression that this PR would adjust the sign of the X coordinate during signing? That would solve the issue.

(I ask this because I remember the first thing I wanted to do in this PR was to make that during key generation only, but that's not possible by @jesseposner's point)

[jesseposner]

There's a negation algorithm in the FROST signing BIP: https://github.com/siv2r/bip-frost-signing

It gets a little tricky to account for both BIP32 tweaks and taproot script tweaks (TapTweaks), which is why the algorithm is a bit complex. The algorithm is implemented here: BlockstreamResearch/secp256k1-zkp#278

It's also the same algorithm used in the MuSig2 BIP and implementation.

[conduition]

@conradoplg Yes, this PR does handle key negation properly at signing time (i haven't read Jesse's linked negation algorithm yet), but how would key negation at signing time prevent a cosigner from injecting a malicious tweak into the group key at DKG time?

Maybe it'd help to actually write out the steps the attacker will do, so we're sure we're all talking about the same thing.

Let's say we run a DKG to construct a simple 2-of-2 FROST group key. Honest participant $1$ samples their random coefficients $\{a_{10}, a_{11} \}$ and broadcasts their commitment $C_1 = \{ \phi_{10}, \phi_{11} \} = \{ a_{10} G, a_{11} G \}$.

A malicious participant $2$ also samples random coefficients $\{ a_{20}', a_{21} \}$, but now they can inject a malicious tweak as follows.

1. Compute the 'internal' group pubkey $Y'$.

$$ Y' = \phi_{10} + a_{20}' \cdot G $$

2. Given some malicious tapscript tree merkle root $m$, compute the tweaked group public key $Y$:

$$ t = H(Y'\ ||\ m) $$

$$ Y = Y' + t G $$

3. Tweak the constant term coefficient by adding $t$.

$$ a_{20} = a_{20}' + t $$

4. Compute the DKG commitment using the maliciously tweaked coefficient.

$$ C_2 = \{\phi_{20}, \phi_{21}\} = \{a_{20} G, a_{21} G\} $$

5. Continue the FROST DKG protocol as normal.

The resulting group pubkey accepted by the group will be:

$$ \begin{align} Y &= \phi_{10} + \phi_{20} \\\ &= \phi_{10} + a_{20} \cdot G \\\ &= \phi_{10} + (a_{20}' + t) \cdot G \\\ &= \phi_{10} + a_{20}' \cdot G + t G \\\ &= Y' + H(Y'\ ||\ m) \cdot G \\\ \end{align} $$

The DKG will succeed because the malicious participant does actually know the coefficients of their secret contribution polynomial and can produce a valid proof of possession. The only drawback of this attack is that it depends on the attacker waiting to hear everyone else's DKG commitments first, in order to compute the rogue tweak. And naturally it doesn't work if the group intends to use BIP32 or BIP341 tweaks after DKG, but if the group uses the raw DKG output pubkey $Y$ in a P2TR script on chain, then the attacker can use the script-spending path described by $m$ to steal the coins.

[MatthewLM]

@conduition Perhaps a simple solution is to tweak the DKG output in a way similar to BIP32? It could be as simple as tweaking by the group public key hash? Then the private shares, public shares and group key provided by the library can include this tweak with no API changes needed.

[conduition]

@MatthewLM Great idea. It's simple and elegantly prevents the footgun, without limiting how participants use their group key. The group can still add further tweaks (e.g. BIP341, BIP32, etc) at signing time too. I spent last night trying to break it, but couldn't see anything wrong.

@jesseposner @conradoplg What do you guys think?

[conduition]

I'm starting the process of resyncing this PR with the upstream v2.0.0 changes on main.

[jesseposner]

This sounds like what @real-or-random suggested here: BlockstreamResearch/bip-frost-dkg#41 (comment)

Ideally, we would not only add a warning, but just fix the problem by always adding a tweak to the threshold pubkey output by the DKG.

[jesseposner]

BIP341 suggests this tweak specifically: Q = P + int(hashTapTweak(bytes(P)))G

[MatthewLM]

Isn't the key already tweaked at sign-time anyway? Looking at my own code, I've used an earlier version of this branch and I see that a TapTweak is already done. I was previously able to create successful Taproot signatures with key-spend tweaks. I'm not sure any changes are needed.

I guess the fear is that the public key might be used outside the library without BIP341 compliance? In that case, pre-tweaking the DKG output might increase safety.

[conduition]

Resyncing is done but not yet pushed to this branch. You can preview the changes here: main...conduition:frost:add-secp256k1-tr-reloaded

@conradoplg Do you have any preference for how i merge the updates into this branch? If it's not important, i'll just force-push the new commit history over this PR. I tagged @mimoo and zebra-lucky as co-contributors in the commit messages but i'm not sure how else to properly attribute.

Isn't the key already tweaked at sign-time anyway?

@MatthewLM Not always. The current code will, by default, sign without any tweak. The untweaked group pubkey which those signatures would verify under is susceptible to rogue tweak attacks at DKG-time.

I think a static unspendable tweak of the group key at DKG time is the ideal scenario here, as this ensures that no sane caller will ever accidentally pay money to a group pubkey which could contain a rogue tweak. I'll see about adding this soon after merging the re-synced branch into this one. I'll try to follow the suggestion of BIP341 (using hashTapTweak as @jesseposner pointed out)

[conduition]

Just to formalize this 'tweak the DKG output' idea a little...

Every FROST participant receives three things from the DKG:

• A signing share $s_i = \sum_{j=1}^n f_j(i)$
• A set of $n$ verification shares $\{Y_1 ... Y_n\}$. Each share is computed using the commitment coefficients: $Y_i = \sum_{j=1}^n \sum_{k=0}^{t-1} i^{k} \cdot \phi_{jk}$
• A group verification key $Y = \sum_{i=1}^n \phi_{i0}$

To prevent malicious tweaks (inserted into some $\phi_{i0}$), we compute a tweak $t = H_{\text{TapTweak}}(Y)$. We then additively tweak all three DKG outputs with $t$.

• $Y_i' = Y_i + tG$
• $Y' = Y + tG$
• $s_i' = s_i + t$

The participants can do this implicitly after DKG, without any extra communication rounds. The shares are still valid and the resulting group key $Y'$ is certain not to have any tapscript spending path.

[conradoplg]

@conradoplg Do you have any preference for how i merge the updates into this branch? If it's not important, i'll just force-push the new commit history over this PR. I tagged @mimoo and zebra-lucky as co-contributors in the commit messages but i'm not sure how else to properly attribute.

It doesn't matter, PRs are squashed in this repo anyway. GitHub will include the contributors in the commit message when it merges.

Let me know if you need help solving conflicts.

[conduition]

I've resynchronized this branch with upstream main to be compatible with the v2.0.0 codebase. Tests are passing and clippy is happy. I'll add DKG tweaking soon in a new commit

[0xBEEFCAF3]

tACK 802de7a
FWIW I ran some functionality tests against a regtest bitcoind setup
My test suite can be found here
Tested:
✅ Key Spend with Trusted Dealer Setup
✅ Script Spend with Trusted Dealer Setup
✅ Key Spend with DKG Setup
✅ Script Spend with DKG Setup

[codecov]

Codecov Report

Attention: Patch coverage is 89.30900% with 82 lines in your changes missing coverage. Please review.

Project coverage is 83.44%. Comparing base (5f4ac6e) to head (59d1da2).
Report is 62 commits behind head on main.

Files with missing lines	Patch %	Lines
frost-secp256k1-tr/src/lib.rs	91.70%	40 Missing ⚠️
frost-secp256k1-tr/src/keys/dkg.rs	0.00%	21 Missing ⚠️
frost-secp256k1-tr/src/keys/repairable.rs	57.14%	18 Missing ⚠️
frost-core/src/lib.rs	92.85%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #730      +/-   ##
==========================================
+ Coverage   82.18%   83.44%   +1.26%     
==========================================
  Files          31       37       +6     
  Lines        3188     4071     +883     
==========================================
+ Hits         2620     3397     +777     
- Misses        568      674     +106     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[conradoplg]

Just FYI I'm actively reviewing this. It looks good, but I think it might be possible to simplify some of the code so I'm working on that.

[conradoplg]

I pushed a cleanup of the code.

My main motivation was to minimize changes to frost_core and also try not to include BIP-340-specific code in it (e.g. y_is_odd(), negation methods etc.). This is because frost_core was audited and I want to minimize the risk of introducing bugs in it.

I change the approach of the additional Ciphersuite methods to be more "high-level".

One aspect which I'm not sure about is tweak handling. In my commit I reverted the SigningTarget/Parameters types because I did not want to change the encoding of the SigningPackage. I feel like additional information that needs to be communicated should be handled by the caller.

This means that, with my changes, tweak handling is not longer "transparent". You will need to call e.g. frost_secp256_k1_tr::round2::sign_with_tweak(signing_package, signing_nonces, key_package, merkle_root) and you won't be able to use just frost_core (e.g. frost_core::round2::sign<frost_secp256_k1_tr::Secp256K1Sha256TR>::sign(...) if you want to use tweaks. Is that acceptable for users of this ciphersuite?

There is also some stuff missing:

• DKG test vectors are not passing. I changed the proof-of-knowledge handling to not use "even Y" because I don't think there is a need to. But this means that the test vectors need to be updated. I can eventually manage to fix those, but if whoever created them could update them, I would appreciate that a lot.
• I need to fix conflicts with main
• Some documentation improvements (probably its README should have information about tweak handling, so I'm waiting on a decision on that API)
• I thinking about using a feature that needs to enabled in frost_core to activate these additional trait methods, so that we don't have to do a major version bump for this.
• Is there an easy way to test if tweak handling is correct? i.e. if a tx signed with it would be really accepted. (Or @0xBEEFCAF3 would you mind rerunning your tests? Do they cover the usage with tweaks?)

I apologize for changing everything so much, but I feel like this approach has some advantages and makes everything a bit easier to reason about. I really appreciate all the effort that went into this.

You can look at the individual commit to see the changes from the original approach, and the overall PR diff to see differences with main (which should be a bit smaller now, in particular regarding frost_core).

Looking forward to your feedback.

[conduition]

Thanks @conradoplg, I'll try to take a look at these changes soon.

[conradoplg]

Thinking about it I don't think we can get away with not using even-Y for DKG proof-of-knowledge since the signature serialization only encodes X and the PoK is represented as a Signature. The tests are not catching this likely because they don't do serialization roundtrips (that's #688). I'll look into it.

[conradoplg]

Thinking about it I don't think we can get away with not using even-Y for DKG proof-of-knowledge since the signature serialization only encodes X and the PoK is represented as a Signature. The tests are not catching this likely because they don't do serialization roundtrips (that's #688). I'll look into it.

Ah nevermind. I am using even-Y for the R component of the signature. It's just the DKG round1::Package public key which is not using even-Y so adjusting the test vectors might be enough.

[conduition]

@conradoplg Tests are passing for me on this branch 🎉

Now that we've got that sorted out, i'm going to do the pruning i suggested here to hopefully reduce code surface area. Fingers crossed it works. Might take a few days but i'll check in again soon 🙂

[0xBEEFCAF3]

Is there an easy way to test if tweak handling is correct? i.e. if a tx signed with it would be really accepted. (Or @0xBEEFCAF3 would you mind rerunning your tests? Do they cover the usage with tweaks?)

@conradoplg yes happy to run the testsuite again. I added a test to cover the taptweak case. In this test, we spend using the FROST key through the default key spend path, while also including optional tapleaf script paths.

All tests are passing against the latest commit (f9237f5)

[conradoplg]

Now that we've got that sorted out, i'm going to do the pruning i suggested here to hopefully reduce code surface area. Fingers crossed it works. Might take a few days but i'll check in again soon 🙂

Thanks, it looks great. I'll sort out the conflicts next.

[conradoplg]

Everything looks great, thank you all for the contributions. I'll merge this soon unless someone wants to add anything?

[conduition]

Thank you very much @conradoplg ❤️

[real-or-random]

Hi, sorry for being late to the party. I'm just curious: Is there any specific reason why someone in the Zcash ecosystem would need this (e.g., swaps with Bitcoin)? Or are you "just" making this available to the world because you feel it's useful that there is a Rust implementation for experimentation and given that Bitcoin is widespread? In either case, it's nice to see a second independent implementation! :)

[conduition]

@real-or-random Hi Tim!

I chose this repo because the ZF FROST implementation is well-audited, reputable and well-documented already. Better to build on solid foundations battle-tested by others, than attempt to reimplement it from scratch myself. Instead I only had to implement a taproot compatibility layer here, which is much lower surface area than FROST itself.

My hope is that with further auditing/review/improvement, this implementation could become the go-to FROST implementation in pure Rust that downstream Bitcoin multisig wallets can pull in.

Someday, FROST will land in libsecp256k1-zkp and will then be pulled into the secp256k1_zkp rust bindings, and that will be ideal for many users. But that will always be FFI, and some applications may need or prefer a pure Rust solution compatible with k256, which this PR is.

As for zcash uses, I'm not a zcash user myself, so I can't speak to utility in that domain. However, this is certainly useful for Nostr, Cashu, and any other spec where secp256k1 signatures must comply with BIP340. I'm curious what people will build there...

[conradoplg]

Hi, sorry for being late to the party. I'm just curious: Is there any specific reason why someone in the Zcash ecosystem would need this (e.g., swaps with Bitcoin)? Or are you "just" making this available to the world because you feel it's useful that there is a Rust implementation for experimentation and given that Bitcoin is widespread? In either case, it's nice to see a second independent implementation! :)

Our original rationale is that BIP-340 is probably the biggest application of FROST and it made sense to support that; this repo was never meant to be Zcash-specific (the Zcash ciphersuites are not even here, but in https://github.com/ZcashFoundation/reddsa/ , though I'm thinking of moving them here too...).

But now it does have an application for Zcash; the upcoming Zcash Shielded Assets will use BIP-340 signatures for issuance, and that's also a great use case for FROST.