From 69a1cf4f2382f2b7725fdbc5a67d7d43896b52fd Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 28 Aug 2024 18:23:14 -0500 Subject: [PATCH 1/4] lnwallet: ensure we re-sign retransmitted commits for taproot channels In this commit, we fix an existing bug with the taproot channel type that can cause force closes if a peer disconnects while attempting to send the commitment signature. Before this commit, since the `PartialSig` we send is never committed to disk, the version read wouldn't contain the musig2 partial sig. We never write these signatures to disk, as each time we make a new session, we need to generate fresh nonces to avoid nonce-reuse. Due to the above interaction, if we went to re-send a signature after a disconnection, the `CommitSig` message we sent wouldn't actually contain a `PartialSigWithNonce`, causing a protocol error. --- lnwallet/channel.go | 38 +++++++++++++++++++++++++++++++++++--- 1 file changed, 35 insertions(+), 3 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 81930aad5..8dba07973 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -4151,6 +4151,27 @@ func (lc *LightningChannel) SignNextCommitment() (*NewCommitState, error) { }, nil } +// resignMusigCommit is used to resign a commitment transaction for taproot +// channels when we need to retransmit a signature after a channel reestablish +// message. Taproot channels use musig2, which means we must use fresh nonces +// each time. After we receive the channel reestablish message, we learn the +// nonce we need to use for the remote party. As a result, we need to generate +// the partial signature again with the new nonce. +func (lc *LightningChannel) resignMusigCommit(commitTx *wire.MsgTx, +) (lnwire.OptPartialSigWithNonceTLV, error) { + + remoteSession := lc.musigSessions.RemoteSession + musig, err := remoteSession.SignCommit(commitTx) + if err != nil { + var none lnwire.OptPartialSigWithNonceTLV + return none, err + } + + partialSig := lnwire.MaybePartialSigWithNonce(musig.ToWireSig()) + + return partialSig, nil +} + // ProcessChanSyncMsg processes a ChannelReestablish message sent by the remote // connection upon re establishment of our connection with them. This method // will return a single message if we are currently out of sync, otherwise a @@ -4428,12 +4449,23 @@ func (lc *LightningChannel) ProcessChanSyncMsg( commitUpdates = append(commitUpdates, logUpdate.UpdateMsg) } + // If this is a taproot channel, then we need to regenerate the + // musig2 signature for the remote party, using their fresh + // nonce. + if lc.channelState.ChanType.IsTaproot() { + partialSig, err := lc.resignMusigCommit( + commitDiff.Commitment.CommitTx, + ) + if err != nil { + return nil, nil, nil, err + } + + commitDiff.CommitSig.PartialSig = partialSig + } + // With the batch of updates accumulated, we'll now re-send the // original CommitSig message required to re-sync their remote // commitment chain with our local version of their chain. - // - // TODO(roasbeef): need to re-sign commitment states w/ - // fresh nonce commitUpdates = append(commitUpdates, commitDiff.CommitSig) // NOTE: If a revocation is not owed, then updates is empty. From 528199839a09a2c0c44b0e7e457dbfd79a1d3d55 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 28 Aug 2024 18:24:20 -0500 Subject: [PATCH 2/4] lnwallet: extract initMusigNonce from initRevocationWindows This'll be useful later to make some enhancements to the existing unit tests. --- lnwallet/test_utils.go | 36 +++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/lnwallet/test_utils.go b/lnwallet/test_utils.go index 2c12c83ae..5e0cac1f8 100644 --- a/lnwallet/test_utils.go +++ b/lnwallet/test_utils.go @@ -435,6 +435,28 @@ func CreateTestChannels(t *testing.T, chanType channeldb.ChannelType, return channelAlice, channelBob, nil } +// initMusigNonce is used to manually setup musig2 nonces for a new channel, +// outside the normal chan-reest flow. +func initMusigNonce(chanA, chanB *LightningChannel) error { + chanANonces, err := chanA.GenMusigNonces() + if err != nil { + return err + } + chanBNonces, err := chanB.GenMusigNonces() + if err != nil { + return err + } + + if err := chanA.InitRemoteMusigNonces(chanBNonces); err != nil { + return err + } + if err := chanB.InitRemoteMusigNonces(chanANonces); err != nil { + return err + } + + return nil +} + // initRevocationWindows simulates a new channel being opened within the p2p // network by populating the initial revocation windows of the passed // commitment state machines. @@ -443,19 +465,7 @@ func initRevocationWindows(chanA, chanB *LightningChannel) error { // either FundingLocked or ChannelReestablish by calling // InitRemoteMusigNonces for both sides. if chanA.channelState.ChanType.IsTaproot() { - chanANonces, err := chanA.GenMusigNonces() - if err != nil { - return err - } - chanBNonces, err := chanB.GenMusigNonces() - if err != nil { - return err - } - - if err := chanA.InitRemoteMusigNonces(chanBNonces); err != nil { - return err - } - if err := chanB.InitRemoteMusigNonces(chanANonces); err != nil { + if err := initMusigNonce(chanA, chanB); err != nil { return err } } From b8e39c231104eb72f7b9f483ce04b5332730d760 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Wed, 28 Aug 2024 18:26:51 -0500 Subject: [PATCH 3/4] lnwallet: expand chan sync tests to cover taproot channels In this commit, we expand some of the existing chan sync tests to cover taproot channels (the others already did). Along the way, we always assert that the `PartialSig` is populated on retransmission. In addition, we now send the new commit sig rather than the existing in-memory one to test the new logic that re-signs the commitment. --- lnwallet/channel_test.go | 186 +++++++++++++++++++++++++++++++++------ 1 file changed, 161 insertions(+), 25 deletions(-) diff --git a/lnwallet/channel_test.go b/lnwallet/channel_test.go index f39dd2d43..e28098052 100644 --- a/lnwallet/channel_test.go +++ b/lnwallet/channel_test.go @@ -3046,19 +3046,11 @@ func restartChannel(channelOld *LightningChannel) (*LightningChannel, error) { return channelNew, nil } -// TestChanSyncOweCommitment tests that if Bob restarts (and then Alice) before -// he receives Alice's CommitSig message, then Alice concludes that she needs -// to re-send the CommitDiff. After the diff has been sent, both nodes should -// resynchronize and be able to complete the dangling commit. -func TestChanSyncOweCommitment(t *testing.T) { - t.Parallel() - +func testChanSyncOweCommitment(t *testing.T, chanType channeldb.ChannelType) { // Create a test channel which will be used for the duration of this // unittest. The channel will be funded evenly with Alice having 5 BTC, // and Bob having 5 BTC. - aliceChannel, bobChannel, err := CreateTestChannels( - t, channeldb.SingleFunderTweaklessBit, - ) + aliceChannel, bobChannel, err := CreateTestChannels(t, chanType) require.NoError(t, err, "unable to create test channels") var fakeOnionBlob [lnwire.OnionPacketSize]byte @@ -3133,6 +3125,15 @@ func TestChanSyncOweCommitment(t *testing.T) { aliceNewCommit, err := aliceChannel.SignNextCommitment() require.NoError(t, err, "unable to sign commitment") + // If this is a taproot channel, then we'll generate fresh verification + // nonce for both sides. + if chanType.IsTaproot() { + _, err = aliceChannel.GenMusigNonces() + require.NoError(t, err) + _, err = bobChannel.GenMusigNonces() + require.NoError(t, err) + } + // Bob doesn't get this message so upon reconnection, they need to // synchronize. Alice should conclude that she owes Bob a commitment, // while Bob should think he's properly synchronized. @@ -3144,7 +3145,7 @@ func TestChanSyncOweCommitment(t *testing.T) { // This is a helper function that asserts Alice concludes that she // needs to retransmit the exact commitment that we failed to send // above. - assertAliceCommitRetransmit := func() { + assertAliceCommitRetransmit := func() *lnwire.CommitSig { aliceMsgsToSend, _, _, err := aliceChannel.ProcessChanSyncMsg( bobSyncMsg, ) @@ -3209,12 +3210,25 @@ func TestChanSyncOweCommitment(t *testing.T) { len(commitSigMsg.HtlcSigs)) } for i, htlcSig := range commitSigMsg.HtlcSigs { - if htlcSig != aliceNewCommit.HtlcSigs[i] { + if !bytes.Equal(htlcSig.RawBytes(), + aliceNewCommit.HtlcSigs[i].RawBytes()) { + t.Fatalf("htlc sig msgs don't match: "+ - "expected %x got %x", - aliceNewCommit.HtlcSigs[i], htlcSig) + "expected %v got %v", + spew.Sdump(aliceNewCommit.HtlcSigs[i]), + spew.Sdump(htlcSig)) } } + + // If this is a taproot channel, then partial sig information + // should be present in the commit sig sent over. This + // signature will be re-regenerated, so we can't compare it + // with the old one. + if chanType.IsTaproot() { + require.True(t, commitSigMsg.PartialSig.IsSome()) + } + + return commitSigMsg } // Alice should detect that she needs to re-send 5 messages: the 3 @@ -3235,14 +3249,19 @@ func TestChanSyncOweCommitment(t *testing.T) { // send the exact same set of messages. aliceChannel, err = restartChannel(aliceChannel) require.NoError(t, err, "unable to restart alice") - assertAliceCommitRetransmit() - // TODO(roasbeef): restart bob as well??? + // To properly simulate a restart, we'll use the *new* signature that + // would send in an actual p2p setting. + aliceReCommitSig := assertAliceCommitRetransmit() // At this point, we should be able to resume the prior state update // without any issues, resulting in Alice settling the 3 htlc's, and // adding one of her own. - err = bobChannel.ReceiveNewCommitment(aliceNewCommit.CommitSigs) + err = bobChannel.ReceiveNewCommitment(&CommitSigs{ + CommitSig: aliceReCommitSig.CommitSig, + HtlcSigs: aliceReCommitSig.HtlcSigs, + PartialSig: aliceReCommitSig.PartialSig, + }) require.NoError(t, err, "bob unable to process alice's commitment") bobRevocation, _, _, err := bobChannel.RevokeCurrentCommitment() require.NoError(t, err, "unable to revoke bob commitment") @@ -3329,16 +3348,53 @@ func TestChanSyncOweCommitment(t *testing.T) { } } -// TestChanSyncOweCommitmentPendingRemote asserts that local updates are applied -// to the remote commit across restarts. -func TestChanSyncOweCommitmentPendingRemote(t *testing.T) { +// TestChanSyncOweCommitment tests that if Bob restarts (and then Alice) before +// he receives Alice's CommitSig message, then Alice concludes that she needs +// to re-send the CommitDiff. After the diff has been sent, both nodes should +// resynchronize and be able to complete the dangling commit. +func TestChanSyncOweCommitment(t *testing.T) { t.Parallel() + testCases := []struct { + name string + chanType channeldb.ChannelType + }{ + { + name: "tweakless", + chanType: channeldb.SingleFunderTweaklessBit, + }, + { + name: "anchors", + chanType: channeldb.SingleFunderTweaklessBit | + channeldb.AnchorOutputsBit, + }, + { + name: "taproot", + chanType: channeldb.SingleFunderTweaklessBit | + channeldb.AnchorOutputsBit | + channeldb.SimpleTaprootFeatureBit, + }, + { + name: "taproot with tapscript root", + chanType: channeldb.SingleFunderTweaklessBit | + channeldb.AnchorOutputsBit | + channeldb.SimpleTaprootFeatureBit | + channeldb.TapscriptRootBit, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + testChanSyncOweCommitment(t, tc.chanType) + }) + } +} + +func testChanSyncOweCommitmentPendingRemote(t *testing.T, + chanType channeldb.ChannelType) { + // Create a test channel which will be used for the duration of this // unittest. - aliceChannel, bobChannel, err := CreateTestChannels( - t, channeldb.SingleFunderTweaklessBit, - ) + aliceChannel, bobChannel, err := CreateTestChannels(t, chanType) require.NoError(t, err, "unable to create test channels") var fakeOnionBlob [lnwire.OnionPacketSize]byte @@ -3421,6 +3477,12 @@ func TestChanSyncOweCommitmentPendingRemote(t *testing.T) { bobChannel, err = restartChannel(bobChannel) require.NoError(t, err, "unable to restart bob") + // If this is a taproot channel, then since Bob just restarted, we need + // to exchange nonces once again. + if chanType.IsTaproot() { + require.NoError(t, initMusigNonce(aliceChannel, bobChannel)) + } + // Bob signs the commitment he owes. bobNewCommit, err := bobChannel.SignNextCommitment() require.NoError(t, err, "unable to sign commitment") @@ -3446,6 +3508,45 @@ func TestChanSyncOweCommitmentPendingRemote(t *testing.T) { } } +// TestChanSyncOweCommitmentPendingRemote asserts that local updates are applied +// to the remote commit across restarts. +func TestChanSyncOweCommitmentPendingRemote(t *testing.T) { + t.Parallel() + + testCases := []struct { + name string + chanType channeldb.ChannelType + }{ + { + name: "tweakless", + chanType: channeldb.SingleFunderTweaklessBit, + }, + { + name: "anchors", + chanType: channeldb.SingleFunderTweaklessBit | + channeldb.AnchorOutputsBit, + }, + { + name: "taproot", + chanType: channeldb.SingleFunderTweaklessBit | + channeldb.AnchorOutputsBit | + channeldb.SimpleTaprootFeatureBit, + }, + { + name: "taproot with tapscript root", + chanType: channeldb.SingleFunderTweaklessBit | + channeldb.AnchorOutputsBit | + channeldb.SimpleTaprootFeatureBit | + channeldb.TapscriptRootBit, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + testChanSyncOweCommitmentPendingRemote(t, tc.chanType) + }) + } +} + // testChanSyncOweRevocation is the internal version of // TestChanSyncOweRevocation that is parameterized based on the type of channel // being used in the test. @@ -3595,8 +3696,6 @@ func testChanSyncOweRevocation(t *testing.T, chanType channeldb.ChannelType) { assertAliceOwesRevoke() - // TODO(roasbeef): restart bob too??? - // We'll continue by then allowing bob to process Alice's revocation // message. _, _, _, _, err = bobChannel.ReceiveRevocation(aliceRevocation) @@ -3645,6 +3744,15 @@ func TestChanSyncOweRevocation(t *testing.T) { testChanSyncOweRevocation(t, taprootBits) }) + t.Run("taproot with tapscript root", func(t *testing.T) { + taprootBits := channeldb.SimpleTaprootFeatureBit | + channeldb.AnchorOutputsBit | + channeldb.ZeroHtlcTxFeeBit | + channeldb.SingleFunderTweaklessBit | + channeldb.TapscriptRootBit + + testChanSyncOweRevocation(t, taprootBits) + }) } func testChanSyncOweRevocationAndCommit(t *testing.T, @@ -3774,6 +3882,14 @@ func testChanSyncOweRevocationAndCommit(t *testing.T, bobNewCommit.HtlcSigs[i]) } } + + // If this is a taproot channel, then partial sig information + // should be present in the commit sig sent over. This + // signature will be re-regenerated, so we can't compare it + // with the old one. + if chanType.IsTaproot() { + require.True(t, bobReCommitSigMsg.PartialSig.IsSome()) + } } // We expect Bob to send exactly two messages: first his revocation @@ -3830,6 +3946,15 @@ func TestChanSyncOweRevocationAndCommit(t *testing.T) { testChanSyncOweRevocationAndCommit(t, taprootBits) }) + t.Run("taproot with tapscript root", func(t *testing.T) { + taprootBits := channeldb.SimpleTaprootFeatureBit | + channeldb.AnchorOutputsBit | + channeldb.ZeroHtlcTxFeeBit | + channeldb.SingleFunderTweaklessBit | + channeldb.TapscriptRootBit + + testChanSyncOweRevocationAndCommit(t, taprootBits) + }) } func testChanSyncOweRevocationAndCommitForceTransition(t *testing.T, @@ -4061,6 +4186,17 @@ func TestChanSyncOweRevocationAndCommitForceTransition(t *testing.T) { t, taprootBits, ) }) + t.Run("taproot with tapscript root", func(t *testing.T) { + taprootBits := channeldb.SimpleTaprootFeatureBit | + channeldb.AnchorOutputsBit | + channeldb.ZeroHtlcTxFeeBit | + channeldb.SingleFunderTweaklessBit | + channeldb.TapscriptRootBit + + testChanSyncOweRevocationAndCommitForceTransition( + t, taprootBits, + ) + }) } // TestChanSyncFailure tests the various scenarios during channel sync where we From 80b257991ee9aa1500b844045a3d0fa752e80fc1 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 3 Sep 2024 17:20:41 -0700 Subject: [PATCH 4/4] docs/release-notes: add release notes entry for taproot chans bug fix --- docs/release-notes/release-notes-0.18.3.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/release-notes/release-notes-0.18.3.md b/docs/release-notes/release-notes-0.18.3.md index b91fda584..c00623d8b 100644 --- a/docs/release-notes/release-notes-0.18.3.md +++ b/docs/release-notes/release-notes-0.18.3.md @@ -80,6 +80,10 @@ blinded path expiry. * [Fix a bug](https://github.com/lightningnetwork/lnd/pull/9039) that would cause UpdateAddHTLC message with blinding point fields to not be re-forwarded correctly on restart. + +* [A bug related to sending dangling channel + updates](https://github.com/lightningnetwork/lnd/pull/9046) after a + reconnection for taproot channels has been fixed. # New Features ## Functional Enhancements