Merge pull request #1215 from bitromortac/2601-closed-channels-restriction

rules: allow non-present channels in restriction

docs/release-notes/release-notes-0.16.1.md

@@ -16,6 +16,11 @@
 
 ### Bug Fixes
 
+* [Improve channel restriction
+  resilience](https://github.com/lightninglabs/lightning-terminal/pull/1215):
+  The channel restriction rule no longer fails to initialize when restricted
+  channels are closed.
+
 ### Functional Changes/Additions
 
 ### Technical and Architectural Updates
@@ -37,4 +42,6 @@
 
 ### Taproot Assets
 
-# Contributors (Alphabetical Order)
+# Contributors (Alphabetical Order)
+
+* bitromortac

rules/channel_restrictions.go

@@ -38,7 +38,14 @@ type ChannelRestrictMgr struct {
 	// chanPointToID is a map from channel point to channel ID's for our
 	// known set of channels.
 	chanPointToID map[string]uint64
-	mu            sync.Mutex
+
+	// checkedIDs tracks channel IDs that we have already attempted to find
+	// in LND's list of open channels but were not present.
+	checkedIDs map[uint64]bool
+
+	// mu is a mutex used to protect the maps and other state in the manager
+	// from concurrent access.
+	mu sync.Mutex
 }
 
 // NewChannelRestrictMgr constructs a new instance of a ChannelRestrictMgr.
@@ -46,6 +53,7 @@ func NewChannelRestrictMgr() *ChannelRestrictMgr {
 	return &ChannelRestrictMgr{
 		chanIDToPoint: make(map[uint64]string),
 		chanPointToID: make(map[string]uint64),
+		checkedIDs:    make(map[uint64]bool),
 	}
 }
 
@@ -72,10 +80,13 @@ func (c *ChannelRestrictMgr) NewEnforcer(ctx context.Context, cfg Config,
 	chanMap := make(map[uint64]bool, len(channels.DenyList))
 	for _, chanID := range channels.DenyList {
 		chanMap[chanID] = true
-		err := c.maybeUpdateChannelMaps(ctx, cfg, chanID)
-		if err != nil {
-			return nil, err
-		}
+	}
+
+	// We'll attempt to update our internal channel maps for any IDs in our
+	// deny list that we don't already know about and haven't checked yet.
+	err := c.maybeUpdateChannelMaps(ctx, cfg, channels.DenyList)
+	if err != nil {
+		return nil, err
 	}
 
 	return &ChannelRestrictEnforcer{
@@ -118,17 +129,26 @@ func (c *ChannelRestrictMgr) EmptyValue() Values {
 }
 
 // maybeUpdateChannelMaps updates the ChannelRestrictMgrs set of known channels
-// iff the channel given by the caller is not found in the current map set.
+// iff any of the channels given by the caller are not found in the current
+// map set and have not been checked previously.
 func (c *ChannelRestrictMgr) maybeUpdateChannelMaps(ctx context.Context,
-	cfg Config, chanID uint64) error {
+	cfg Config, chanIDs []uint64) error {
 
 	c.mu.Lock()
 	defer c.mu.Unlock()
 
-	// If we already know of this channel, we don't need to go update our
-	// maps.
-	_, ok := c.chanIDToPoint[chanID]
-	if ok {
+	var needsSync bool
+	for _, id := range chanIDs {
+		_, known := c.chanIDToPoint[id]
+		if !known && !c.checkedIDs[id] {
+			needsSync = true
+			break
+		}
+	}
+
+	// If we already know about all these channels or have checked them,
+	// then we don't need to do anything.
+	if !needsSync {
 		return nil
 	}
 
@@ -139,39 +159,59 @@ func (c *ChannelRestrictMgr) maybeUpdateChannelMaps(ctx context.Context,
 		return err
 	}
 
-	var (
-		found bool
-		point string
-		id    uint64
-	)
-
-	// Update our set of maps and also make sure that the channel specified
-	// by the caller is valid given our set of open channels.
+	// Update our set of maps with all currently open channels.
 	for _, channel := range chans {
-		point = channel.ChannelPoint
-		id = channel.ChannelID
-
-		c.chanPointToID[point] = id
-		c.chanIDToPoint[id] = point
-
-		if id == chanID {
-			found = true
-		}
+		c.chanPointToID[channel.ChannelPoint] = channel.ChannelID
+		c.chanIDToPoint[channel.ChannelID] = channel.ChannelPoint
 	}
 
-	if !found {
-		return fmt.Errorf("invalid channel ID")
+	// For every ID we were looking for, if it's still not in our known
+	// maps, we mark it as checked so we don't trigger another sync for it.
+	for _, id := range chanIDs {
+		if _, ok := c.chanIDToPoint[id]; !ok {
+			c.checkedIDs[id] = true
+		}
 	}
 
 	return nil
 }
 
-func (c *ChannelRestrictMgr) getChannelID(point string) (uint64, bool) {
+// getChannelIDWithRetryCheck performs an atomic lookup of a channel point
+// while also determining whether the caller should retry their request to allow
+// for channel mapping resynchronization. When a channel point is not found in
+// our known mappings, we must decide whether to allow the operation or request
+// a retry. If any entries in the deny list remain unmapped to channel points,
+// we cannot be certain whether the unknown channel is restricted, so we signal
+// for a retry to trigger a fresh synchronization with the node's current
+// channel state.
+func (c *ChannelRestrictMgr) getChannelIDWithRetryCheck(point string,
+	denyList []uint64) (id uint64, found bool, shouldRetry bool) {
+
 	c.mu.Lock()
 	defer c.mu.Unlock()
 
-	id, ok := c.chanPointToID[point]
-	return id, ok
+	// First, check if we know this channel point.
+	id, found = c.chanPointToID[point]
+	if found {
+		return id, true, false
+	}
+
+	// Channel not found. Check if we have any unmapped deny list entries.
+	// If we do, we can't be sure whether this unknown channel is
+	// restricted or not, so we must trigger a retry to refresh our
+	// mappings.
+	for _, restrictedID := range denyList {
+		if _, mapped := c.chanIDToPoint[restrictedID]; !mapped {
+			// We have unmapped restrictions - clear the negative
+			// cache to force a fresh sync on retry.
+			c.checkedIDs = make(map[uint64]bool)
+			return 0, false, true
+		}
+	}
+
+	// Channel not found, but all deny list entries are mapped, so this
+	// unknown channel is definitely not in our restriction list.
+	return 0, false, false
 }
 
 // ChannelRestrictEnforcer enforces requests and responses against a
@@ -278,8 +318,23 @@ func (c *ChannelRestrictEnforcer) checkers() map[string]mid.RoundTripChecker {
 					"%s:%d", txid.String(), index,
 				)
 
-				id, ok := c.mgr.getChannelID(point)
-				if !ok {
+				// Atomically check if we know this channel and
+				// whether we need to retry.
+				id, found, shouldRetry := c.mgr.
+					getChannelIDWithRetryCheck(
+						point, c.DenyList,
+					)
+
+				if shouldRetry {
+					return fmt.Errorf("unknown channel " +
+						"point, please retry the " +
+						"request")
+				}
+
+				if !found {
+					// Channel is unknown but all deny list
+					// entries are mapped, so this channel
+					// is definitely not restricted.
 					return nil
 				}
 

rules/channel_restrictions_test.go

@@ -12,6 +12,7 @@ import (
 	"github.com/lightninglabs/lightning-terminal/session"
 	"github.com/lightninglabs/lndclient"
 	"github.com/lightningnetwork/lnd/lnrpc"
+	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 )
 
@@ -37,22 +38,25 @@ func TestChannelRestrictCheckRequest(t *testing.T) {
 
 	ctx := context.Background()
 	mgr := NewChannelRestrictMgr()
-	cfg := &mockLndClient{
-		channels: []lndclient.ChannelInfo{
-			{
-				ChannelID:    chanID1,
-				ChannelPoint: chanPointStr1,
-			},
-			{
-				ChannelID:    chanID2,
-				ChannelPoint: chanPointStr2,
-			},
-			{
-				ChannelID:    chanID3,
-				ChannelPoint: chanPointStr3,
-			},
+	cfg := &mockLndClient{}
+	cfg.On(
+		"ListChannels", mock.Anything, mock.Anything, mock.Anything,
+		mock.Anything,
+	).Return([]lndclient.ChannelInfo{
+		{
+			ChannelID:    chanID1,
+			ChannelPoint: chanPointStr1,
 		},
-	}
+		{
+			ChannelID:    chanID2,
+			ChannelPoint: chanPointStr2,
+		},
+		{
+			ChannelID:    chanID3,
+			ChannelPoint: chanPointStr3,
+		},
+	}, nil)
+
 	enf, err := mgr.NewEnforcer(ctx, cfg, &ChannelRestrict{
 		DenyList: []uint64{
 			chanID1, chanID2,
@@ -149,18 +153,23 @@ func newTXID() ([]byte, uint32, error) {
 type mockLndClient struct {
 	lndclient.LightningClient
 	Config
-
-	channels []lndclient.ChannelInfo
+	mock.Mock
 }
 
 func (m *mockLndClient) GetLndClient() lndclient.LightningClient {
 	return m
 }
 
-func (m *mockLndClient) ListChannels(_ context.Context, _, _ bool,
-	_ ...lndclient.ListChannelsOption) ([]lndclient.ChannelInfo, error) {
+func (m *mockLndClient) ListChannels(ctx context.Context, public, active bool,
+	opts ...lndclient.ListChannelsOption) ([]lndclient.ChannelInfo, error) {
 
-	return m.channels, nil
+	args := m.Called(ctx, public, active, opts)
+
+	if args.Get(0) != nil {
+		return args.Get(0).([]lndclient.ChannelInfo), args.Error(1)
+	}
+
+	return nil, args.Error(1)
 }
 
 // TestChannelRestrictRealToPseudo tests that the ChannelRestrict's RealToPseudo
@@ -292,3 +301,186 @@ func TestChannelRestrictRealToPseudo(t *testing.T) {
 		})
 	}
 }
+
+// TestChannelRestrictResilience ensures that the ChannelRestrictEnforcer is
+// resilient to missing channels during initialization.
+func TestChannelRestrictResilience(t *testing.T) {
+	var (
+		ctx = context.Background()
+		mgr = NewChannelRestrictMgr()
+	)
+
+	// Set up two channel points and IDs.
+	txid1, index1, err := newTXID()
+	require.NoError(t, err)
+	chanPointStr1 := fmt.Sprintf("%s:%d", hex.EncodeToString(txid1), index1)
+	chanID1, _ := firewalldb.NewPseudoUint64()
+	chanPoint1 := &lnrpc.ChannelPoint{
+		FundingTxid: &lnrpc.ChannelPoint_FundingTxidStr{
+			FundingTxidStr: hex.EncodeToString(txid1),
+		},
+		OutputIndex: index1,
+	}
+
+	txid2, index2, err := newTXID()
+	require.NoError(t, err)
+	chanPointStr2 := fmt.Sprintf("%s:%d", hex.EncodeToString(txid2), index2)
+	chanID2, _ := firewalldb.NewPseudoUint64()
+	chanPoint2 := &lnrpc.ChannelPoint{
+		FundingTxid: &lnrpc.ChannelPoint_FundingTxidStr{
+			FundingTxidStr: hex.EncodeToString(txid2),
+		},
+		OutputIndex: index2,
+	}
+
+	// Request: A request that tries to fetch a channel point that is not
+	// known yet. We expect the manager to try to refresh the channel list
+	// again to find the missing channel. The negative cache is empty at
+	// this point. The call fails because chanPoint2 is not known yet.
+	cfg := &mockLndClient{}
+	cfg.On(
+		"ListChannels", mock.Anything, mock.Anything, mock.Anything,
+		mock.Anything,
+	).Return(
+		[]lndclient.ChannelInfo{
+			// Initially we only have chanID1 open. Somebody was
+			// able to guess chanID2 even though it's not open yet.
+			{
+				ChannelID:    chanID1,
+				ChannelPoint: chanPointStr1,
+			},
+		}, nil)
+
+	// Each time a request comes in, a new enforcer is created.
+	enf, err := mgr.NewEnforcer(ctx, cfg, &ChannelRestrict{
+		DenyList: []uint64{chanID2},
+	})
+	require.NoError(t, err)
+
+	_, err = enf.HandleRequest(
+		ctx, "/lnrpc.Lightning/UpdateChannelPolicy",
+		&lnrpc.PolicyUpdateRequest{
+			Scope: &lnrpc.PolicyUpdateRequest_ChanPoint{
+				ChanPoint: chanPoint2,
+			},
+		},
+	)
+
+	// The request fails because the manager doesn't know about the mapping
+	// of chanPoint2 to chanID2. The negative cache is reset to force a
+	// reload of the mapping on the next request.
+	require.ErrorContains(t, err, "unknown channel point, please retry "+
+		"the request")
+	cfg.AssertExpectations(t)
+
+	// Request: Another request that tries to fetch a known channel point.
+	// We expect another call to ListChannels to refresh the mapping, since
+	// the negative cache was cleared after the last failed request.
+	cfg = &mockLndClient{}
+	cfg.On(
+		"ListChannels", mock.Anything, mock.Anything, mock.Anything,
+		mock.Anything,
+	).Return(
+		[]lndclient.ChannelInfo{
+			{
+				ChannelID:    chanID1,
+				ChannelPoint: chanPointStr1,
+			},
+		}, nil)
+
+	enf, err = mgr.NewEnforcer(ctx, cfg, &ChannelRestrict{
+		DenyList: []uint64{chanID2},
+	})
+	require.NoError(t, err)
+
+	_, err = enf.HandleRequest(
+		ctx, "/lnrpc.Lightning/UpdateChannelPolicy",
+		&lnrpc.PolicyUpdateRequest{
+			Scope: &lnrpc.PolicyUpdateRequest_ChanPoint{
+				ChanPoint: chanPoint1,
+			},
+		},
+	)
+	require.NoError(t, err)
+	cfg.AssertExpectations(t)
+
+	// Request: In case we retry the request for the unknown channel, we
+	// should error again. This time we don't expect another call to
+	// ListChannels because the negative cache was not invalidated before.
+	cfg = &mockLndClient{}
+	enf, err = mgr.NewEnforcer(ctx, cfg, &ChannelRestrict{
+		DenyList: []uint64{chanID2},
+	})
+	require.NoError(t, err)
+
+	_, err = enf.HandleRequest(
+		ctx, "/lnrpc.Lightning/UpdateChannelPolicy",
+		&lnrpc.PolicyUpdateRequest{
+			Scope: &lnrpc.PolicyUpdateRequest_ChanPoint{
+				ChanPoint: chanPoint2,
+			},
+		},
+	)
+
+	// The call errors, which invalidates the negative cache again.
+	require.ErrorContains(t, err, "unknown channel point, please retry "+
+		"the request")
+	cfg.AssertExpectations(t)
+
+	// We simulate the channel getting confirmed.
+	cfg = &mockLndClient{}
+	cfg.On(
+		"ListChannels", mock.Anything, mock.Anything, mock.Anything,
+		mock.Anything,
+	).Return(
+		[]lndclient.ChannelInfo{
+			{
+				ChannelID:    chanID1,
+				ChannelPoint: chanPointStr1,
+			},
+			{
+				ChannelID:    chanID2,
+				ChannelPoint: chanPointStr2,
+			},
+		}, nil)
+
+	// Request: Now the channel is known and in the deny list. The manager
+	// resyncs the channel list again and should now know about chanID2
+	// mapping to chanPoint2.
+	enf, err = mgr.NewEnforcer(ctx, cfg, &ChannelRestrict{
+		DenyList: []uint64{chanID2},
+	})
+	require.NoError(t, err)
+
+	// The request gets blocked.
+	_, err = enf.HandleRequest(
+		ctx, "/lnrpc.Lightning/UpdateChannelPolicy",
+		&lnrpc.PolicyUpdateRequest{
+			Scope: &lnrpc.PolicyUpdateRequest_ChanPoint{
+				ChanPoint: chanPoint2,
+			},
+		},
+	)
+	require.ErrorContains(t, err, "illegal action on channel in channel "+
+		"restriction list")
+	cfg.AssertExpectations(t)
+
+	// Request: Request to a channel not in the deny list. It should be
+	// allowed, without fetching the channel list again.
+	cfg = &mockLndClient{}
+	enf, err = mgr.NewEnforcer(ctx, cfg, &ChannelRestrict{
+		DenyList: []uint64{chanID2},
+	})
+	require.NoError(t, err)
+
+	_, err = enf.HandleRequest(
+		ctx, "/lnrpc.Lightning/UpdateChannelPolicy",
+		&lnrpc.PolicyUpdateRequest{
+			Scope: &lnrpc.PolicyUpdateRequest_ChanPoint{
+				ChanPoint: chanPoint1,
+			},
+		},
+	)
+	require.NoError(t, err)
+	cfg.AssertExpectations(t)
+}

rules/peer_restrictions_test.go

@@ -12,10 +12,12 @@ import (
 	"github.com/lightninglabs/lndclient"
 	"github.com/lightningnetwork/lnd/lnrpc"
 	"github.com/lightningnetwork/lnd/routing/route"
+	"github.com/stretchr/testify/mock"
 	"github.com/stretchr/testify/require"
 )
 
 // TestPeerRestrictCheckRequest ensures that the PeerRestrictEnforcer correctly
+
 // accepts or denys a request.
 func TestPeerRestrictCheckRequest(t *testing.T) {
 	txid1, index1, err := newTXID()
@@ -51,28 +53,31 @@ func TestPeerRestrictCheckRequest(t *testing.T) {
 
 	ctx := context.Background()
 	mgr := NewPeerRestrictMgr()
-	cfg := &mockLndClient{
-		channels: []lndclient.ChannelInfo{
-			{
-				ChannelPoint: chanPointStr1,
-				PubKeyBytes:  peerKey1,
-			},
-			{
-				ChannelPoint: chanPointStr2,
-				PubKeyBytes:  peerKey2,
-			},
-			{
-				ChannelPoint: chanPointStr3,
-				PubKeyBytes:  peerKey3,
-			},
+	cfg := &mockLndClient{}
+	cfg.On(
+		"ListChannels", mock.Anything, mock.Anything, mock.Anything,
+		mock.Anything,
+	).Return([]lndclient.ChannelInfo{
+		{
+			ChannelPoint: chanPointStr1,
+			PubKeyBytes:  peerKey1,
 		},
-	}
+		{
+			ChannelPoint: chanPointStr2,
+			PubKeyBytes:  peerKey2,
+		},
+		{
+			ChannelPoint: chanPointStr3,
+			PubKeyBytes:  peerKey3,
+		},
+	}, nil)
 
 	enf, err := mgr.NewEnforcer(ctx, cfg, &PeerRestrict{
 		DenyList: []string{
 			peerID1, peerID2,
 		},
 	})
+
 	require.NoError(t, err)
 
 	// A request for an irrelevant URI should be allowed.
@@ -198,6 +203,9 @@ func TestPeerRestrictCheckRequest(t *testing.T) {
 		ctx, "/lnrpc.Lightning/BatchOpenChannel", batchOpenReq,
 	)
 	require.NoError(t, err)
+
+	// Assert expected calls were made.
+	cfg.AssertExpectations(t)
 }
 
 // TestPeerRestrictionRealToPseudo tests that the PeerRestriction's RealToPseudo