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: fix early nonce gen option #2003

Merged
merged 2 commits into from
Jul 11, 2023

Conversation

sputn1ck
Copy link
Collaborator

Previously the early nonce generation option was not being respected when creating the context, with the WithKnownSigners option being used.

@coveralls
Copy link

coveralls commented Jul 10, 2023

Pull Request Test Coverage Report for Build 5519660551

  • 7 of 9 (77.78%) changed or added relevant lines in 1 file are covered.
  • 10 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.02%) to 55.24%

Changes Missing Coverage Covered Lines Changed/Added Lines %
btcec/schnorr/musig2/context.go 7 9 77.78%
Files with Coverage Reduction New Missed Lines %
peer/peer.go 10 73.2%
Totals Coverage Status
Change from base Build 5512933726: -0.02%
Covered Lines: 26692
Relevant Lines: 48320

💛 - Coveralls

@sputn1ck sputn1ck requested review from Roasbeef and guggero July 10, 2023 14:45
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.

Motivation makes sense, can we tack on a unit test to ensure the new code path works as expected and as a guard against regressions?

default:
return nil, ErrSignersNotSpecified
}

// If early nonce generation is specified, then we'll generate
Copy link
Member

Choose a reason for hiding this comment

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

Ah I guess my previous use case always had the number of signers specified (case of LN channel funding).

@sputn1ck sputn1ck force-pushed the musig2_fix_early_nonce_gen branch 2 times, most recently from 39ec882 to ff0d41d Compare July 10, 2023 19:42
@sputn1ck
Copy link
Collaborator Author

Motivation makes sense, can we tack on a unit test to ensure the new code path works as expected and as a guard against regressions?

I adapted the early nonce gen test to use both ways of creating a context. Can also add a completely new unit test if preferred.

@sputn1ck sputn1ck requested a review from Roasbeef July 10, 2023 19:51
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.

Nice fix, LGTM 🎉

sputn1ck added 2 commits July 11, 2023 14:01
Previously the early nonce generation option was not being respected
when creating the context, with the WithKnownSigners option being
used. This commit fixes that.
This commit changes the early nonce gen test to use the KnownSigners
Option for one of the contexts.
@sputn1ck sputn1ck force-pushed the musig2_fix_early_nonce_gen branch from ff0d41d to 883a03d Compare July 11, 2023 12:02
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 🎎

@Roasbeef Roasbeef merged commit 7faa9b2 into btcsuite:master Jul 11, 2023
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