From 6bb0588ad8e7f922e4ad5580f51265a4097af08f Mon Sep 17 00:00:00 2001 From: 0xSHKWON Date: Fri, 14 Aug 2026 08:17:00 +0900 Subject: [PATCH] common/lru, core/vm: count key bytes against the precompile cache budget (#35526) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #35473 keys the precompile cache on the input, but SizeConstrainedCache only counts the value’s bytes, so a cacheable empty output (e.g. a failed ECRECOVER) is never evicted and grows the cache without bound. Skipping empty outputs alone would still leave sha256/ripemd160 ~256x over budget, and would only hold as long as every failing output happens to be empty. --- common/lru/blob_lru.go | 33 ++++++++++++++++++--- common/lru/blob_lru_test.go | 51 ++++++++++++++++++++++++++++++++ core/vm/precompile_cache.go | 6 +++- core/vm/precompile_cache_test.go | 45 ++++++++++++++++++++++++++++ 4 files changed, 130 insertions(+), 5 deletions(-) diff --git a/common/lru/blob_lru.go b/common/lru/blob_lru.go index c9b3398503..1916fe1e17 100644 --- a/common/lru/blob_lru.go +++ b/common/lru/blob_lru.go @@ -30,24 +30,49 @@ type blobType interface { // is at capacity, and a new item is added, older items are evicted until the size // constraint is met. // +// By default only the value bytes count towards the size constraint. If the key +// carries a meaningful share of an entry's memory, pass a key-size function to +// NewSizeConstrainedCacheWithKeySize so it is accounted too. +// // OBS: This cache assumes that items are content-addressed: keys are unique per content. // In other words: two Add(..) with the same key K, will always have the same value V. type SizeConstrainedCache[K comparable, V blobType] struct { size uint64 maxSize uint64 + keySize func(K) uint64 lru BasicLRU[K, V] lock sync.Mutex } -// NewSizeConstrainedCache creates a new size-constrained LRU cache. +// NewSizeConstrainedCache creates a new size-constrained LRU cache in which only +// the value bytes count towards the size constraint. func NewSizeConstrainedCache[K comparable, V blobType](maxSize uint64) *SizeConstrainedCache[K, V] { + return NewSizeConstrainedCacheWithKeySize[K, V](maxSize, nil) +} + +// NewSizeConstrainedCacheWithKeySize is like NewSizeConstrainedCache but also +// charges each entry's key against the size constraint, measured by keySize. +// This matters when the key, not the value, holds the bulk of an entry's memory. +// A nil keySize counts keys as free, identical to NewSizeConstrainedCache. +func NewSizeConstrainedCacheWithKeySize[K comparable, V blobType](maxSize uint64, keySize func(K) uint64) *SizeConstrainedCache[K, V] { return &SizeConstrainedCache[K, V]{ size: 0, maxSize: maxSize, + keySize: keySize, lru: NewBasicLRU[K, V](math.MaxInt), } } +// entrySize reports the number of bytes an entry counts for against the size +// constraint: the value length, plus the key size when a measure was provided. +func (c *SizeConstrainedCache[K, V]) entrySize(key K, value V) uint64 { + n := uint64(len(value)) + if c.keySize != nil { + n += c.keySize(key) + } + return n +} + // Add adds a value to the cache. Returns true if an eviction occurred. // OBS: This cache assumes that items are content-addressed: keys are unique per content. // In other words: two Add(..) with the same key K, will always have the same value V. @@ -59,15 +84,15 @@ func (c *SizeConstrainedCache[K, V]) Add(key K, value V) (evicted bool) { // Unless it is already present, might need to evict something. // OBS: If it is present, we still call Add internally to bump the recentness. if !c.lru.Contains(key) { - targetSize := c.size + uint64(len(value)) + targetSize := c.size + c.entrySize(key, value) for targetSize > c.maxSize { evicted = true - _, v, ok := c.lru.RemoveOldest() + k, v, ok := c.lru.RemoveOldest() if !ok { // list is now empty. Break break } - targetSize -= uint64(len(v)) + targetSize -= c.entrySize(k, v) } c.size = targetSize } diff --git a/common/lru/blob_lru_test.go b/common/lru/blob_lru_test.go index ca1b0ddd74..84bb372bcf 100644 --- a/common/lru/blob_lru_test.go +++ b/common/lru/blob_lru_test.go @@ -153,3 +153,54 @@ func TestSizeConstrainedCacheEmpties(t *testing.T) { } } } + +// TestSizeConstrainedCacheCountsKeys verifies that a cache constructed with a +// key-size function charges the key against the byte budget, so entries whose +// bytes live in the key (empty or tiny values) can no longer grow it without +// bound. +func TestSizeConstrainedCacheCountsKeys(t *testing.T) { + const ( + maxSize = 64 * 1024 + keyLen = 256 + ) + c := NewSizeConstrainedCacheWithKeySize[string, []byte](maxSize, func(k string) uint64 { return uint64(len(k)) }) + + // Add far more distinct keyLen-byte keys with EMPTY values than the budget + // can hold. Without key accounting the size would stay 0 and nothing would + // ever be evicted. + const n = 10000 + for i := 0; i < n; i++ { + var key [keyLen]byte + binary.BigEndian.PutUint64(key[:8], uint64(i)) + c.Add(string(key[:]), nil) + } + if c.size > maxSize { + t.Fatalf("reported size %d exceeds budget %d", c.size, maxSize) + } + if c.lru.Len() >= n { + t.Fatalf("no eviction happened: kept all %d empty-value entries", n) + } + // The earliest entry (all-zero key) must have been evicted. + var first [keyLen]byte + if _, ok := c.Get(string(first[:])); ok { + t.Fatal("oldest empty-value entry was never evicted") + } +} + +// TestSizeConstrainedCacheKeysFreeByDefault pins the default behaviour: the +// plain constructor does not count keys, so existing callers are unaffected. +func TestSizeConstrainedCacheKeysFreeByDefault(t *testing.T) { + c := NewSizeConstrainedCache[string, []byte](1024) + const n = 1000 + for i := 0; i < n; i++ { + var key [512]byte + binary.BigEndian.PutUint64(key[:8], uint64(i)) + c.Add(string(key[:]), nil) // empty value -> size stays 0 + } + if c.size != 0 { + t.Fatalf("expected size 0 with keys free, got %d", c.size) + } + if c.lru.Len() != n { + t.Fatalf("expected all %d entries retained, got %d", n, c.lru.Len()) + } +} diff --git a/core/vm/precompile_cache.go b/core/vm/precompile_cache.go index 3045c38f68..e02fa57083 100644 --- a/core/vm/precompile_cache.go +++ b/core/vm/precompile_cache.go @@ -149,7 +149,11 @@ func (c *PrecompileCache) store(scope precompileCacheScope, key []byte, output [ if results == nil { c.data.mu.Lock() if results = c.data.caches[scope]; results == nil { - results = lru.NewSizeConstrainedCache[string, []byte](maxCacheablePrecompileBytes) + // The key is the precompile input, which dwarfs the output (up to + // maxCacheablePrecompileInput against maxCacheablePrecompileOutput), + // so it must count towards the budget. Without it an empty output + // (e.g. a failed ECRECOVER) would let the cache grow without bound. + results = lru.NewSizeConstrainedCacheWithKeySize[string, []byte](maxCacheablePrecompileBytes, func(k string) uint64 { return uint64(len(k)) }) c.data.caches[scope] = results } c.data.mu.Unlock() diff --git a/core/vm/precompile_cache_test.go b/core/vm/precompile_cache_test.go index 75f18d68a7..cbfdf123e7 100644 --- a/core/vm/precompile_cache_test.go +++ b/core/vm/precompile_cache_test.go @@ -18,6 +18,7 @@ package vm import ( "bytes" + "encoding/binary" "encoding/hex" "encoding/json" "fmt" @@ -431,3 +432,47 @@ func BenchmarkPrecompileCacheSynthetic(b *testing.B) { } } } + +// TestPrecompileCacheBoundedByKeyBytes checks that the precompile result cache +// stays within its byte budget even when its entries carry empty outputs, such +// as a failed ECRECOVER. The cache key is the precompile input, so it must +// count towards the budget; otherwise a stream of distinct failing inputs would +// grow the cache without bound, since an empty output adds nothing to the size. +func TestPrecompileCacheBoundedByKeyBytes(t *testing.T) { + cache := NewPrecompileCache() + scope := precompileCacheScope{activePrecompiledContracts(params.Rules{}), common.BytesToAddress([]byte{0x1})} + ec := &ecrecover{} + + // A failing signature runs to an empty output with no error, and is still + // eligible for caching. Keep its key as the eviction witness. + var first [128]byte + first[63] = 0xff // invalid v + out, err := ec.Run(first[:]) + if err != nil || len(out) != 0 { + t.Fatalf("expected empty failed-ecrecover output, got len=%d err=%v", len(out), err) + } + firstKey, ok := precompileCacheKey(ec, first[:]) + if !ok { + t.Fatal("failed ecrecover unexpectedly not cacheable") + } + cache.store(scope, firstKey, out) + + // Store many more distinct failing inputs. A trailing sentinel byte keeps + // every normalized key a full 128 bytes, so their key bytes alone overrun + // the 1 MiB budget several times over. + const n = 20000 + for i := 1; i < n; i++ { + var in [128]byte + in[63] = 0xff + in[127] = 0x01 + binary.BigEndian.PutUint64(in[64:72], uint64(i)) + key, ok := precompileCacheKey(ec, in[:]) + if !ok { + t.Fatalf("input %d not cacheable", i) + } + cache.store(scope, key, nil) + } + if _, hit := cache.load(scope, firstKey); hit { + t.Fatal("cache is unbounded: oldest empty-output entry was never evicted") + } +}