-
Notifications
You must be signed in to change notification settings - Fork 126
utils/MuSig2Sign: validate signer inputs and accept raw keys #1110
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,15 +4,40 @@ import ( | |
| "fmt" | ||
|
|
||
| "github.com/btcsuite/btcd/btcec/v2" | ||
| "github.com/btcsuite/btcd/btcec/v2/schnorr" | ||
| "github.com/btcsuite/btcd/btcec/v2/schnorr/musig2" | ||
| "github.com/lightningnetwork/lnd/input" | ||
| ) | ||
|
|
||
| // MuSig2Sign will create a MuSig2 signature for the passed message using the | ||
| // passed private keys. | ||
| func MuSig2Sign(version input.MuSig2Version, privKeys []*btcec.PrivateKey, | ||
| pubKeys []*btcec.PublicKey, tweaks *input.MuSig2Tweaks, | ||
| msg [32]byte) ([]byte, error) { | ||
| // passed raw private keys. It expects at least two signing keys. | ||
| func MuSig2Sign(version input.MuSig2Version, keys [][32]byte, | ||
| tweaks *input.MuSig2Tweaks, msg [32]byte) ([]byte, error) { | ||
|
|
||
| privKeys := make([]*btcec.PrivateKey, len(keys)) | ||
| pubKeys := make([]*btcec.PublicKey, len(keys)) | ||
|
|
||
| // First parse the raw private keys and also create the corresponding | ||
| // public keys. | ||
| for i, key := range keys { | ||
| privKeys[i], pubKeys[i] = btcec.PrivKeyFromBytes(key[:]) | ||
|
|
||
| // MuSig2 v0.4 expects x-only public keys. | ||
| if version == input.MuSig2Version040 { | ||
| pubKey := pubKeys[i].SerializeCompressed() | ||
| xOnlyPubKey, err := schnorr.ParsePubKey(pubKey[1:]) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error parsing x-only "+ | ||
| "pubkey: %v", err) | ||
| } | ||
|
|
||
| pubKeys[i] = xOnlyPubKey | ||
| } | ||
|
Comment on lines
+26
to
+35
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When using MuSig2 v0.4 with x-only public keys, if the original public key has an odd Y coordinate, the corresponding private key must be negated to match the x-only public key (which is assumed to have an even Y coordinate). Failing to do so will result in an invalid signature because the signer will be using a private key that does not correspond to the public key used in the MuSig2 aggregation. You should check the parity of the public key and negate the private key if necessary.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we need to add an extra private-key negation in our wrapper for review item 1. For Inside that implementation, Then, during signing, So the odd-Y to x-only adjustment is already handled by I added a test case to make sure that odd Y keys work. |
||
| } | ||
|
|
||
| if len(privKeys) < 2 { | ||
| return nil, fmt.Errorf("need at least two signing keys") | ||
| } | ||
|
Comment on lines
+17
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The check for the minimum number of signing keys should be performed at the beginning of the function. This avoids unnecessary allocations and processing (such as parsing keys and performing x-only conversions) when the input is invalid. if len(keys) < 2 {
return nil, fmt.Errorf("need at least two signing keys")
}
privKeys := make([]*btcec.PrivateKey, len(keys))
pubKeys := make([]*btcec.PublicKey, len(keys))
// First parse the raw private keys and also create the corresponding
// public keys.
for i, key := range keys {
privKeys[i], pubKeys[i] = btcec.PrivKeyFromBytes(key[:])
// MuSig2 v0.4 expects x-only public keys.
if version == input.MuSig2Version040 {
pubKey := pubKeys[i].SerializeCompressed()
xOnlyPubKey, err := schnorr.ParsePubKey(pubKey[1:])
if err != nil {
return nil, fmt.Errorf("error parsing x-only "+
"pubkey: %v", err)
}
pubKeys[i] = xOnlyPubKey
}
} |
||
|
|
||
| // Next we'll create MuSig2 sessions for each individual private | ||
| // signing key. | ||
|
|
@@ -74,7 +99,7 @@ func MuSig2Sign(version input.MuSig2Version, privKeys []*btcec.PrivateKey, | |
| } | ||
|
|
||
| if !haveAllSigs { | ||
| return nil, fmt.Errorf("combinging MuSig2 signatures " + | ||
| return nil, fmt.Errorf("combining MuSig2 signatures " + | ||
| "failed") | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,148 @@ | ||
| package utils | ||
|
|
||
| import ( | ||
| "testing" | ||
|
|
||
| "github.com/btcsuite/btcd/btcec/v2" | ||
| "github.com/btcsuite/btcd/btcec/v2/schnorr" | ||
| "github.com/lightninglabs/loop/test" | ||
| "github.com/lightningnetwork/lnd/input" | ||
| "github.com/stretchr/testify/require" | ||
| ) | ||
|
|
||
| // rawKeys returns serialized private keys for the given test seeds. | ||
| func rawKeys(seeds ...int32) [][32]byte { | ||
| keys := make([][32]byte, len(seeds)) | ||
| for i, seed := range seeds { | ||
| privKey, _ := test.CreateKey(seed) | ||
| copy(keys[i][:], privKey.Serialize()) | ||
| } | ||
|
|
||
| return keys | ||
| } | ||
|
|
||
| // signerPubKeys returns signer public keys derived from the given seeds in the | ||
| // format expected by the given MuSig2 version. | ||
| func signerPubKeys(t *testing.T, version input.MuSig2Version, | ||
| seeds ...int32) []*btcec.PublicKey { | ||
|
|
||
| t.Helper() | ||
|
|
||
| pubKeys := make([]*btcec.PublicKey, len(seeds)) | ||
| for i, seed := range seeds { | ||
| _, pubKey := test.CreateKey(seed) | ||
|
|
||
| if version == input.MuSig2Version040 { | ||
| var err error | ||
| pubKey, err = schnorr.ParsePubKey( | ||
| schnorr.SerializePubKey(pubKey), | ||
| ) | ||
| require.NoError(t, err) | ||
| } | ||
|
|
||
| pubKeys[i] = pubKey | ||
| } | ||
|
|
||
| return pubKeys | ||
| } | ||
|
|
||
| // hasOddY returns true if the compressed serialization of the public key uses | ||
| // the odd-Y prefix. | ||
| func hasOddY(pubKey *btcec.PublicKey) bool { | ||
| return pubKey.SerializeCompressed()[0] == 0x03 | ||
| } | ||
|
|
||
| // TestMuSig2SignRejectsSingleSigner ensures the helper fails fast with a clear | ||
| // error instead of entering an invalid one-party MuSig2 flow. | ||
| func TestMuSig2SignRejectsSingleSigner(t *testing.T) { | ||
| _, err := MuSig2Sign( | ||
| input.MuSig2Version100RC2, | ||
| rawKeys(1), | ||
| &input.MuSig2Tweaks{}, | ||
| [32]byte{}, | ||
| ) | ||
| require.ErrorContains(t, err, "need at least two signing keys") | ||
| } | ||
|
|
||
| // TestMuSig2SignSupportsVersions verifies the helper works with the supported | ||
| // MuSig2 versions used in Loop. | ||
| func TestMuSig2SignSupportsVersions(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| tweaks := &input.MuSig2Tweaks{} | ||
| msg := [32]byte{1} | ||
|
|
||
| testCases := []struct { | ||
| name string | ||
| version input.MuSig2Version | ||
| seeds []int32 | ||
| oddYSigner int32 | ||
| }{ | ||
| { | ||
| name: testVersionName(input.MuSig2Version040), | ||
| version: input.MuSig2Version040, | ||
| seeds: []int32{1, 2}, | ||
| }, | ||
| { | ||
| name: testVersionName(input.MuSig2Version100RC2), | ||
| version: input.MuSig2Version100RC2, | ||
| seeds: []int32{1, 2}, | ||
| }, | ||
| { | ||
| name: testVersionName(input.MuSig2Version040) + | ||
| " odd Y", | ||
| version: input.MuSig2Version040, | ||
| seeds: []int32{5, 1}, | ||
| oddYSigner: 5, | ||
| }, | ||
| } | ||
|
|
||
| for _, testCase := range testCases { | ||
| t.Run(testCase.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| if testCase.oddYSigner != 0 { | ||
| _, oddYKey := test.CreateKey( | ||
| testCase.oddYSigner, | ||
| ) | ||
| require.True(t, hasOddY(oddYKey)) | ||
| } | ||
|
|
||
| keys := rawKeys(testCase.seeds...) | ||
| pubKeys := signerPubKeys( | ||
| t, testCase.version, testCase.seeds..., | ||
| ) | ||
|
|
||
| sigBytes, err := MuSig2Sign( | ||
| testCase.version, keys, tweaks, msg, | ||
| ) | ||
| require.NoError(t, err) | ||
| require.Len(t, sigBytes, 64) | ||
|
|
||
| sig, err := schnorr.ParseSignature(sigBytes) | ||
| require.NoError(t, err) | ||
|
|
||
| combinedKey, err := input.MuSig2CombineKeys( | ||
| testCase.version, pubKeys, true, tweaks, | ||
| ) | ||
| require.NoError(t, err) | ||
| require.True( | ||
| t, sig.Verify(msg[:], combinedKey.FinalKey), | ||
| ) | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| // testVersionName returns a stable subtest name for a MuSig2 version. | ||
| func testVersionName(version input.MuSig2Version) string { | ||
| switch version { | ||
| case input.MuSig2Version040: | ||
| return "MuSig2 0.4" | ||
|
|
||
| case input.MuSig2Version100RC2: | ||
| return "MuSig2 1.0RC2" | ||
|
|
||
| default: | ||
| return "unknown" | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is more efficient to validate the input length before performing any cryptographic operations or memory allocations. Moving the check for at least two signing keys to the beginning of the function will allow it to fail fast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put the check after the translation to reduce the diff size. In the real use case the check never fires, so this has no performance impact.