-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
Musig2: Update to 1.0.0.rc2 #1913
Conversation
Pull Request Test Coverage Report for Build 3373590167Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
264452c
to
baa44c2
Compare
baa44c2
to
bbe104d
Compare
jonasnick/bips#74 is now merged, no new changes to the reference code have been made since my last commits cc @Roasbeef |
This commit adds the pk option to NonceGen and makes it mandatory. Reference: jonasnick/bips@a89f857
This commit adds the public key to the sec nonce and ensures that we're signing with the right key. Reference: jonasnick/bips#74
This commit adds a check that the public key of the private key that is passed to the Sign function is included in the slice of public keys. Reference jonasnick/bips@ea47d52
bbe104d
to
d99a169
Compare
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.
Thanks for the update, LGTM 🎉
0, | ||
1 | ||
], | ||
"key_indices": [ |
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.
nit: avoid these formatting changes (probably done by the IDE) to keep the diff smaller?
|
||
// ErrPubkeyInvalid is returned when the pubkey of the WithPublicKey | ||
// option is not passed or of invalid length. | ||
ErrPubkeyInvalid = errors.New("nonce generation requires a valid pubkey") |
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.
nit: use PubKey
capitalization in name?
@@ -142,6 +151,14 @@ func WithCustomRand(r io.Reader) NonceGenOption { | |||
} | |||
} | |||
|
|||
// WithPublicKey is the mandatory public key that will be mixed into the nonce |
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.
Not sure if it's obvious from the context or whether we should add "the mandatory public key that corresponds to the signer's private key" or something to that effect to the comment?
@@ -38,6 +38,10 @@ var ( | |||
// ErrSecretNonceZero is returned when a secret nonce is passed in a | |||
// zero. | |||
ErrSecretNonceZero = fmt.Errorf("secret nonce is blank") | |||
|
|||
// ErrSecNoncePubkey is returned when the signing key does not match the |
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.
nit: capitalization of PubKey and missing full stop.
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.
LGTM 🥟
Will make another PR to fix the issue with the benchmark.
This PR adds the changes from jonasnick/bips#74 which adresses the risk described in https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-October/021000.html .
Will leave it as draft, until the bip PR is merged.
The
bench_test.go/BenchmarkPartialVerify
are failing, however they also seem to be failing on master.