diff --git a/rules/channel_restrictions.go b/rules/channel_restrictions.go index 52c97679..09e27840 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 fb14cc0d..916e6bd3 100644 --- a/rules/channel_restrictions_test.go +++ b/rules/channel_restrictions_test.go @@ -310,20 +310,177 @@ func TestChannelRestrictResilience(t *testing.T) { 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, + } - // Initially, LND has no channels. + 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{}, nil) + ).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) - // We create an enforcer that denies chanID1 (maybe a closed channel or - // generally unknown). This will be fixed in a future commit. - _, err := mgr.NewEnforcer(ctx, cfg, &ChannelRestrict{ - DenyList: []uint64{chanID1}, + // Each time a request comes in, a new enforcer is created. + enf, err := mgr.NewEnforcer(ctx, cfg, &ChannelRestrict{ + DenyList: []uint64{chanID2}, }) - require.ErrorContains(t, err, "invalid channel ID") + 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) }