lnd: remove peer from peerChanInfo when necessary

We now remove the peer from `peerChanInfo` if this peer doesn't have
channels with us. Also patched a unit test for `removePeerAccess`.

Author: yyforyongyu

accessman.go

@@ -611,18 +611,14 @@ func (a *accessMan) addPeerAccess(remotePub *btcec.PublicKey,
 
 // removePeerAccess removes the peer's access from the maps. This should be
 // called when the peer has been disconnected.
-func (a *accessMan) removePeerAccess(remotePub *btcec.PublicKey) {
-	ctx := btclog.WithCtx(
-		context.TODO(), lnutils.LogPubKey("peer", remotePub),
-	)
-	acsmLog.DebugS(ctx, "Removing peer access")
+func (a *accessMan) removePeerAccess(peerPubKey string) {
+	ctx := btclog.WithCtx(context.TODO(), "peer", peerPubKey)
+	acsmLog.DebugS(ctx, "Removing access:")
 
 	a.banScoreMtx.Lock()
 	defer a.banScoreMtx.Unlock()
 
-	peerMapKey := string(remotePub.SerializeCompressed())
-
-	status, found := a.peerScores[peerMapKey]
+	status, found := a.peerScores[peerPubKey]
 	if !found {
 		acsmLog.InfoS(ctx, "Peer score not found during removal")
 		return
@@ -639,7 +635,31 @@ func (a *accessMan) removePeerAccess(remotePub *btcec.PublicKey) {
 			"new_restricted", a.numRestricted)
 	}
 
-	acsmLog.TraceS(ctx, "Deleting peer from peerScores")
+	acsmLog.TraceS(ctx, "Deleting from peerScores:")
+
+	delete(a.peerScores, peerPubKey)
+
+	// We now check whether this peer has channels with us or not.
+	info, found := a.peerChanInfo[peerPubKey]
+	if !found {
+		acsmLog.DebugS(ctx, "Chan info not found during removal:")
+		return
+	}
+
+	// Exit early if the peer has channel(s) with us.
+	if info.HasOpenOrClosedChan {
+		acsmLog.DebugS(ctx, "Skipped removing peer with channels:")
+		return
+	}
+
+	// Skip removing the peer if it has pending open/close with us.
+	if info.PendingOpenCount != 0 {
+		acsmLog.DebugS(ctx, "Skipped removing peer with pending "+
+			"channels:")
+		return
+	}
 
-	delete(a.peerScores, peerMapKey)
+	// Given this peer has no channels with us, we can now remove it.
+	delete(a.peerChanInfo, peerPubKey)
+	acsmLog.TraceS(ctx, "Removed peer from peerChanInfo:")
 }

accessman_test.go

@@ -703,3 +703,111 @@ func TestAddPeerAccessOutbound(t *testing.T) {
 	expecedScore = peerSlotStatus{state: peerStatusTemporary}
 	require.Equal(t, expecedScore, score)
 }
+
+// TestRemovePeerAccess asserts `removePeerAccess` correctly update the
+// accessman's internal state based on the peer's status.
+func TestRemovePeerAccess(t *testing.T) {
+	t.Parallel()
+
+	// Create a testing accessMan.
+	a := &accessMan{
+		peerChanInfo: make(map[string]channeldb.ChanCount),
+		peerScores:   make(map[string]peerSlotStatus),
+	}
+
+	// numRestrictedExpected specifies the final value to expect once the
+	// test finishes.
+	var numRestrictedExpected int
+
+	// peer1 exists with an open channel, which should not be removed. Since
+	// it has protected status, the numRestricted should stay unchanged.
+	peer1 := "peer1"
+	a.peerChanInfo[peer1] = channeldb.ChanCount{
+		HasOpenOrClosedChan: true,
+	}
+	peer1Access := peerStatusProtected
+	a.peerScores[peer1] = peerSlotStatus{state: peer1Access}
+
+	// peer2 exists with a pending channel, which should not be removed.
+	// Since it has temporary status, the numRestricted should stay
+	// unchanged.
+	peer2 := "peer2"
+	a.peerChanInfo[peer2] = channeldb.ChanCount{
+		PendingOpenCount: 1,
+	}
+	peer2Access := peerStatusTemporary
+	a.peerScores[peer2] = peerSlotStatus{state: peer2Access}
+
+	// peer3 exists without any channels, which will be removed. Since it
+	// has restricted status, the numRestricted should be decremented.
+	peer3 := "peer3"
+	a.peerChanInfo[peer3] = channeldb.ChanCount{}
+	peer3Access := peerStatusRestricted
+	a.peerScores[peer3] = peerSlotStatus{state: peer3Access}
+	numRestrictedExpected--
+
+	// peer4 exists with a score and a temporary status, which will be
+	// removed.
+	peer4 := "peer4"
+	peer4Access := peerStatusTemporary
+	a.peerScores[peer4] = peerSlotStatus{state: peer4Access}
+
+	// peer5 doesn't exist, removing it will be a NOOP.
+	peer5 := "peer5"
+
+	// We now assert `removePeerAccess` behaves as expected.
+	//
+	// Remove peer1 should change nothing.
+	a.removePeerAccess(peer1)
+
+	// peer1 should be removed from peerScores but not peerChanInfo.
+	_, found := a.peerScores[peer1]
+	require.False(t, found)
+	_, found = a.peerChanInfo[peer1]
+	require.True(t, found)
+
+	// Remove peer2 should change nothing.
+	a.removePeerAccess(peer2)
+
+	// peer2 should be removed from peerScores but not peerChanInfo.
+	_, found = a.peerScores[peer2]
+	require.False(t, found)
+	_, found = a.peerChanInfo[peer2]
+	require.True(t, found)
+
+	// Remove peer3 should remove it from the maps.
+	a.removePeerAccess(peer3)
+
+	// peer3 should be removed from peerScores and peerChanInfo.
+	_, found = a.peerScores[peer3]
+	require.False(t, found)
+	_, found = a.peerChanInfo[peer3]
+	require.False(t, found)
+
+	// Remove peer4 should remove it from the maps.
+	a.removePeerAccess(peer4)
+
+	// peer4 should be removed from peerScores and NOT be found in
+	// peerChanInfo.
+	_, found = a.peerScores[peer4]
+	require.False(t, found)
+	_, found = a.peerChanInfo[peer4]
+	require.False(t, found)
+
+	// Remove peer5 should be NOOP.
+	a.removePeerAccess(peer5)
+
+	// peer5 should NOT be found.
+	_, found = a.peerScores[peer5]
+	require.False(t, found)
+	_, found = a.peerChanInfo[peer5]
+	require.False(t, found)
+
+	// Finally, assert the numRestricted is decremented as expected. Given
+	// we only have peer3 which has restricted access, it should decrement
+	// once.
+	//
+	// NOTE: The value is actually negative here, which is allowed in
+	// accessman.
+	require.EqualValues(t, numRestrictedExpected, a.numRestricted)
+}

server.go

@@ -4964,7 +4964,8 @@ func (s *server) removePeer(p *peer.Brontide) {
 	}
 
 	// Remove the peer's access permission from the access manager.
-	s.peerAccessMan.removePeerAccess(p.IdentityKey())
+	peerPubStr := string(p.IdentityKey().SerializeCompressed())
+	s.peerAccessMan.removePeerAccess(peerPubStr)
 
 	// Copy the peer's error buffer across to the server if it has any items
 	// in it so that we can restore peer errors across connections.