multi: send signature announcement once

This change aligns our behavior with latest protocol guidelines. That
is, when we receive a signature announcement when we already have the
full proof we reply with our signature announcement once per
(re)connection.

see: lightning/bolts#1256

Author: gijswijs

discovery/gossiper.go

@@ -3483,12 +3483,22 @@ func (d *AuthenticatedGossiper) handleAnnSig(ctx context.Context,
 	if chanInfo.AuthProof != nil {
 		// If we already have the fully assembled proof, then the peer
 		// sending us their proof has probably not received our local
-		// proof yet. So be kind and send them our proof.
+		// proof yet. So be kind and send them our proof, but only if we
+		// haven't done so since (re)connecting.
 		if nMsg.isRemote {
-			peerID := nMsg.source.SerializeCompressed()
-			log.Debugf("Got AnnounceSignatures for channel with " +
-				"full proof.")
+			// If we already sent our proof to this peer once since
+			// (re)connecting, then we can skip sending it again.
+			if nMsg.peer.HasSentProof(ann.ChannelID) {
+				log.Debugf("Skipping sending announcement " +
+					"signatures since we already did " +
+					"during this connection.")
+
+				nMsg.err <- nil
+
+				return nil, true
+			}
 
+			peerID := nMsg.source.SerializeCompressed()
 			d.wg.Add(1)
 			go func() {
 				defer d.wg.Done()
@@ -3514,7 +3524,6 @@ func (d *AuthenticatedGossiper) handleAnnSig(ctx context.Context,
 					remoteNSB,
 					remoteBSB, nil,
 				)
-
 				if err != nil {
 					log.Errorf("Failed to generate "+
 						"announcement signature: %v",
@@ -3530,6 +3539,8 @@ func (d *AuthenticatedGossiper) handleAnnSig(ctx context.Context,
 					return
 				}
 
+				nMsg.peer.RecordProofSent(ann.ChannelID)
+
 				log.Debugf("Signature announcement sent to "+
 					"peer=%x for chanID=%v", peerID,
 					ann.ChannelID)

discovery/gossiper_test.go

@@ -1774,16 +1774,23 @@ out:
 	}
 }
 
-// TestSignatureAnnouncementFullProofWhenRemoteProof tests that if a remote
+// TestSignatureAnnouncementResendWhenRemoteProof tests that if a remote
 // proof is received when we already have the full proof, the gossiper will send
-// the full proof (ChannelAnnouncement) to the remote peer.
-func TestSignatureAnnouncementFullProofWhenRemoteProof(t *testing.T) {
+// our signature announcement max once per connection to the remote peer.
+func TestSignatureAnnouncementResendWhenRemoteProof(t *testing.T) {
 	t.Parallel()
 	ctx := context.Background()
 
 	tCtx, err := createTestCtx(t, proofMatureDelta, false)
 	require.NoError(t, err, "can't create context")
 
+	// We'll create our test sync manager to have one active syncer.
+	syncMgr := newTestSyncManager(1)
+	syncMgr.Start()
+	defer syncMgr.Stop()
+
+	tCtx.gossiper.syncMgr = syncMgr
+
 	batch, err := tCtx.createLocalAnnouncements(0)
 	require.NoError(t, err, "can't generate announcements")
 
@@ -1797,8 +1804,16 @@ func TestSignatureAnnouncementFullProofWhenRemoteProof(t *testing.T) {
 		remoteKey, sentToPeer, tCtx.gossiper.quit, false,
 	)
 
+	// We create an active syncer for our remote peer.
+	err = syncMgr.InitSyncState(remotePeer)
+	require.NoError(t, err, "failed to init sync state")
+	remoteSyncer := assertSyncerExistence(t, syncMgr, remotePeer)
+	assertTransitionToChansSynced(t, remoteSyncer, remotePeer)
+	assertActiveGossipTimestampRange(t, remotePeer)
+	assertSyncerStatus(t, remoteSyncer, chansSynced, ActiveSync)
+
 	// Override NotifyWhenOnline to return the remote peer which we expect
-	// meesages to be sent to.
+	// messages to be sent to.
 	tCtx.gossiper.reliableSender.cfg.NotifyWhenOnline = func(_ [33]byte,
 		peerChan chan<- lnpeer.Peer) {
 
@@ -1940,7 +1955,64 @@ func TestSignatureAnnouncementFullProofWhenRemoteProof(t *testing.T) {
 	}
 
 	// Now give the gossiper the remote proof yet again. This should
-	// trigger a send of the signature announcement.
+	// trigger a send of our signature announcement.
+	select {
+	case err = <-tCtx.gossiper.ProcessRemoteAnnouncement(
+		ctx, batch.remoteProofAnn, remotePeer,
+	):
+	case <-time.After(2 * time.Second):
+		t.Fatal("did not process local announcement")
+	}
+	require.NoError(t, err, "unable to process remote proof")
+
+	// We expect the gossiper to send this message to the remote peer.
+	select {
+	case msg := <-sentToPeer:
+		_, ok := msg.(*lnwire.AnnounceSignatures1)
+		if !ok {
+			t.Fatalf("expected AnnounceSignatures1, instead got "+
+				"%T", msg)
+		}
+	case <-time.After(2 * time.Second):
+		t.Fatal("did not send local proof to peer")
+	}
+
+	// Now give the gossiper the remote proof a 2nd time. This should _not_
+	// trigger a send of our signature announcement, since we already sent
+	// it once and we only send it once per connection.
+	select {
+	case err = <-tCtx.gossiper.ProcessRemoteAnnouncement(
+		ctx, batch.remoteProofAnn, remotePeer,
+	):
+	case <-time.After(2 * time.Second):
+		t.Fatal("did not process local announcement")
+	}
+	require.NoError(t, err, "unable to process remote proof")
+
+	// We expect the gossiper to _not_ send this message to the remote peer.
+	select {
+	case msg := <-sentToPeer:
+		_, ok := msg.(*lnwire.AnnounceSignatures1)
+		if ok {
+			t.Fatalf("got an AnnounceSignatures1 when none was "+
+				"expected %T", msg)
+		}
+	case <-time.After(2 * time.Second):
+		break
+	}
+
+	// We simulate the remote peer disconnecting and reconnecting by
+	// creating a new mock peer. This will reset the connection-specific
+	// state, such as whether we've already sent the proof to this peer.
+	remotePeer = newMockPeer(
+		remoteKey, sentToPeer, tCtx.gossiper.quit, false,
+	)
+
+	// Now give the gossiper the remote proof a 3rd time. This should
+	// trigger a send of our signature announcement, since the syncer
+	// was pruned and we now have a new syncer for the remote peer.
+	// This is to simulate the case where the remote peer disconnects and
+	// reconnects, and we have to send the signature announcement again.
 	select {
 	case err = <-tCtx.gossiper.ProcessRemoteAnnouncement(
 		ctx, batch.remoteProofAnn, remotePeer,

discovery/mock_test.go

@@ -8,6 +8,7 @@ import (
 
 	"github.com/btcsuite/btcd/btcec/v2"
 	"github.com/btcsuite/btcd/wire"
+	"github.com/lightningnetwork/lnd/fn/v2"
 	"github.com/lightningnetwork/lnd/lnpeer"
 	"github.com/lightningnetwork/lnd/lnwire"
 )
@@ -19,6 +20,11 @@ type mockPeer struct {
 	sentMsgs     chan lnwire.Message
 	quit         chan struct{}
 	disconnected atomic.Bool
+
+	// The following fields are used to mock the announcement proof-related
+	// methods.
+	proofsSentMtx   sync.Mutex
+	proofSentToChan fn.Set[lnwire.ChannelID]
 }
 
 var _ lnpeer.Peer = (*mockPeer)(nil)
@@ -27,9 +33,10 @@ func newMockPeer(pk *btcec.PublicKey, sentMsgs chan lnwire.Message,
 	quit chan struct{}, disconnected bool) *mockPeer {
 
 	p := &mockPeer{
-		pk:       pk,
-		sentMsgs: sentMsgs,
-		quit:     quit,
+		pk:              pk,
+		sentMsgs:        sentMsgs,
+		quit:            quit,
+		proofSentToChan: fn.NewSet[lnwire.ChannelID](),
 	}
 	p.disconnected.Store(disconnected)
 
@@ -93,6 +100,29 @@ func (p *mockPeer) Disconnect(err error) {
 	p.disconnected.Store(true)
 }
 
+func (p *mockPeer) RecordProofSent(chanID lnwire.ChannelID) {
+	p.proofsSentMtx.Lock()
+	defer p.proofsSentMtx.Unlock()
+
+	if p.proofSentToChan == nil {
+		p.proofSentToChan = make(map[lnwire.ChannelID]struct{})
+	}
+	p.proofSentToChan[chanID] = struct{}{}
+}
+
+func (p *mockPeer) HasSentProof(chanID lnwire.ChannelID) bool {
+	p.proofsSentMtx.Lock()
+	defer p.proofsSentMtx.Unlock()
+
+	if p.proofSentToChan == nil {
+		return false
+	}
+
+	_, ok := p.proofSentToChan[chanID]
+
+	return ok
+}
+
 // mockMessageStore is an in-memory implementation of the MessageStore interface
 // used for the gossiper's unit tests.
 type mockMessageStore struct {

funding/manager_test.go

@@ -370,6 +370,12 @@ func (n *testNode) RemovePendingChannel(_ lnwire.ChannelID) error {
 	return nil
 }
 
+func (n *testNode) RecordProofSent(chanID lnwire.ChannelID) {}
+
+func (n *testNode) HasSentProof(chanID lnwire.ChannelID) bool {
+	return false
+}
+
 func createTestWallet(cdb *channeldb.ChannelStateDB, netParams *chaincfg.Params,
 	notifier chainntnfs.ChainNotifier, wc lnwallet.WalletController,
 	signer input.Signer, keyRing keychain.SecretKeyRing,

htlcswitch/link_test.go

@@ -2128,6 +2128,12 @@ func (m *mockPeer) RemovePendingChannel(_ lnwire.ChannelID) error {
 	return nil
 }
 
+func (p *mockPeer) RecordProofSent(chanID lnwire.ChannelID) {}
+
+func (p *mockPeer) HasSentProof(chanID lnwire.ChannelID) bool {
+	return false
+}
+
 type singleLinkTestHarness struct {
 	aliceSwitch      *Switch
 	aliceLink        ChannelLink

htlcswitch/mock.go

@@ -321,6 +321,12 @@ func (s *mockServer) QuitSignal() <-chan struct{} {
 	return s.quit
 }
 
+func (s *mockServer) RecordProofSent(chanID lnwire.ChannelID) {}
+
+func (s *mockServer) HasSentProof(chanID lnwire.ChannelID) bool {
+	return false
+}
+
 // mockHopIterator represents the test version of hop iterator which instead
 // of encrypting the path in onion blob just stores the path as a list of hops.
 type mockHopIterator struct {

lnpeer/mock_peer.go

@@ -81,3 +81,9 @@ func (m *MockPeer) RemoteFeatures() *lnwire.FeatureVector {
 }
 
 func (m *MockPeer) Disconnect(err error) {}
+
+func (s *MockPeer) RecordProofSent(chanID lnwire.ChannelID) {}
+
+func (s *MockPeer) HasSentProof(chanID lnwire.ChannelID) bool {
+	return false
+}

lnpeer/peer.go

@@ -49,6 +49,14 @@ type Peer interface {
 	// point from all indexes associated with the peer.
 	WipeChannel(*wire.OutPoint)
 
+	// RecordProofSent should be called when a proof has been sent for a
+	// given channel ID.
+	RecordProofSent(lnwire.ChannelID)
+
+	// HasSentProof should be called to check whether a proof has been sent
+	// for a given channel ID.
+	HasSentProof(lnwire.ChannelID) bool
+
 	// PubKey returns the serialized public key of the remote peer.
 	PubKey() [33]byte
 

peer/brontide.go

@@ -606,6 +606,16 @@ type Brontide struct {
 	// peer's chansync message with its own over and over again.
 	resentChanSyncMsg map[lnwire.ChannelID]struct{}
 
+	// proofsSentMtx is a mutex that protects the proofSentToChan set.
+	proofsSentMtx sync.Mutex
+
+	// proofSentToChan is used when we already have the fully assembled
+	// proof for a channel and the peer sending us their proof has probably
+	// not received our local proof yet. So we are kind and send them our
+	// proof, but only if we haven't done so since (re)connecting. We keep
+	// track of sends with this map, so we don't send it twice.
+	proofSentToChan fn.Set[lnwire.ChannelID]
+
 	// channelEventClient is the channel event subscription client that's
 	// used to assist retry enabling the channels. This client is only
 	// created when the reenableTimeout is no greater than 1 minute. Once
@@ -675,6 +685,7 @@ func NewBrontide(cfg Config) *Brontide {
 		linkFailures:       make(chan linkFailureReport),
 		chanCloseMsgs:      make(chan *closeMsg),
 		resentChanSyncMsg:  make(map[lnwire.ChannelID]struct{}),
+		proofSentToChan:    fn.NewSet[lnwire.ChannelID](),
 		startReady:         make(chan struct{}),
 		log:                peerLog.WithPrefix(logPrefix),
 		msgRouter:          msgRouter,
@@ -4498,6 +4509,26 @@ func (p *Brontide) WipeChannel(chanPoint *wire.OutPoint) {
 	p.cfg.Switch.RemoveLink(chanID)
 }
 
+// RecordProofSent should be called when a proof has been sent for a given
+// channel ID.
+//
+// NOTE: Part of the lnpeer.Peer interface.
+func (p *Brontide) RecordProofSent(chanID lnwire.ChannelID) {
+	p.proofsSentMtx.Lock()
+	defer p.proofsSentMtx.Unlock()
+	p.proofSentToChan.Add(chanID)
+}
+
+// HasSentProof should be called to check whether a proof has been sent for a
+// given channel ID.
+//
+// NOTE: Part of the lnpeer.Peer interface.
+func (p *Brontide) HasSentProof(chanID lnwire.ChannelID) bool {
+	p.proofsSentMtx.Lock()
+	defer p.proofsSentMtx.Unlock()
+	return p.proofSentToChan.Contains(chanID)
+}
+
 // handleInitMsg handles the incoming init message which contains global and
 // local feature vectors. If feature vectors are incompatible then disconnect.
 func (p *Brontide) handleInitMsg(msg *lnwire.Init) error {