From dc42b160a03650f2fb45ef2c65081ce21f11ed2c Mon Sep 17 00:00:00 2001 From: Eugene Siegel Date: Fri, 13 Oct 2023 08:00:49 -0700 Subject: [PATCH 1/4] multi: skip InitRemoteMusigNonces if we've already called it Prior to this commit, taproot channels had a bug: - If a disconnect happened before peer.AddNewChannel was called, then the subsequent reconnect would call peer.AddNewChannel and attempt the ChannelReestablish dance. - peer.AddNewChannel would call NewLightningChannel with populated nonce ChannelOpts. This in turn would call InitRemoteMusigNonces which would create a new musig pair session and set the channel's pendingVerificationNonce to nil. - During the reestablish dance, ProcessChanSyncMsg would be called. This would also call InitRemoteMusigNonces, except it would fail since pendingVerificationNonce was set to nil in the previous invocation. To fix this, we add a new functional option to signal to the init logic that it doesn't need to call InitRemoteMusigNonces in in ProcessChanSyncMsg. --- lnwallet/channel.go | 34 ++++++++++++++++++++++++++++++---- peer/brontide.go | 29 ++++++++++++++++++----------- 2 files changed, 48 insertions(+), 15 deletions(-) diff --git a/lnwallet/channel.go b/lnwallet/channel.go index 5ca771cb1..faef703a1 100644 --- a/lnwallet/channel.go +++ b/lnwallet/channel.go @@ -1344,6 +1344,9 @@ type LightningChannel struct { // fundingOutput is the funding output (script+value). fundingOutput wire.TxOut + // opts is the set of options that channel was initialized with. + opts *channelOpts + sync.RWMutex } @@ -1351,6 +1354,14 @@ type LightningChannel struct { // is created. type ChannelOpt func(*channelOpts) +// channelOpts is the set of options used to create a new channel. +type channelOpts struct { + localNonce *musig2.Nonces + remoteNonce *musig2.Nonces + + skipNonceInit bool +} + // WithLocalMusigNonces is used to bind an existing verification/local nonce to // a new channel. func WithLocalMusigNonces(nonce *musig2.Nonces) ChannelOpt { @@ -1367,10 +1378,15 @@ func WithRemoteMusigNonces(nonces *musig2.Nonces) ChannelOpt { } } -// channelOpts is the set of options used to create a new channel. -type channelOpts struct { - localNonce *musig2.Nonces - remoteNonce *musig2.Nonces +// WithSkipNonceInit is used to modify the way nonces are handled during +// channel initialization for taproot channels. If this option is specified, +// then when we receive the chan reest message from the remote party, we won't +// modify our nonce state. This is needed if we create a channel, get a channel +// ready message, then also get the chan reest message after that. +func WithSkipNonceInit() ChannelOpt { + return func(o *channelOpts) { + o.skipNonceInit = true + } } // defaultChannelOpts returns the set of default options for a new channel. @@ -1429,6 +1445,7 @@ func NewLightningChannel(signer input.Signer, RemoteFundingKey: state.RemoteChanCfg.MultiSigKey.PubKey, taprootNonceProducer: taprootNonceProducer, log: build.NewPrefixLog(logPrefix, walletLog), + opts: opts, } switch { @@ -4263,6 +4280,12 @@ func (lc *LightningChannel) ProcessChanSyncMsg( "not sent") case lc.channelState.ChanType.IsTaproot() && msg.LocalNonce != nil: + if lc.opts.skipNonceInit { + // Don't call InitRemoteMusigNonces if we have already + // done so. + break + } + err := lc.InitRemoteMusigNonces(&musig2.Nonces{ PubNonce: *msg.LocalNonce, }) @@ -8763,6 +8786,9 @@ func (lc *LightningChannel) InitRemoteMusigNonces(remoteNonce *musig2.Nonces, lc.pendingVerificationNonce = nil + lc.opts.localNonce = nil + lc.opts.remoteNonce = nil + return nil } diff --git a/peer/brontide.go b/peer/brontide.go index 86aa99aae..9453be39f 100644 --- a/peer/brontide.go +++ b/peer/brontide.go @@ -3824,17 +3824,6 @@ func (p *Brontide) addActiveChannel(c *lnpeer.NewChannel) error { chanPoint := &c.FundingOutpoint chanID := lnwire.NewChanIDFromOutPoint(chanPoint) - // If not already active, we'll add this channel to the set of active - // channels, so we can look it up later easily according to its channel - // ID. - lnChan, err := lnwallet.NewLightningChannel( - p.cfg.Signer, c.OpenChannel, - p.cfg.SigPool, c.ChanOpts..., - ) - if err != nil { - return fmt.Errorf("unable to create LightningChannel: %w", err) - } - // If we've reached this point, there are two possible scenarios. If // the channel was in the active channels map as nil, then it was // loaded from disk and we need to send reestablish. Else, it was not @@ -3842,6 +3831,24 @@ func (p *Brontide) addActiveChannel(c *lnpeer.NewChannel) error { // fresh channel. shouldReestablish := p.isLoadedFromDisk(chanID) + chanOpts := c.ChanOpts + if shouldReestablish { + // If we have to do the reestablish dance for this channel, + // ensure that we don't try to call InitRemoteMusigNonces twice + // by calling SkipNonceInit. + chanOpts = append(chanOpts, lnwallet.WithSkipNonceInit()) + } + + // If not already active, we'll add this channel to the set of active + // channels, so we can look it up later easily according to its channel + // ID. + lnChan, err := lnwallet.NewLightningChannel( + p.cfg.Signer, c.OpenChannel, p.cfg.SigPool, chanOpts..., + ) + if err != nil { + return fmt.Errorf("unable to create LightningChannel: %w", err) + } + // Store the channel in the activeChannels map. p.activeChannels.Store(chanID, lnChan) From 20e731b6367f60fe8c0d59da63fc11c29fe472b1 Mon Sep 17 00:00:00 2001 From: Slyghtning Date: Tue, 10 Oct 2023 19:52:30 +0200 Subject: [PATCH 2/4] itest: assertions to check channel status --- lntest/harness_assertion.go | 30 ++++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/lntest/harness_assertion.go b/lntest/harness_assertion.go index 76ff292ef..6ead7d277 100644 --- a/lntest/harness_assertion.go +++ b/lntest/harness_assertion.go @@ -352,6 +352,31 @@ func (h *HarnessTest) AssertTopologyChannelOpen(hn *node.HarnessNode, func (h *HarnessTest) AssertChannelExists(hn *node.HarnessNode, cp *lnrpc.ChannelPoint) *lnrpc.Channel { + return h.assertChannelStatus(hn, cp, true) +} + +// AssertChannelActive checks if a channel identified by the specified channel +// point is active. +func (h *HarnessTest) AssertChannelActive(hn *node.HarnessNode, + cp *lnrpc.ChannelPoint) *lnrpc.Channel { + + return h.assertChannelStatus(hn, cp, true) +} + +// AssertChannelInactive checks if a channel identified by the specified channel +// point is inactive. +func (h *HarnessTest) AssertChannelInactive(hn *node.HarnessNode, + cp *lnrpc.ChannelPoint) *lnrpc.Channel { + + return h.assertChannelStatus(hn, cp, false) +} + +// assertChannelStatus asserts that a channel identified by the specified +// channel point exists from the point-of-view of the node and that it is either +// active or inactive depending on the value of the active parameter. +func (h *HarnessTest) assertChannelStatus(hn *node.HarnessNode, + cp *lnrpc.ChannelPoint, active bool) *lnrpc.Channel { + var ( channel *lnrpc.Channel err error @@ -364,11 +389,12 @@ func (h *HarnessTest) AssertChannelExists(hn *node.HarnessNode, } // Check whether the channel is active, exit early if it is. - if channel.Active { + if channel.Active == active { return nil } - return fmt.Errorf("channel point not active") + return fmt.Errorf("expected channel_active=%v, got %v", + active, channel.Active) }, DefaultTimeout) require.NoErrorf(h, err, "%s: timeout checking for channel point: %v", From 12f23d2352d1fe4e6e4ec64af30ab25abcb1619f Mon Sep 17 00:00:00 2001 From: Slyghtning Date: Tue, 10 Oct 2023 19:54:26 +0200 Subject: [PATCH 3/4] itest: simple taproot channel status --- itest/list_on_test.go | 4 +++ itest/lnd_open_channel_test.go | 54 ++++++++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+) diff --git a/itest/list_on_test.go b/itest/list_on_test.go index 0674d646b..9b1499bd8 100644 --- a/itest/list_on_test.go +++ b/itest/list_on_test.go @@ -462,6 +462,10 @@ var allTestCases = []*lntest.TestCase{ Name: "taproot", TestFunc: testTaproot, }, + { + Name: "simple taproot channel activation", + TestFunc: testSimpleTaprootChannelActivation, + }, { Name: "wallet import account", TestFunc: testWalletImportAccount, diff --git a/itest/lnd_open_channel_test.go b/itest/lnd_open_channel_test.go index 674d9a62b..d8cf66485 100644 --- a/itest/lnd_open_channel_test.go +++ b/itest/lnd_open_channel_test.go @@ -768,3 +768,57 @@ func testFundingExpiryBlocksOnPending(ht *lntest.HarnessTest) { chanPoint := lntest.ChanPointFromPendingUpdate(update) ht.CloseChannel(alice, chanPoint) } + +// testSimpleTaprootChannelActivation ensures that a simple taproot channel is +// active if the initiator disconnects and reconnects in between channel opening +// and channel confirmation. +func testSimpleTaprootChannelActivation(ht *lntest.HarnessTest) { + simpleTaprootChanArgs := lntest.NodeArgsForCommitType( + lnrpc.CommitmentType_SIMPLE_TAPROOT, + ) + + // Make the new set of participants. + alice := ht.NewNode("alice", simpleTaprootChanArgs) + defer ht.Shutdown(alice) + bob := ht.NewNode("bob", simpleTaprootChanArgs) + defer ht.Shutdown(bob) + + ht.FundCoins(btcutil.SatoshiPerBitcoin, alice) + + // Make sure Alice and Bob are connected. + ht.EnsureConnected(alice, bob) + + // Create simple taproot channel opening parameters. + params := lntest.OpenChannelParams{ + FundMax: true, + CommitmentType: lnrpc.CommitmentType_SIMPLE_TAPROOT, + Private: true, + } + + // Alice opens the channel to Bob. + pendingChan := ht.OpenChannelAssertPending(alice, bob, params) + + // We'll create the channel point to be able to close the channel once + // our test is done. + chanPoint := &lnrpc.ChannelPoint{ + FundingTxid: &lnrpc.ChannelPoint_FundingTxidBytes{ + FundingTxidBytes: pendingChan.Txid, + }, + OutputIndex: pendingChan.OutputIndex, + } + + // We disconnect and reconnect Alice and Bob before the channel is + // confirmed. Our expectation is that the channel is active once the + // channel is confirmed. + ht.DisconnectNodes(alice, bob) + ht.EnsureConnected(alice, bob) + + // Mine six blocks to confirm the channel funding transaction. + ht.MineBlocksAndAssertNumTxes(6, 1) + + // Verify that Alice sees an active channel to Bob. + ht.AssertChannelActive(alice, chanPoint) + + // Our test is done and Alice closes her channel to Bob. + ht.CloseChannel(alice, chanPoint) +} From 8c121ee5e08dfe7c991a142acc1efb9bda75c234 Mon Sep 17 00:00:00 2001 From: Olaoluwa Osuntokun Date: Tue, 24 Oct 2023 18:25:19 -0700 Subject: [PATCH 4/4] docs/release-notes: add entry for nonce init fix --- docs/release-notes/release-notes-0.17.1.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/release-notes/release-notes-0.17.1.md b/docs/release-notes/release-notes-0.17.1.md index 2caf937c6..54238d898 100644 --- a/docs/release-notes/release-notes-0.17.1.md +++ b/docs/release-notes/release-notes-0.17.1.md @@ -23,6 +23,9 @@ bit when sending `update_fail_malformed_htlc`. This avoids a force close with other implementations. +* A bug that would cause taproot channels to sometimes not display as active + [has been fixed](https://github.com/lightningnetwork/lnd/pull/8104). + # New Features ## Functional Enhancements @@ -67,4 +70,5 @@ # Contributors (Alphabetical Order) * Eugene Siegel +* Olaoluwa Osuntokun * Yong Yu