Merge pull request #8104 from Roasbeef/taproot-chans-nonce-double-init

multi: skip InitRemoteMusigNonces if we've already called it

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

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,

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)
+}

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",

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
 }
 

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)