From da13c7ab66e26fa74f10c81d3c85116535eb2db3 Mon Sep 17 00:00:00 2001 From: yyforyongyu Date: Thu, 19 Jun 2025 17:35:58 +0800 Subject: [PATCH] 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`. --- accessman.go | 40 ++++++++++++----- accessman_test.go | 108 ++++++++++++++++++++++++++++++++++++++++++++++ server.go | 3 +- 3 files changed, 140 insertions(+), 11 deletions(-) diff --git a/accessman.go b/accessman.go index 80a629569..66ec71646 100644 --- a/accessman.go +++ b/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, peerMapKey) + 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 + } + + // Given this peer has no channels with us, we can now remove it. + delete(a.peerChanInfo, peerPubKey) + acsmLog.TraceS(ctx, "Removed peer from peerChanInfo:") } diff --git a/accessman_test.go b/accessman_test.go index 6409d0806..30a315eec 100644 --- a/accessman_test.go +++ b/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) +} diff --git a/server.go b/server.go index 8eb5c374a..657942dd9 100644 --- a/server.go +++ b/server.go @@ -4956,7 +4956,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.