btcec/schnorr/musig2: add new musig2 implementation based on musig2 draft BIP #1820
Conversation
Pull Request Test Coverage Report for Build 2242455488
💛 - Coveralls |
|
cc @Crypt-iQ |
Roasbeef
force-pushed
the
musig2
branch
2 times, most recently
from
ab04f5c
to
2108cfa
Compare
|
I've scrapped the prior approach and pushed up a new final commit that introduces an easier way to manage multi-party signing, along with a best-effort attempt at detecting nonce reuse. With that, the final checkbox on this PR is complete! |
|
Addressed latest comments, and fixed a possible issue w.r.t the way sorting was handled. Right now we check for a few places before sorting which makes things more efficient (n comparison ops vs nlogn). If the code is made a bit more clever, we can pass through context with the set of keys themselves (making a new internal wrapper struct) if things are sorted or not... |
|
Pushed up a commit to address the latest comments. The final question in my mind here is: should we make the existing |
| // PublicNonce returns the public nonce for a signer. This should be sent to | ||
| // other parties before signing begins, so they can compute the aggregated | ||
| // public nonce. | ||
| func (s *Session) PublicNonce() [PubNonceSize]byte { |
There was a problem hiding this comment.
Would be nice if we could also access the session's Context through a method, then we'd only need to store/reference one of them.
There was a problem hiding this comment.
Hmm the idea was that you'd only store the context (since it's just the set of keys), and never store (assuming you mean write to disk?) the session. Are there any relevant fields you think should be exported in the context itself?
There was a problem hiding this comment.
I mean for keeping things in memory on the signing RPC side. We need to keep the session object in memory to be able to add nonces and partial signatures. But maybe we don't need the context after creating it and getting the combined key from it.
btcec/schnorr/musig2/context.go
Outdated
| // | ||
| // NOTE: This struct should be used over the raw Sign API whenever possible. | ||
| func NewContext(signingKey *btcec.PrivateKey, | ||
| signers []*btcec.PublicKey, shouldSort bool) (*Context, error) { |
There was a problem hiding this comment.
Can we document that the public keys need to be parsed with schnorr.ParsePubKey()? Otherwise the below schnorr.ParsePubKey(schnorr.SerializePubKey(...)) fails for odd keys.
I assume we need to use x-only public keys everywhere throughout this API?
There was a problem hiding this comment.
What do you mean it fails? If the key actually has an even y coord, then that'll effectively just lose that bit and present it as a key with an odd y coordinate.
There was a problem hiding this comment.
Or you mean if you don't do this to the signer key amongst the passed set of signers then things fail? Or that we should just "re encode" everything here?
There was a problem hiding this comment.
Sorry, the comment was a bit imprecise. I got the ErrSignerNotInKeySet error when I didn't re-encode all the signer public keys. I was able to fix it by just doing the same parse(serialize()) to all of them, including the local key. But just from the API/godoc it wasn't clear to me whether I should do that or not (since that will have an effect on the resulting signature for sure?).
btcec/schnorr/musig2/context.go
Outdated
| // of public keys for each of the other signers. | ||
| // | ||
| // NOTE: This struct should be used over the raw Sign API whenever possible. | ||
| func NewContext(signingKey *btcec.PrivateKey, |
There was a problem hiding this comment.
Could we abstract the signing key behind an interface so we don't need to specify it here already? I'm trying to think of a way to support remote signing in a way where the watch-only node could keep track of the signing sessions and would only redirect the raw Sign() call to the signing node. That way the signing node could remain fully stateless. Otherwise we have to proxy all the MuSig2 RPC calls directly to the signing node and it needs to keep track of the session state.
Maybe a new SignSchnorr RPC on the signing service would help here?
There was a problem hiding this comment.
Could we abstract the signing key behind an interface so we don't need to specify it here already? I'm trying to think of a way to support remote signing in a way where the watch-only node could keep track of the signing sessions and would only redirect the raw Sign() call to the signing node
Hmm, I think that's possible. But in the remote signing setting, isn't it the case that the caller already doesn't have access to the private key, and all the session state would actually live entirely in the remote signer?
I can see how it can be nice to have the signing node be stateless, but given that it's the one that needs to manage the nonce state (which is where the danger is), I'm not sure how we can avoid it having that extra in memory state (assuming all protocols are designed to drop the nonce state if things need to resume of connections be re-established or w/e).
Maybe a new SignSchnorr RPC on the signing service would help here?
Yeah this was my original idea, but then I saw the way you sort of made the decision implicitly when signing outputs to figure out if ECDSA vs schnorr should be used. One thing to keep in mind here though, is that the schnorr partial signature is not the same as a normal BIP-340 signature. So we'd need a SignPartialSchnorr that would only be used in the musig2 case, with something else being used for generic bip 340 signatures outside of the Bitcoin sighash scenario.
There was a problem hiding this comment.
Ah, you're right of course. Moving the nonce state from the remote signer to the watch-only node would indeed mean we'd need the SignPartialSchnorr RPC to accept the nonces in its request, which would make it insecure again.
Okay, I guess for now it's okay to just forward the full MuSig2 API to the remote signer and keep the state there. It just means that the remote signer can no longer easily be load balanced between multiple nodes. But maybe that isn't anything that we (or someone else) currently need in the first place.
| // nonce re-use is attempted. | ||
| // | ||
| // NOTE: This struct should be used over the raw Sign API whenever possible. | ||
| type Session struct { |
There was a problem hiding this comment.
I really like this API! It should make this quite easy to use and also safe.
One use case I was thinking about that wouldn't be possible with the current API is the channel funding 2-of-2 MuSig2 case. Because if we want to send our nonces over in the open_channel message, we wouldn't know the other party's public key yet, so couldn't create a full context yet. But we'd need a session to get access to "safe" nonces.
Maybe we can combine the two structs and keep some kind of progression state for the different steps?
There was a problem hiding this comment.
One use case I was thinking about that wouldn't be possible with the current API is the channel funding 2-of-2 MuSig2 case. Because if we want to send our nonces over in the open_channel message, we wouldn't know the other party's public key yet,
Great point! I was coming at this from the PoV of initial commitment signing, where we already had created the joint public key at that point...
One way to handle this would be to add a functional argument here that can be passed in which just uses the nonces specified by the caller. This would also be useful for deterministic tests. This way, you either initialize with the set of public keys (then the nonce is produced), or you do it with just your key (in order to gain access to the nonce) with you specifying the keys of the remote party in a future step.
Would need to think about this a bit though, as it then exposes the caller (somewhat) to nonce generation themselves, but I think we can wrap that up at a high level.
There was a problem hiding this comment.
Or we permit passing in only a single key there, generate the nonces as normal, but return errors from certain calls until the other key is registered? So we'd allow a dynamic key set, with a similar Register method for the additional set of keys.
There was a problem hiding this comment.
Also re context vs session, my initial idea was that both sides would maintain the some context, and then make new sessions on the fly each time they need to piggy back nonces into messages. By next week or so, I should have a more concrete sketch of the flow here both during funding as well as normal commitment updates.
|
Aight pushed another commit that supports taproot-native tweaking. A few new options have been added to make things easier in the context, and the may context also provides access to the aggregated key before tweaking, since that is needed to be used as the internal key. |
|
Added another commit with explicit BIP 86 support (so signing with an output key that provably doesn't commit to a script path). |
| combinedKey := btcec.NewPublicKey(&finalKeyJ.X, &finalKeyJ.Y) | ||
|
|
||
| // At this point, if this is a taproot tweak, then we'll modify the | ||
| // base tweak value to use the BIP 341 tweak value. | ||
| if opts.taprootTweak { | ||
| // Emulate the same behavior as txscript.ComputeTaprootOutputKey | ||
| // which only operates on the x-only public key. | ||
| key, _ := schnorr.ParsePubKey(schnorr.SerializePubKey( |
There was a problem hiding this comment.
I don't think this extra step is actually needed, since we commit to the x-only key below in the hash.
There was a problem hiding this comment.
Amazing work! The API is very nice to use and feature complete for our first use cases in lnd.
We might want to make it possible to supply the private and public nonces to a session for certain use cases. But IMO that can also be done in a follow-up PR.
Found a few typos while working with the API, feel free to ignore 😅
| partialSig, err := Sign( | ||
| secNonce, signSetPrivKey, aggregatedNonce, keySet, msg, | ||
| ) | ||
| if err != nil { |
There was a problem hiding this comment.
nit: use require in all tests?
|
Ok, I've pushed a final (?) set of commits that adds the new nocne generation scheme in the updated spce (as a new commit), and also updates to add the remaining test vectors. I'll make a new PR to allow the nonce generation to take place before the session itself has been created. One thing to note is that I used type params in one of the new commits, so that makes the module now require Go 1.18. |
Roasbeef
changed the title
|
Alright, I just pushed the final (for real this time) commit which adds the ability to create a context without knowing all the signers ahead of time. A test has been added to exercise this mode and show callers how it can be done. I've also started to pass in more information into the aux input for the new nonce generation. |
|
I don't know what the generics and Go 1.18 change means for btcec/v2 users. There are a huge number of projects using btcec and a handful already using btcec/v2, including go-ethereum now, and if btcec/v2 can't be built with even Go 1.17, that's something to avoid if at all possible. Possibly it only makes a difference if someone tries to compile the musig2 package, but I'm not sure. If generics become required for btcec/v2 before resolving that ambiguous import issue, there's probably no point of the fix in #1848 any more. go-ethereum (and my own projects) will likely have to revert to the btcec that shipped with v0.22.0-beta. If btcec/v2.1.4 could be cut with just that ambiguous import resolution, and/or this work would move btcec to v3, consumers could probably get by. I wanna say it's gonna be fine with only the musig2 package using generics, but I'm a little anxious about that. |
Totally reasonable, the generics usage in that PR is pretty trivial. Removing it would just mean 10 or so extra more lines. It was mostly an excuse for me to get more familiar with the new syntax/semantics. Re ordering, I think it makes sense to do your PR first, make a tag (which'll clean up that ambiguous import issue), and then do the same with this PR. Nothing in |
|
Cool, thanks for considering that. However, I think @davecgh is on to a pretty good and relatively simple solution to a handful of issues, all with one v0.22.1 tag on a branch forking back off of the v0.22.0-beta tag's commit: #1839 (comment) |
|
Ended up "unrolling" the generics usage in the latest diff: https://github.com/btcsuite/btcd/compare/afbf14a3a061b961c7fe0d21dcbbc6c941a33027..93f45d9c94cf3aaf7850fff81335e68681e1275f#diff-483dff6c6a92e57a03ff4489e863f31d6b1e48f5b8ec036602cb3802471a8829R178-R200 Pending CI, we should be g2g after this. Once this lands, I'll tag a new version of the |
Roasbeef
force-pushed
the
musig2
branch
2 times, most recently
from
58dcd30
to
94b8c57
Compare
In this commit, we add the set of key aggregation routines for musig2. This includes the main public key aggregation method, as well as the aggregation coefficient which is used to compute "mu" when signing. The logic in this implementation is based on the musig2 paper, as well as this spec: https://github.com/ElementsProject/secp256k1-zkp/blob/master/doc/musig-spec.mediawiki.
In this commit, we add the ability to generate the secret+public nonces, as well as combine a series of nonces into a single combined nonce (which is used when doing multi signing).
…bination In this commit, we build on the prior two commits by adding the ability to generate partial musig2 signatures, validate them individually, and finally combine them into a single signature. Much of the logic here is unoptimized, and will be optimized in a later commit. In addition, we also want to eventually have a nicer API to support the book keeping necessary during multi signing.
In this commit, we add test vectors which are extracted from the secp256k1-zkp/ codebase and match up with the current draft specification.
In this commit, we add a final test case that exercises the act of generating partial signatures amongst 100 signers, combining them into a single signature, and finally verifying to make sure the final signature is valid.
In this commit, we optimize signing+verification mainly by only
computing values once, and reducing allocations when possible.
The following optimizations have been implemented:
* Use a single buffer allocation in keyHashFingerprint to avoid
dynamic buffer growth+re-sizing
* Remove the isSecondKey computation and replace that with a single
routine that computes the index of the second unique key.
* Optimize keyHashFingerprint usage by only computing it once during
signing +verification.
A further optimization is possible: use the x coordinate of a key for
comparisons instead of computing the full sexualision. We need to do
the latter atm, as the X() method of the public key struct will allocate
more memory as it allocate and sets the buffer in place.
The final benchmarks of before and after this commit:
benchmark old ns/op new ns/op delta
BenchmarkPartialSign/num_signers=10/fast_sign=true/sort=true-8 1227374 1194047 -2.72%
BenchmarkPartialSign/num_signers=10/fast_sign=true/sort=false-8 1217743 1191468 -2.16%
BenchmarkPartialSign/num_signers=10/fast_sign=false/sort=true-8 2755544 2698827 -2.06%
BenchmarkPartialSign/num_signers=10/fast_sign=false/sort=false-8 2754749 2694547 -2.19%
BenchmarkPartialSign/num_signers=100/fast_sign=true/sort=true-8 12382654 10561204 -14.71%
BenchmarkPartialSign/num_signers=100/fast_sign=true/sort=false-8 12260134 10315376 -15.86%
BenchmarkPartialSign/num_signers=100/fast_sign=false/sort=true-8 24832061 22009935 -11.36%
BenchmarkPartialSign/num_signers=100/fast_sign=false/sort=false-8 24650086 21022833 -14.71%
BenchmarkPartialVerify/sort_keys=true/num_signers=10-8 1485787 1473377 -0.84%
BenchmarkPartialVerify/sort_keys=false/num_signers=10-8 1447275 1465139 +1.23%
BenchmarkPartialVerify/sort_keys=true/num_signers=100-8 12503482 10672618 -14.64%
BenchmarkPartialVerify/sort_keys=false/num_signers=100-8 12388289 10581398 -14.59%
BenchmarkCombineSigs/num_signers=10-8 0.00 0.00 +0.00%
BenchmarkCombineSigs/num_signers=100-8 0.00 0.00 -1.95%
BenchmarkAggregateNonces/num_signers=10-8 0.00 0.00 -0.76%
BenchmarkAggregateNonces/num_signers=100-8 0.00 0.00 +1.13%
BenchmarkAggregateKeys/num_signers=10/sort_keys=true-8 0.00 0.00 -0.09%
BenchmarkAggregateKeys/num_signers=10/sort_keys=false-8 0.00 0.01 +559.94%
BenchmarkAggregateKeys/num_signers=100/sort_keys=true-8 0.01 0.01 -11.30%
BenchmarkAggregateKeys/num_signers=100/sort_keys=false-8 0.01 0.01 -11.66%
benchmark old allocs new allocs delta
BenchmarkPartialSign/num_signers=10/fast_sign=true/sort=true-8 458 269 -41.27%
BenchmarkPartialSign/num_signers=10/fast_sign=true/sort=false-8 409 222 -45.72%
BenchmarkPartialSign/num_signers=10/fast_sign=false/sort=true-8 892 524 -41.26%
BenchmarkPartialSign/num_signers=10/fast_sign=false/sort=false-8 841 467 -44.47%
BenchmarkPartialSign/num_signers=100/fast_sign=true/sort=true-8 14366 3089 -78.50%
BenchmarkPartialSign/num_signers=100/fast_sign=true/sort=false-8 13143 1842 -85.98%
BenchmarkPartialSign/num_signers=100/fast_sign=false/sort=true-8 27596 4964 -82.01%
BenchmarkPartialSign/num_signers=100/fast_sign=false/sort=false-8 26309 3707 -85.91%
BenchmarkPartialVerify/sort_keys=true/num_signers=10-8 430 243 -43.49%
BenchmarkPartialVerify/sort_keys=false/num_signers=10-8 430 243 -43.49%
BenchmarkPartialVerify/sort_keys=true/num_signers=100-8 13164 1863 -85.85%
BenchmarkPartialVerify/sort_keys=false/num_signers=100-8 13164 1863 -85.85%
BenchmarkCombineSigs/num_signers=10-8 0 0 +0.00%
BenchmarkCombineSigs/num_signers=100-8 0 0 +0.00%
BenchmarkAggregateNonces/num_signers=10-8 0 0 +0.00%
BenchmarkAggregateNonces/num_signers=100-8 0 0 +0.00%
BenchmarkAggregateKeys/num_signers=10/sort_keys=true-8 0 0 +0.00%
BenchmarkAggregateKeys/num_signers=10/sort_keys=false-8 0 0 +0.00%
BenchmarkAggregateKeys/num_signers=100/sort_keys=true-8 0 0 +0.00%
BenchmarkAggregateKeys/num_signers=100/sort_keys=false-8 0 0 +0.00%
benchmark old bytes new bytes delta
BenchmarkPartialSign/num_signers=10/fast_sign=true/sort=true-8 27854 14878 -46.59%
BenchmarkPartialSign/num_signers=10/fast_sign=true/sort=false-8 25508 12605 -50.58%
BenchmarkPartialSign/num_signers=10/fast_sign=false/sort=true-8 54982 29476 -46.39%
BenchmarkPartialSign/num_signers=10/fast_sign=false/sort=false-8 52581 26805 -49.02%
BenchmarkPartialSign/num_signers=100/fast_sign=true/sort=true-8 1880138 166996 -91.12%
BenchmarkPartialSign/num_signers=100/fast_sign=true/sort=false-8 1820561 106295 -94.16%
BenchmarkPartialSign/num_signers=100/fast_sign=false/sort=true-8 3706291 275344 -92.57%
BenchmarkPartialSign/num_signers=100/fast_sign=false/sort=false-8 3642725 214122 -94.12%
BenchmarkPartialVerify/sort_keys=true/num_signers=10-8 26995 14078 -47.85%
BenchmarkPartialVerify/sort_keys=false/num_signers=10-8 26980 14078 -47.82%
BenchmarkPartialVerify/sort_keys=true/num_signers=100-8 1822043 107767 -94.09%
BenchmarkPartialVerify/sort_keys=false/num_signers=100-8 1822046 107752 -94.09%
BenchmarkCombineSigs/num_signers=10-8 0 0 +0.00%
BenchmarkCombineSigs/num_signers=100-8 0 0 +0.00%
BenchmarkAggregateNonces/num_signers=10-8 0 0 +0.00%
BenchmarkAggregateNonces/num_signers=100-8 0 0 +0.00%
BenchmarkAggregateKeys/num_signers=10/sort_keys=true-8 0 0 +0.00%
BenchmarkAggregateKeys/num_signers=10/sort_keys=false-8 0 0 +0.00%
BenchmarkAggregateKeys/num_signers=100/sort_keys=true-8 0 0 +0.00%
BenchmarkAggregateKeys/num_signers=100/sort_keys=false-8 0 0 +0.00%
In this commit, we introduce an easier to use API for musig2 signing in the Session and Context structs. The Context struct represents a particular musig2 signing context which is defined by the set of signers. The struct can be serialized to disk as it contains no volatile information. A given context can be kept for each signer in the final set. The Session struct represents an ephemeral musig2 signing session. It handles nonce generation, key aggregation, nonce combination, signature combination, and final sig verification all in one API. The API also protects against nonce generation by not exposing nonces to the end user and also attempting to catch nonce re-use (assuming no process forking) across sessions.
In this commit, we add support for signing with tweaked aggregated keys. Such signing is required when signing for a taproot output key that actually commits to a script tree root, or was generated using BIP 86. A series of new functional arguments (that can likely be de-dup'd using Go's new type params), have been added to allow callers to optionally flip on this new behavior.
In this commit, we add a series of new options and methods to make it easier to use the package in the context of a taproot output that commits to a script root or some other value. Before this series of changes, the API was hard to use in this context as the taproot tweak actually includes the internal public key, which in this case is the aggregated public key. So you actually needed to call that API w/o the tweak, get that, then recompute the tweak itself. To make things easier in the taproot context, we've added a series of new options that'll return the aggregated key before any tweaks (to be used as the internal key), and also handle computing the BIP 341 tweak value for the caller.
In this commit, we add a series of new functional optinos to make signing for an aggregated key where the final taproot output key was derived using BIP 86. This can be used in cases where no script path shuold be allowed, and only an n-of-n multi-sig should be used.
In this commit, we update the nonce generation to support optional parameters defined in the latest BIP draft. These parameters are optional, but if specified my mitigate the effect of weak randomness when generating the nonce. Given the protocol doesn't require signers to prove how they generate their nonces, this update is mainly to ensure strict spec compliance, and is effectively optional.
In this commit, we enable early nonce generation, allowing callers to obtain generated nonces before the total set of signers is actually known. This type of nonce generation is useful for contexts like LN funding when we want to minimize the round trips and send nonces before we know the pubkey of the other party.
|
Build looking good now, thanks for all the review! Excited for devs to experiment w/ this stuff in the wild |
In this commit, we add a fully featured
musig2implementation capable of generating partial signatures, combining them, and finally ensuring that the combined signature is a valid BIP-340 schnorr signature. This implementation is based off of this spec from thesecp256k1-zkprepo.I consider much of the external API to still be a WIP, but everything is functional as is. The API will likely evolve as we start to use it upstream in
lnd,pool,loop, etc, etc.Before this PR is ready for merging, I want to further optimize it (adding benchmarks along the way), as during signing and nonce generation a lot of values end up being computed multiple times, when instead they can be computed once and memoized from there. I think it would also be useful to export a nicer version of the bookkeeping struct created in the final test for multi party signing.
TODOs