tree: fix mock signer nonce handling in signing tests - #1090
Merged
Conversation
In this commit, we fix two MuSig2 nonce handling bugs in the mockMuSig2Signer used by the lib/tree signing tests. First, MuSig2CreateSession advertised a freshly generated nonce in the session info but created the underlying musig2 session via ctx.NewSession(), which generates its own internal nonce pair. The nonce peers aggregated was never the nonce the session signed with. We now pass the advertised nonces into the session via musig2.WithPreGeneratedNonce. Second, MuSig2RegisterCombinedNonce registered the pre-aggregated nonce through RegisterPubNonce, which treats the aggregate as a single peer nonce. The session's combined nonce then became ownNonce + aggregate instead of the aggregate, and haveAll only flipped for 2-signer sessions. We now use the RegisterCombinedNonce primitive that btcec v2.5.0 provides for exactly this coordinator-style flow. These bugs were invisible because every ceremony in the suite is 2-of-2 and no test ever combined partial signatures across participants: a partial signature over a wrong combined nonce looks identical to a correct one until the aggregate is verified. We close that gap by extending TestFullSigningFlow to combine both parties' partial signatures per transaction and verify the final Schnorr signature against the node's final taproot key. Both subtests fail against the old mock with ErrFinalSigInvalid and pass with the fix.
In this commit, we port the lib/tree mock signer fixes to the round test harness's realMuSig2Signer, which was copied from the same broken pattern: MuSig2CreateSession created its musig2 session without WithPreGeneratedNonce, so the advertised nonce was never the one the session signed with, and MuSig2RegisterCombinedNonce fed the aggregate through RegisterPubNonce, corrupting the session's combined nonce and only flipping haveAll for 2-signer sessions. No round test currently drives a full combine through this signer: the error-path test (newUnaggregatedSignerSession) still gets its expected "not all nonces registered" failure since allNoncesKnown starts false, and generateValidTreeSignatures builds raw btcd sessions directly for real combines. The fix is about keeping the harness a correct reference for input.MuSig2Signer behavior rather than changing any current test outcome.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In this PR, we fix two MuSig2 nonce handling bugs in the
mockMuSig2Signerthat backs thelib/treesigning tests, and extend the suite so the full ceremony is actually verified end to end.The mock had two problems that stayed invisible in the existing suite.
MuSig2CreateSessionadvertised a freshly generated nonce in the session info, but created the underlying session viactx.NewSession(), which generates its own internal pair, so peers aggregated a nonce the session never signed with. AndMuSig2RegisterCombinedNoncefed the pre-aggregated nonce throughRegisterPubNonce, which treats the aggregate as one peer's nonce: the session's combined nonce becameownNonce + aggregateinstead of the aggregate, andhaveAllonly flipped for 2-signer sessions. btcec v2.5.0 has the right primitive for this coordinator-style flow,Session.RegisterCombinedNonce, and the mock now uses it along withmusig2.WithPreGeneratedNonceat session creation.Why the suite never noticed
Every ceremony in the lib/tree tests is 2-of-2, so the miscounted registration still flipped
haveAll, and no test ever combined partial signatures across participants:TestFullSigningFlowstopped at "partial signature is non-nil", which a signature over a wrong combined nonce satisfies just as well as a correct one. The pattern only blew up when the asset-tree e2e in #1084 ran a real multi-party ceremony against an in-process signer copied from this mock and hit "not all nonces registered".To keep the mock honest going forward, we extend both
TestFullSigningFlowsubtests to combine the two parties' partial signatures for every transaction and verify the final Schnorr signature against the node's final taproot key. Both subtests fail against the old mock withErrFinalSigInvalidand pass with the fix.The real production path is covered by the lumos itests (real client daemon against the in-process operator), so this is a test-only change: the value is a correct in-repo reference for
input.MuSig2Signerbehavior and a fast local signal for lib/tree signing regressions. The second commit ports the same fix toround/harness_test.go'srealMuSig2Signer, which was copied from this mock. No round test outcome changes: the error-path test that deliberately skips nonce registration still gets its expected "not all nonces registered" failure, and the harness's real combines already went through raw btcd sessions.