Skip to content

Commit 648869b

Browse files
committed
fix(shrex/peers): bound blacklisted hashes with an LRU cache
The peer manager stored blacklisted datahashes in a plain map that was never pruned, so it grew for the entire lifetime of a node. A peer broadcasting shrexsub notifications with arbitrary invalid datahashes could drive unbounded memory growth. Replace the map with a fixed-size LRU cache. The blacklist is a best-effort spam filter, so evicting the least-recently-used entries is acceptable. Closes #1926
1 parent b4738cb commit 648869b

3 files changed

Lines changed: 40 additions & 7 deletions

File tree

share/shwap/p2p/shrex/peers/manager.go

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ import (
88
"sync/atomic"
99
"time"
1010

11+
lru "github.com/hashicorp/golang-lru/v2"
1112
logging "github.com/ipfs/go-log/v2"
1213
pubsub "github.com/libp2p/go-libp2p-pubsub"
1314
"github.com/libp2p/go-libp2p/core/event"
@@ -41,6 +42,11 @@ const (
4142
// storedPoolsAmount is the amount of pools for recent headers that will be stored in the peer
4243
// manager
4344
storedPoolsAmount = 10
45+
46+
// blacklistedHashesCacheSize bounds the number of blacklisted datahashes kept in memory.
47+
// The blacklist is a best-effort spam filter, so evicting the least-recently-used entries
48+
// is acceptable and prevents unbounded growth from a stream of invalid datahashes.
49+
blacklistedHashesCacheSize = 1024
4450
)
4551

4652
type result string
@@ -74,8 +80,8 @@ type Manager struct {
7480
// nodes collects nodes' peer.IDs found via discovery
7581
nodes *pool
7682

77-
// hashes that are not in the chain
78-
blacklistedHashes map[string]bool
83+
// hashes that are not in the chain, bounded by an LRU to avoid unbounded growth
84+
blacklistedHashes *lru.Cache[string, struct{}]
7985

8086
metrics *metrics
8187

@@ -111,12 +117,17 @@ func NewManager(
111117
return nil, err
112118
}
113119

120+
blacklistedHashes, err := lru.New[string, struct{}](blacklistedHashesCacheSize)
121+
if err != nil {
122+
return nil, fmt.Errorf("shrex/peer-manager: creating blacklisted hashes cache: %w", err)
123+
}
124+
114125
s := &Manager{
115126
params: params,
116127
connGater: connGater,
117128
host: host,
118129
pools: make(map[string]*syncPool),
119-
blacklistedHashes: make(map[string]bool),
130+
blacklistedHashes: blacklistedHashes,
120131
headerSubDone: make(chan struct{}),
121132
disconnectedPeersDone: make(chan struct{}),
122133
tag: tag,
@@ -445,7 +456,7 @@ func (m *Manager) isBlacklistedPeer(peerID peer.ID) bool {
445456
func (m *Manager) isBlacklistedHash(hash share.DataHash) bool {
446457
m.lock.Lock()
447458
defer m.lock.Unlock()
448-
return m.blacklistedHashes[hash.String()]
459+
return m.blacklistedHashes.Contains(hash.String())
449460
}
450461

451462
func (m *Manager) validatedPool(hashStr string, height uint64) *syncPool {
@@ -520,7 +531,7 @@ func (m *Manager) cleanUp() []peer.ID {
520531
"hash", h,
521532
"peer_list", p.peersList)
522533
// blacklist hash
523-
m.blacklistedHashes[h] = true
534+
m.blacklistedHashes.Add(h, struct{}{})
524535

525536
// blacklist peers
526537
for _, peer := range p.peersList {

share/shwap/p2p/shrex/peers/manager_test.go

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package peers
22

33
import (
44
"context"
5+
"strconv"
56
"sync"
67
"testing"
78
"time"
@@ -287,7 +288,7 @@ func TestManager(t *testing.T) {
287288
// check that no peers or hashes were blacklisted
288289
manager.params.PoolValidationTimeout = 0
289290
require.Len(t, blacklisted, 0)
290-
require.Len(t, manager.blacklistedHashes, 0)
291+
require.Equal(t, 0, manager.blacklistedHashes.Len())
291292
})
292293

293294
t.Run("pools store window", func(t *testing.T) {
@@ -469,6 +470,27 @@ func TestIntegration(t *testing.T) {
469470
})
470471
}
471472

473+
// TestManager_blacklistedHashesBounded ensures the blacklisted hashes cache
474+
// does not grow without bound. Previously it was a plain map with no eviction,
475+
// so a peer spamming invalid datahashes could grow it indefinitely (#1926).
476+
func TestManager_blacklistedHashesBounded(t *testing.T) {
477+
ctx, cancel := context.WithTimeout(context.Background(), time.Second*5)
478+
defer cancel()
479+
480+
headerSub := newSubLock(testHeader(), nil)
481+
manager, err := testManager(ctx, headerSub)
482+
require.NoError(t, err)
483+
484+
// blacklist far more datahashes than the cache bound, as would happen on a
485+
// long-running node fed a stream of invalid hashes.
486+
for i := range blacklistedHashesCacheSize * 3 {
487+
manager.blacklistedHashes.Add(strconv.Itoa(i), struct{}{})
488+
}
489+
490+
require.Equal(t, blacklistedHashesCacheSize, manager.blacklistedHashes.Len(),
491+
"blacklisted hashes cache must stay bounded")
492+
}
493+
472494
func testManager(ctx context.Context, headerSub libhead.Subscriber[*header.ExtendedHeader]) (*Manager, error) {
473495
host, err := mocknet.New().GenPeer()
474496
if err != nil {

share/shwap/p2p/shrex/peers/metrics.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -277,6 +277,6 @@ func (m *Manager) shrexPools() map[poolStatus]int64 {
277277
shrexPools[poolStatusValidated]++
278278
}
279279

280-
shrexPools[poolStatusBlacklisted] = int64(len(m.blacklistedHashes))
280+
shrexPools[poolStatusBlacklisted] = int64(m.blacklistedHashes.Len())
281281
return shrexPools
282282
}

0 commit comments

Comments
 (0)