diff --git a/docs/release-notes/release-notes-0.16.1.md b/docs/release-notes/release-notes-0.16.1.md index a0f0bb530..cdf53024e 100644 --- a/docs/release-notes/release-notes-0.16.1.md +++ b/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) \ No newline at end of file +# Contributors (Alphabetical Order) + +* bitromortac diff --git a/rules/channel_restrictions.go b/rules/channel_restrictions.go index 52c976799..09e278402 100644 --- a/rules/channel_restrictions.go +++ b/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 } diff --git a/rules/channel_restrictions_test.go b/rules/channel_restrictions_test.go index 6102851c4..916e6bd3b 100644 --- a/rules/channel_restrictions_test.go +++ b/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) { + + args := m.Called(ctx, public, active, opts) + + if args.Get(0) != nil { + return args.Get(0).([]lndclient.ChannelInfo), args.Error(1) + } - return m.channels, nil + 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) +} diff --git a/rules/peer_restrictions_test.go b/rules/peer_restrictions_test.go index abfa30540..5b8fedb77 100644 --- a/rules/peer_restrictions_test.go +++ b/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