Merge pull request #9876 from yyforyongyu/fix-accessman

accessman: remove restrictions on protected/temporary peers

Author: guggero

accessman.go

@@ -93,27 +93,9 @@ func (a *accessMan) assignPeerPerms(remotePub *btcec.PublicKey) (
 	// Default is restricted unless the below filters say otherwise.
 	access := peerStatusRestricted
 
-	shouldDisconnect, err := a.cfg.shouldDisconnect(remotePub)
-	if err != nil {
-		acsmLog.ErrorS(ctx, "Error checking disconnect status", err)
-
-		// Access is restricted here.
-		return access, err
-	}
-
-	if shouldDisconnect {
-		acsmLog.WarnS(ctx, "Peer is banned, assigning restricted access",
-			ErrGossiperBan)
-
-		// Access is restricted here.
-		return access, ErrGossiperBan
-	}
-
 	// Lock banScoreMtx for reading so that we can update the banning maps
 	// below.
 	a.banScoreMtx.RLock()
-	defer a.banScoreMtx.RUnlock()
-
 	if count, found := a.peerCounts[peerMapKey]; found {
 		if count.HasOpenOrClosedChan {
 			acsmLog.DebugS(ctx, "Peer has open/closed channel, "+
@@ -127,23 +109,45 @@ func (a *accessMan) assignPeerPerms(remotePub *btcec.PublicKey) (
 			access = peerStatusTemporary
 		}
 	}
+	a.banScoreMtx.RUnlock()
+
+	// Exit early if the peer status is no longer restricted.
+	if access != peerStatusRestricted {
+		return access, nil
+	}
+
+	// Check whether this peer is banned.
+	shouldDisconnect, err := a.cfg.shouldDisconnect(remotePub)
+	if err != nil {
+		acsmLog.ErrorS(ctx, "Error checking disconnect status", err)
+
+		// Access is restricted here.
+		return access, err
+	}
+
+	if shouldDisconnect {
+		acsmLog.WarnS(ctx, "Peer is banned, assigning restricted access",
+			ErrGossiperBan)
+
+		// Access is restricted here.
+		return access, ErrGossiperBan
+	}
 
 	// If we've reached this point and access hasn't changed from
 	// restricted, then we need to check if we even have a slot for this
 	// peer.
-	if access == peerStatusRestricted {
-		acsmLog.DebugS(ctx, "Peer has no channels, assigning "+
-			"restricted access")
+	acsmLog.DebugS(ctx, "Peer has no channels, assigning restricted access")
 
-		if a.numRestricted >= a.cfg.maxRestrictedSlots {
-			acsmLog.WarnS(ctx, "No more restricted slots "+
-				"available, denying peer",
-				ErrNoMoreRestrictedAccessSlots,
-				"num_restricted", a.numRestricted,
-				"max_restricted", a.cfg.maxRestrictedSlots)
+	a.banScoreMtx.RLock()
+	defer a.banScoreMtx.RUnlock()
 
-			return access, ErrNoMoreRestrictedAccessSlots
-		}
+	if a.numRestricted >= a.cfg.maxRestrictedSlots {
+		acsmLog.WarnS(ctx, "No more restricted slots available, "+
+			"denying peer", ErrNoMoreRestrictedAccessSlots,
+			"num_restricted", a.numRestricted, "max_restricted",
+			a.cfg.maxRestrictedSlots)
+
+		return access, ErrNoMoreRestrictedAccessSlots
 	}
 
 	return access, nil

accessman_test.go

@@ -151,3 +151,246 @@ func TestAccessManRestrictedSlots(t *testing.T) {
 	err = a.newPendingCloseChan(peerKey4)
 	require.ErrorIs(t, err, ErrNoMoreRestrictedAccessSlots)
 }
+
+// TestAssignPeerPerms asserts that the peer's access status is correctly
+// assigned.
+func TestAssignPeerPerms(t *testing.T) {
+	t.Parallel()
+
+	// genPeerPub is a helper closure that generates a random public key.
+	genPeerPub := func() *btcec.PublicKey {
+		peerPriv, err := btcec.NewPrivateKey()
+		require.NoError(t, err)
+
+		return peerPriv.PubKey()
+	}
+
+	disconnect := func(_ *btcec.PublicKey) (bool, error) {
+		return true, nil
+	}
+
+	noDisconnect := func(_ *btcec.PublicKey) (bool, error) {
+		return false, nil
+	}
+
+	var testCases = []struct {
+		name             string
+		peerPub          *btcec.PublicKey
+		chanCount        channeldb.ChanCount
+		shouldDisconnect func(*btcec.PublicKey) (bool, error)
+		numRestricted    int
+
+		expectedStatus peerAccessStatus
+		expectedErr    error
+	}{
+		// peer1 has a channel with us, and we expect it to have a
+		// protected status.
+		{
+			name:    "peer with channels",
+			peerPub: genPeerPub(),
+			chanCount: channeldb.ChanCount{
+				HasOpenOrClosedChan: true,
+			},
+			shouldDisconnect: noDisconnect,
+			expectedStatus:   peerStatusProtected,
+			expectedErr:      nil,
+		},
+		// peer2 has a channel open and a pending channel with us, we
+		// expect it to have a protected status.
+		{
+			name:    "peer with channels and pending channels",
+			peerPub: genPeerPub(),
+			chanCount: channeldb.ChanCount{
+				HasOpenOrClosedChan: true,
+				PendingOpenCount:    1,
+			},
+			shouldDisconnect: noDisconnect,
+			expectedStatus:   peerStatusProtected,
+			expectedErr:      nil,
+		},
+		// peer3 has a pending channel with us, and we expect it to have
+		// a temporary status.
+		{
+			name:    "peer with pending channels",
+			peerPub: genPeerPub(),
+			chanCount: channeldb.ChanCount{
+				HasOpenOrClosedChan: false,
+				PendingOpenCount:    1,
+			},
+			shouldDisconnect: noDisconnect,
+			expectedStatus:   peerStatusTemporary,
+			expectedErr:      nil,
+		},
+		// peer4 has no channel with us, and we expect it to have a
+		// restricted status.
+		{
+			name:    "peer with no channels",
+			peerPub: genPeerPub(),
+			chanCount: channeldb.ChanCount{
+				HasOpenOrClosedChan: false,
+				PendingOpenCount:    0,
+			},
+			shouldDisconnect: noDisconnect,
+			expectedStatus:   peerStatusRestricted,
+			expectedErr:      nil,
+		},
+		// peer5 has no channel with us, and we expect it to have a
+		// restricted status. We also expect the error `ErrGossiperBan`
+		// to be returned given we will use a mocked `shouldDisconnect`
+		// in this test to disconnect on peer5 only.
+		{
+			name:    "peer with no channels and banned",
+			peerPub: genPeerPub(),
+			chanCount: channeldb.ChanCount{
+				HasOpenOrClosedChan: false,
+				PendingOpenCount:    0,
+			},
+			shouldDisconnect: disconnect,
+			expectedStatus:   peerStatusRestricted,
+			expectedErr:      ErrGossiperBan,
+		},
+		// peer6 has no channel with us, and we expect it to have a
+		// restricted status. We also expect the error
+		// `ErrNoMoreRestrictedAccessSlots` to be returned given
+		// we only allow 1 restricted peer in this test.
+		{
+			name:    "peer with no channels and restricted",
+			peerPub: genPeerPub(),
+			chanCount: channeldb.ChanCount{
+				HasOpenOrClosedChan: false,
+				PendingOpenCount:    0,
+			},
+			shouldDisconnect: noDisconnect,
+			numRestricted:    1,
+
+			expectedStatus: peerStatusRestricted,
+			expectedErr:    ErrNoMoreRestrictedAccessSlots,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			peerStr := string(tc.peerPub.SerializeCompressed())
+
+			initPerms := func() (map[string]channeldb.ChanCount,
+				error) {
+
+				return map[string]channeldb.ChanCount{
+					peerStr: tc.chanCount,
+				}, nil
+			}
+
+			cfg := &accessManConfig{
+				initAccessPerms:    initPerms,
+				shouldDisconnect:   tc.shouldDisconnect,
+				maxRestrictedSlots: 1,
+			}
+
+			a, err := newAccessMan(cfg)
+			require.NoError(t, err)
+
+			// Initialize the internal state of the accessman.
+			a.numRestricted = int64(tc.numRestricted)
+
+			status, err := a.assignPeerPerms(tc.peerPub)
+			require.Equal(t, tc.expectedStatus, status)
+			require.ErrorIs(t, tc.expectedErr, err)
+		})
+	}
+}
+
+// TestAssignPeerPermsBypassRestriction asserts that when a peer has a channel
+// with us, either it being open, pending, or closed, no restriction is placed
+// on this peer.
+func TestAssignPeerPermsBypassRestriction(t *testing.T) {
+	t.Parallel()
+
+	// genPeerPub is a helper closure that generates a random public key.
+	genPeerPub := func() *btcec.PublicKey {
+		peerPriv, err := btcec.NewPrivateKey()
+		require.NoError(t, err)
+
+		return peerPriv.PubKey()
+	}
+
+	// Mock shouldDisconnect to always return true and assert that it has no
+	// effect on the peer.
+	disconnect := func(_ *btcec.PublicKey) (bool, error) {
+		return true, nil
+	}
+
+	var testCases = []struct {
+		name           string
+		peerPub        *btcec.PublicKey
+		chanCount      channeldb.ChanCount
+		expectedStatus peerAccessStatus
+	}{
+		// peer1 has a channel with us, and we expect it to have a
+		// protected status.
+		{
+			name:    "peer with channels",
+			peerPub: genPeerPub(),
+			chanCount: channeldb.ChanCount{
+				HasOpenOrClosedChan: true,
+			},
+			expectedStatus: peerStatusProtected,
+		},
+		// peer2 has a channel open and a pending channel with us, we
+		// expect it to have a protected status.
+		{
+			name:    "peer with channels and pending channels",
+			peerPub: genPeerPub(),
+			chanCount: channeldb.ChanCount{
+				HasOpenOrClosedChan: true,
+				PendingOpenCount:    1,
+			},
+			expectedStatus: peerStatusProtected,
+		},
+		// peer3 has a pending channel with us, and we expect it to have
+		// a temporary status.
+		{
+			name:    "peer with pending channels",
+			peerPub: genPeerPub(),
+			chanCount: channeldb.ChanCount{
+				HasOpenOrClosedChan: false,
+				PendingOpenCount:    1,
+			},
+			expectedStatus: peerStatusTemporary,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			peerStr := string(tc.peerPub.SerializeCompressed())
+
+			initPerms := func() (map[string]channeldb.ChanCount,
+				error) {
+
+				return map[string]channeldb.ChanCount{
+					peerStr: tc.chanCount,
+				}, nil
+			}
+
+			// Config the accessman such that it has zero max slots
+			// and always return true on `shouldDisconnect`. We
+			// should see the peers in this test are not affected by
+			// these checks.
+			cfg := &accessManConfig{
+				initAccessPerms:    initPerms,
+				shouldDisconnect:   disconnect,
+				maxRestrictedSlots: 0,
+			}
+
+			a, err := newAccessMan(cfg)
+			require.NoError(t, err)
+
+			status, err := a.assignPeerPerms(tc.peerPub)
+			require.NoError(t, err)
+			require.Equal(t, tc.expectedStatus, status)
+		})
+	}
+}

config.go

@@ -240,7 +240,7 @@ const (
 
 	// DefaultNumRestrictedSlots is the default number of restricted slots
 	// we'll allocate in the server.
-	DefaultNumRestrictedSlots = 30
+	DefaultNumRestrictedSlots = 100
 
 	// BitcoinChainName is a string that represents the Bitcoin blockchain.
 	BitcoinChainName = "bitcoin"

docs/release-notes/release-notes-0.19.1.md

@@ -34,6 +34,10 @@
 - Fixed [a case](https://github.com/lightningnetwork/lnd/pull/9890) where LND
   would crash because of misaligned of a data struct on 32 bit systems.
 
+- Fixed [a case](https://github.com/lightningnetwork/lnd/pull/9876) where a peer
+  may be treated as restricted peer although it used to have a channel with the
+  node.
+
 # New Features
 
 ## Functional Enhancements

sample-lnd.conf

@@ -567,7 +567,7 @@
 ; http-header-timeout=5s
 
 ; The number of restricted slots the server will allocate for peers.
-; num-restricted-slots=30
+; num-restricted-slots=100
 
 ; If true, a peer will *not* be disconnected if a pong is not received in time
 ; or is mismatched. Defaults to false, meaning peers *will* be disconnected on