Skip to content
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: Catch up to 0.4.0 #1865

Merged
merged 11 commits into from
Aug 11, 2022
Merged

MuSig2: Catch up to 0.4.0 #1865

merged 11 commits into from
Aug 11, 2022

Conversation

sputn1ck
Copy link
Collaborator

@sputn1ck sputn1ck commented Jun 24, 2022

This PR fixes nonce generation to include XORing the passed secret key with the random bytes. (see: https://github.com/jonasnick/bips/blob/musig2/bip-musig2.mediawiki#nonce-generation-1)

Additionally I updated the nonce generation to 0.3.0 and added the testcases
(https://github.com/jonasnick/bips/blob/musig2/bip-musig2.mediawiki#Change_Log)
(jonasnick/bips@f421692)

I'm currently in the progress of catching up to 0.4.0

  • 0.3.0: Hash i - 1 instead of i in NonceGen
  • 0.3.1: Add NonceGen test vectors
  • 0.3.2: Add a lot of test vectors and improve handling of invalid contributions in reference code.
    • Aggregate Keys testvectors
    • Aggregate Nonces testvectores
    • Sign testvectors
    • Partial Sign testvectors
    • Sig Agg testvectors
  • 0.4.0: Allow the output of NonceAgg to be infinity and add test vectors

@sputn1ck sputn1ck force-pushed the musig2_0.3.0 branch 2 times, most recently from 1a73614 to a862f9b Compare June 27, 2022 21:59
@sputn1ck sputn1ck changed the title MuSig2: Fix nonce generation and update to 0.3.0 MuSig2: Catch up to 0.4.0 Jun 27, 2022
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Happy to see there weren't any fundamental correctness issues discovered with the addition of the new test vectors.

Completed an initial pass, mainly some style comments, and also some suggestions to elevate the utility functions added to the top-level module. This'll allow us to de-dup common routines like parsing a point into jacobian coords across a few of our projects.

btcec/schnorr/musig2/nonces.go Outdated Show resolved Hide resolved
btcec/schnorr/musig2/musig2_test.go Outdated Show resolved Hide resolved
btcec/schnorr/musig2/keys.go Show resolved Hide resolved
btcec/schnorr/musig2/musig2_test.go Outdated Show resolved Hide resolved
btcec/schnorr/musig2/musig2_test.go Outdated Show resolved Hide resolved
btcec/schnorr/musig2/nonces.go Outdated Show resolved Hide resolved
btcec/schnorr/musig2/nonces.go Outdated Show resolved Hide resolved
btcec/schnorr/musig2/nonces.go Outdated Show resolved Hide resolved
btcec/schnorr/musig2/sign.go Outdated Show resolved Hide resolved
btcec/schnorr/musig2/musig2_test.go Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Jul 15, 2022

Pull Request Test Coverage Report for Build 2759678038

Warning: 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

  • 106 of 117 (90.6%) changed or added relevant lines in 6 files are covered.
  • 21 unchanged lines in 4 files lost coverage.
  • Overall coverage increased (+0.08%) to 55.121%

Changes Missing Coverage Covered Lines Changed/Added Lines %
btcec/error.go 0 3 0.0%
btcec/curve.go 28 32 87.5%
btcec/schnorr/musig2/sign.go 40 44 90.91%
Files with Coverage Reduction New Missed Lines %
connmgr/connmanager.go 2 86.07%
txscript/taproot.go 2 95.93%
database/ffldb/blockio.go 4 92.62%
peer/peer.go 13 73.2%
Totals Coverage Status
Change from base Build 2456385554: 0.08%
Covered Lines: 26513
Relevant Lines: 48100

💛 - Coveralls

@Roasbeef
Copy link
Member

Bump.

@sputn1ck sputn1ck force-pushed the musig2_0.3.0 branch 3 times, most recently from d7acd5d to cb1d4b5 Compare July 29, 2022 09:25
@sputn1ck
Copy link
Collaborator Author

Added the requested changes, however I'm unsure re continue on error vs return

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Latest changes are looking really good! Just a few minor comments

btcec/btcec.go Show resolved Hide resolved
btcec/schnorr/musig2/keys.go Show resolved Hide resolved
btcec/schnorr/musig2/musig2_test.go Outdated Show resolved Hide resolved
return nil, err
}

var nonce, r1J, r2J btcec.JacobianPoint
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mean reducing duplication by moving some of the above into a new routine, so we can use it elsewhere. Don't consider it a blocker though, IIRC there's an existing TODO lingering around somewhere for it

btcec/curve.go Outdated Show resolved Hide resolved
btcec/curve.go Outdated Show resolved Hide resolved
)

nonceJ, err := btcec.ParseJacobian(
slicer(pubNonceBytes))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it can fit all one the next line? If not, the closing paren here should be on a new line

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be 81 chars

@Roasbeef
Copy link
Member

Roasbeef commented Aug 2, 2022

cc @guggero

Copy link
Collaborator

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice PR! Just a few minor comments, mostly style nits.

btcec/schnorr/musig2/nonces.go Outdated Show resolved Hide resolved
btcec/schnorr/musig2/nonces.go Outdated Show resolved Hide resolved
btcec/schnorr/musig2/musig2_test.go Show resolved Hide resolved
btcec/schnorr/musig2/musig2_test.go Show resolved Hide resolved
btcec/schnorr/musig2/nonces.go Outdated Show resolved Hide resolved
btcec/schnorr/musig2/musig2_test.go Outdated Show resolved Hide resolved
btcec/schnorr/musig2/musig2_test.go Show resolved Hide resolved
btcec/schnorr/musig2/musig2_test.go Outdated Show resolved Hide resolved
btcec/curve.go Outdated Show resolved Hide resolved
btcec/curve.go Show resolved Hide resolved
@sputn1ck
Copy link
Collaborator Author

sputn1ck commented Aug 4, 2022

Pushed the requested changes from @Roasbeef and @guggero

@sputn1ck sputn1ck requested review from Roasbeef and guggero August 4, 2022 09:59
Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🌺

This commit updates the musig2 module to allow
infinity nonces, as per Musig2 0.4.0.
@Roasbeef Roasbeef merged commit da4b534 into btcsuite:master Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants