Skip to content

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?

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