diff --git a/accessman.go b/accessman.go index 586755ade..9667fbc2e 100644 --- a/accessman.go +++ b/accessman.go @@ -93,6 +93,30 @@ func (a *accessMan) assignPeerPerms(remotePub *btcec.PublicKey) ( // Default is restricted unless the below filters say otherwise. access := peerStatusRestricted + // Lock banScoreMtx for reading so that we can update the banning maps + // below. + a.banScoreMtx.RLock() + if count, found := a.peerCounts[peerMapKey]; found { + if count.HasOpenOrClosedChan { + acsmLog.DebugS(ctx, "Peer has open/closed channel, "+ + "assigning protected access") + + access = peerStatusProtected + } else if count.PendingOpenCount != 0 { + acsmLog.DebugS(ctx, "Peer has pending channel(s), "+ + "assigning temporary access") + + 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) @@ -109,41 +133,21 @@ func (a *accessMan) assignPeerPerms(remotePub *btcec.PublicKey) ( 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, "+ - "assigning protected access") - - access = peerStatusProtected - } else if count.PendingOpenCount != 0 { - acsmLog.DebugS(ctx, "Peer has pending channel(s), "+ - "assigning temporary access") - - access = peerStatusTemporary - } - } - // 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 diff --git a/accessman_test.go b/accessman_test.go index ee638cf13..0663a9b4e 100644 --- a/accessman_test.go +++ b/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) + }) + } +} diff --git a/config.go b/config.go index a0bfe02b7..c66c7c919 100644 --- a/config.go +++ b/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" diff --git a/docs/release-notes/release-notes-0.19.1.md b/docs/release-notes/release-notes-0.19.1.md index 656d6b559..3624508cc 100644 --- a/docs/release-notes/release-notes-0.19.1.md +++ b/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 diff --git a/sample-lnd.conf b/sample-lnd.conf index 113a0b460..f4c622ed7 100644 --- a/sample-lnd.conf +++ b/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