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 87.55074% with 92 lines in your changes missing coverage. Please review.

Project coverage is 82.73%. Comparing base (5f4ac6e) to head (802de7a).
Report is 53 commits behind head on main.

Files with missing lines	Patch %	Lines
frost-secp256k1-tr/src/lib.rs	87.87%	48 Missing ⚠️
frost-secp256k1-tr/src/keys/dkg.rs	0.00%	21 Missing ⚠️
frost-secp256k1-tr/src/keys/repairable.rs	55.00%	18 Missing ⚠️
frost-core/src/traits.rs	96.40%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #730      +/-   ##
==========================================
+ Coverage   82.18%   82.73%   +0.55%     
==========================================
  Files          31       37       +6     
  Lines        3188     3950     +762     
==========================================
+ Hits         2620     3268     +648     
- Misses        568      682     +114     

☔ 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]

Haven't finished reviewing yet but I'm leaving this note here to remind me to ensure we have tests in the taproot crate covering tweaked signing, since check_sign now only does untweaked signing.

[conradoplg]

I added a test in tweaking_tests.rs, where I basically copied the README test and added tweaking. I don't like the repetition but I think it will do it for now.

[conduition]

Notes:

• The group commitment is negated in the ciphersuite verify_share method
• The verifying share is negated in the aggregate function, via the ciphersuite pre_aggregate method

[conduition]

Apologies for the wall of text here, i wanted to be detailed so that future-me will understand what present-me is planning 😉

We might be able to prune this method, and the whole Ciphersuite::Context related-type idea that you're using to cache is_group_commitment_even.

<C>::compute_group_commitment is only used in two locations:

1. frost-core/src/round2.rs in the sign function.

2. frost-core/src/lib.rs in the aggregate function.

For (1), the resulting group commitment is passed to the <C>::challenge(...) invocation, but in BIP340, the challenge only commits to the x-coordinate of the group's public nonce (AKA group_commitment.to_element().to_affine().x()). So it doesn't matter if the group commitment point is negated here or not. The only place in which the commitment parity does matter is when calling <C>::compute_signature_share, where we conditionally negate our signer's secret nonces.

For (2), the resulting group commitment is used to construct the aggregated Signature. We verify the signature using <C>::verify_signature, and if that fails we start hunting for cheaters. The only two spots in here where the parity of Signature.R matters are:

• the call to <C>::verify_signature to check the aggregate signature.
• the call to <C>::verify_share to detect cheaters.

What if we change the Secp256K1Sha256TR::pre_verify implementation to convert signature.R into an even-parity point before verification? This would mean verifiers would accept a Signature with an even OR an odd parity nonce, but that's OK because the parity bit will be discarded eventually anyway (BIP340 sigs dont have nonce parity). This means we don't need to negate the group commitment during aggregation.

Then we can get rid of the compute_group_commitment method on ciphersuite, remove the Context concept, and just pass in the un-negated group commitment into the ciphersuite methods which need to know $R$ parity (i.e. <C>::compute_signature_share and <C>::verify_share).

If you like this idea i'd be happy to implement it, but before we do any more changes I'd like to get the test vectors updated and have tests passing first. I'll work on that over the next few days

[conradoplg]

This sounds great! I'd like to avoid having to pass the group commitment around if we could, but this feels much better than having to add Context. Please go ahead if you want, thanks.

[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)