From dc42b160a03650f2fb45ef2c65081ce21f11ed2c Mon Sep 17 00:00:00 2001
From: Eugene Siegel <elzeigel@gmail.com>
Date: Fri, 13 Oct 2023 08:00:49 -0700
Subject: [PATCH] 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)