From 3d5ead232eeb36468922cfee4a808c66ba54548e Mon Sep 17 00:00:00 2001 From: awskii Date: Tue, 11 Aug 2026 19:07:33 +0700 Subject: [PATCH 1/6] execution/commitment: make the storage-branch cache route allocation-free MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit storageRoute ran on every storage-branch Get/Put/Invalidate/PinEntry once any contract is pinned, and rebuilt the 32-byte account hash the slow way — a CompactToHex expansion plus a second 32-byte alloc — even though ContractHashFromPrefix already computes exactly that value with no allocations, proven equivalent by TestContractHashFromPrefix_MatchesReference. Only the shallow fixed-array tier needs the expanded storage nibbles at all; the deep overflow tier is keyed by the original prefix. Also collapses Invalidate's deep-tier Get+Delete into one LoadAndDelete, which hashes the key once and closes the TOCTOU window between the two. storageRoute takes caller-owned scratch for the nibble slice: the function is far past the inlining budget, so returning a slice into a local array heap-promotes it on every call and undoes the win. The now-dead acct return is dropped. BenchmarkPreloadDrain, Apple M5 Max (18 cores), -benchtime=100x -count=10: d67 sec/op 1.558m -> 1.406m -9.72% B/op 4.954Mi -> 4.488Mi -9.42% d68 sec/op 33.83m -> 29.87m -11.69% B/op 108.3Mi -> 100.8Mi -6.90% allocs/op -16.37% (d67) / -15.82% (d68) All p=0.000, n=10. execution/commitment/... and db/state/... green, plus -race on TestBranchCache*. --- execution/commitment/branch_cache.go | 95 ++++++++++++++++------- execution/commitment/branch_cache_test.go | 65 ++++++++++++++++ 2 files changed, 134 insertions(+), 26 deletions(-) diff --git a/execution/commitment/branch_cache.go b/execution/commitment/branch_cache.go index 3332d633763..503b023a97a 100644 --- a/execution/commitment/branch_cache.go +++ b/execution/commitment/branch_cache.go @@ -26,7 +26,6 @@ import ( "github.com/erigontech/erigon/common/log/v3" "github.com/erigontech/erigon/common/maphash" "github.com/erigontech/erigon/execution/cache/coherence" - "github.com/erigontech/erigon/execution/commitment/nibbles" ) // u64ident is the freelru hash callback for uint64 keys already well-distributed @@ -428,37 +427,39 @@ func (c *BranchCache) trunkSlot(prefix []byte, forWrite bool) *atomic.Pointer[br // nibbles + S storage nibbles) into its contract storageTrunk and the // storage-nibble path. Returns ok=false for non-storage prefixes (< 64 nibbles) // so the caller falls through to the LRU tail. When create is true the -// contract's storageTrunk is allocated on demand (PinEntry path). acct is the -// 32-byte packed account hash (the map key). -func (c *BranchCache) storageRoute(prefix []byte, create bool) (st *trunk, acct []byte, stor []byte, ok bool) { - if len(prefix) < 33 { - return nil, nil, nil, false +// contract's storageTrunk is allocated on demand (PinEntry path). nibBuf is +// caller-owned scratch space for the returned stor slice — storageRoute is too +// large to inline, so a value it returned a slice into would otherwise force +// a heap allocation on every call; letting the caller's own stack array back +// it instead keeps the common lookup allocation-free. +func (c *BranchCache) storageRoute(prefix []byte, create bool, nibBuf *[4]byte) (st *trunk, stor []byte, ok bool) { + acctHash, ok := ContractHashFromPrefix(prefix) + if !ok { + return nil, nil, false } - // Nothing pinned and not creating: skip the CompactToHex + packed-key alloc - // that every >=64-nibble read would otherwise pay before finding no pins. + // Nothing pinned and not creating: skip the stor decode that every + // >=64-nibble read would otherwise pay before finding no pins. if !create && c.pinned.Load() == nil { - return nil, nil, nil, false - } - nib := nibbles.CompactToHex(prefix) - if len(nib) < 64 { - return nil, nil, nil, false + return nil, nil, false } - packed := make([]byte, 32) - for i := range 32 { - packed[i] = nib[2*i]<<4 | nib[2*i+1] + packed := acctHash[:] + n := storageNibbles(prefix, nibBuf) + if n > 4 { + stor = prefix // slot() only inspects len(path) past 4, never the content + } else { + stor = nibBuf[:n] } - stor = nib[64:] if p := c.pinned.Load(); p != nil { if st, found := p.Get(packed); found { - return st, packed, stor, true + return st, stor, true } } if !create { - return nil, packed, stor, false + return nil, stor, false } st = newStorageTrunk(c.maxDepth) c.pinnedForWrite().Set(packed, st) - return st, packed, stor, true + return st, stor, true } // pinnedForWrite returns the pinned-contract map, allocating it on first pin. @@ -496,6 +497,45 @@ func ContractHashFromPrefix(prefix []byte) (hash [32]byte, ok bool) { return hash, true } +// storageNibbles decodes the storage-trie path nibbles that follow the +// 64-nibble account hash in prefix into nib, matching +// nibbles.CompactToHex(prefix)[64:]. Only the first 4 are materialized (n +// reports the true count) since that is as deep as the fixed-array tier +// goes; n > 4 routes to the deep overflow map, which is keyed by prefix +// directly and never inspects these nibbles. Assumes prefix carries no +// terminator flag, true for every BranchCache prefix — a trie traversal +// path, never a full leaf key. +func storageNibbles(prefix []byte, nib *[4]byte) (n int) { + odd := prefix[0]&0x10 != 0 + n = 2*len(prefix) - 2 - 64 + if odd { + n++ + } + if n > 4 { + return n + } + if odd { + for i := 0; i < n; i++ { + b := prefix[32+(i+1)/2] + if i&1 == 0 { + nib[i] = b & 0x0f + } else { + nib[i] = b >> 4 + } + } + return n + } + for i := 0; i < n; i++ { + b := prefix[33+i/2] + if i&1 == 0 { + nib[i] = b >> 4 + } else { + nib[i] = b & 0x0f + } + } + return n +} + // clearTrunk resets every resident account-trunk slot (depths 0-4) to nil in // place (atomic per-slot stores, not a pointer swap — lock-free readers deref // c.accountTrunk concurrently). @@ -571,7 +611,8 @@ func (c *BranchCache) lookup(prefix []byte) (*branchCacheEntry, bool) { // Pinned tier: per-contract storage trunk (fixed skeleton + deep overflow). // Only a lookup that actually routes to a pinned trunk counts toward the // pinned hit/miss stats; account-trie and tail-only prefixes are excluded. - if st, _, stor, ok := c.storageRoute(prefix, false); ok { + var nibBuf [4]byte + if st, stor, ok := c.storageRoute(prefix, false, &nibBuf); ok { var entry *branchCacheEntry if slot := st.slot(stor, false); slot != nil { entry = slot.Load() @@ -611,7 +652,8 @@ func (c *BranchCache) store(prefix []byte, entry *branchCacheEntry) { } // Keep a prefix already pinned in a storage trunk in place across the // per-block invalidate+Put refresh rather than dropping it to the tail. - if st, _, stor, ok := c.storageRoute(prefix, false); ok { + var nibBuf [4]byte + if st, stor, ok := c.storageRoute(prefix, false, &nibBuf); ok { if slot := st.slot(stor, false); slot != nil { if slot.Load() != nil { slot.Store(entry) @@ -641,7 +683,8 @@ func (c *BranchCache) PinEntry(prefix []byte, data []byte, step, txN uint64) { defer stripe.Unlock() entry := &branchCacheEntry{data: dataCopy, step: step, txN: txN, epoch: c.coh.Epoch()} - st, _, stor, ok := c.storageRoute(prefix, true) + var nibBuf [4]byte + st, stor, ok := c.storageRoute(prefix, true, &nibBuf) if !ok { c.tailForWrite().Add(maphash.Hash(prefix), entry) return @@ -727,13 +770,13 @@ func (c *BranchCache) Invalidate(prefix []byte) { slot.Store(nil) return } - if st, _, stor, ok := c.storageRoute(prefix, false); ok { + var nibBuf [4]byte + if st, stor, ok := c.storageRoute(prefix, false, &nibBuf); ok { if slot := st.slot(stor, false); slot != nil { if slot.Swap(nil) != nil { c.pinnedEntries.Add(-1) } - } else if _, exists := st.deep.Get(prefix); exists { - st.deep.Delete(prefix) + } else if _, loaded := st.deep.LoadAndDelete(prefix); loaded { c.pinnedEntries.Add(-1) } } diff --git a/execution/commitment/branch_cache_test.go b/execution/commitment/branch_cache_test.go index 88a660cfadb..fc476ca70ca 100644 --- a/execution/commitment/branch_cache_test.go +++ b/execution/commitment/branch_cache_test.go @@ -17,12 +17,15 @@ package commitment import ( + "math/rand" "runtime" "strings" "sync" "testing" "github.com/stretchr/testify/require" + + "github.com/erigontech/erigon/execution/commitment/nibbles" ) // TestBranchCache_AccountTrunkRouting verifies account-trie branches at nibble @@ -449,3 +452,65 @@ func TestBranchCache_ConcurrentTailGrow(t *testing.T) { } wg.Wait() } + +// storageNibblesReference is the pre-optimization computation kept as the +// oracle: full CompactToHex expansion, then slice past the 64 account +// nibbles. storageNibbles must agree with it whenever the true count is <=4. +func storageNibblesReference(prefix []byte) (nib [4]byte, n int) { + full := nibbles.CompactToHex(prefix) + n = len(full) - 64 + for i := 0; i < n && i < 4; i++ { + nib[i] = full[64+i] + } + return nib, n +} + +func TestStorageNibbles_MatchesReference(t *testing.T) { + rng := rand.New(rand.NewSource(2)) + for range 5000 { + l := 33 + rng.Intn(20) // spans storLen 0 through comfortably past 4 + prefix := make([]byte, l) + rng.Read(prefix) + oddBit := byte(0) + if rng.Intn(2) == 1 { + oddBit = 0x10 + } + prefix[0] = prefix[0]&0x0f | oddBit // no terminator flag: storageNibbles' precondition + + wantNib, wantN := storageNibblesReference(prefix) + var gotNib [4]byte + gotN := storageNibbles(prefix, &gotNib) + require.Equalf(t, wantN, gotN, "n mismatch len=%d prefix0=%#x", l, prefix[0]) + if gotN <= 4 { + require.Equalf(t, wantNib, gotNib, "nibbles mismatch len=%d prefix0=%#x", l, prefix[0]) + } + } +} + +// TestBranchCache_StorageRoute_ZeroAlloc verifies storageRoute's account-hash +// and storage-nibble decode no longer pay the CompactToHex + packed-key +// allocations on a storage-tier lookup, for both odd and even flag parity. +func TestBranchCache_StorageRoute_ZeroAlloc(t *testing.T) { + c := NewBranchCache(100) + + even := make([]byte, 33) // even flag, storLen=0 → storage trunk's d0 + for i := 1; i < 33; i++ { + even[i] = byte(i) + } + c.PinEntry(even, []byte("even"), 0, 100) + + odd := make([]byte, 34) // odd flag, storLen=3 → storage trunk's d3 + odd[0] = 0x10 + for i := 1; i < 34; i++ { + odd[i] = byte(i * 7) + } + c.PinEntry(odd, []byte("odd"), 0, 100) + + for _, prefix := range [][]byte{even, odd} { + allocs := testing.AllocsPerRun(1000, func() { + var nibBuf [4]byte // caller-owned scratch, as in Get/store/PinEntry/Invalidate + _, _, _ = c.storageRoute(prefix, false, &nibBuf) + }) + require.Zerof(t, allocs, "storageRoute must not allocate on a storage-tier lookup, prefix0=%#x", prefix[0]) + } +} From 29f389e8d0ba3a362fc4f2857df6021e4e12367e Mon Sep 17 00:00:00 2001 From: Alexey Sharov Date: Wed, 12 Aug 2026 14:01:32 +0700 Subject: [PATCH 2/6] save --- p2p/protocols/eth/protocol_test.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/p2p/protocols/eth/protocol_test.go b/p2p/protocols/eth/protocol_test.go index d9f50c50704..26c7215367f 100644 --- a/p2p/protocols/eth/protocol_test.go +++ b/p2p/protocols/eth/protocol_test.go @@ -412,13 +412,15 @@ func TestHashOrNumberEncodeRLPPointerIsAllocFree(t *testing.T) { } valueAllocs := testing.AllocsPerRun(200, mustEncode(hn.Hash)) pointerAllocs := testing.AllocsPerRun(200, mustEncode(&hn.Hash)) - t.Logf("allocs/op: byValue=%v byPointer=%v", valueAllocs, pointerAllocs) - if pointerAllocs >= valueAllocs { - t.Errorf("pointer form should allocate less: value=%v pointer=%v", valueAllocs, pointerAllocs) - } - // encBuffer is pooled, and sync.Pool drops values under the race detector. - //goland:noinspection GoBoolExpressions - if !race.Enabled && pointerAllocs != 0 { - t.Errorf("pointer form should not allocate, got %v", pointerAllocs) + if !race.Enabled { + t.Logf("allocs/op: byValue=%v byPointer=%v", valueAllocs, pointerAllocs) + if pointerAllocs >= valueAllocs { + t.Errorf("pointer form should allocate less: value=%v pointer=%v", valueAllocs, pointerAllocs) + } + // encBuffer is pooled, and sync.Pool drops values under the race detector. + //goland:noinspection GoBoolExpressions + if pointerAllocs != 0 { + t.Errorf("pointer form should not allocate, got %v", pointerAllocs) + } } } From 43bf315bce886c2c1531985cd14840ced29b3306 Mon Sep 17 00:00:00 2001 From: awskii Date: Wed, 12 Aug 2026 15:16:47 +0700 Subject: [PATCH 3/6] execution/commitment, common/maphash: fix the storage-route order and two pin races MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit storageRoute decoded the account hash, and then the storage nibbles, before the pinned-map check that discards both. With nothing pinned a storage-branch read paid the whole decode to learn there were no pins; once anything was pinned it paid it again for every non-pinned contract. Length guard and pinned check now come first, and pinned is loaded once instead of twice with no guarantee the two reads agreed. Measured on a 34-byte odd-flag prefix with nothing pinned, 2e6 iters x5: 9.812 -> 1.498 ns/op odd, 2.228 -> 1.344 ns/op even. The create path did Get-then-Set on the pinned map. Two storage slots of one contract can take different put stripes, so both racers could miss, both build a trunk, and the loser's trunk and its already-counted entry were dropped: 45-56 lost pins per 200 attempts, never zero. LoadOrStore hands both the same trunk, and 200 attempts now lose none. store() re-populated a pinned slot behind Invalidate's back — it checked the slot non-nil, then stored, while Invalidate cleared it lock-free and decremented. The entry came back without a matching increment, so PinnedCount drifted and reached -2 under -race. The slot now takes a CAS loop and the deep tier a new maphash.ReplaceIfPresent, which never inserts, so neither can resurrect what Invalidate removed. slot() takes the nibble count, which drops the stor = prefix placeholder and the unstated invariant that n > 4 implied len(prefix) >= 35. storageNibbles gets the length guard it assumed and collapses to one loop over a single parity offset. The new round-trip test covers storage depths 0-8 in both parities, including the deep tier the suite never reached. --- common/maphash/maphash.go | 13 +++ execution/commitment/branch_cache.go | 119 +++++++++------------- execution/commitment/branch_cache_test.go | 38 +++++++ 3 files changed, 101 insertions(+), 69 deletions(-) diff --git a/common/maphash/maphash.go b/common/maphash/maphash.go index 0ef8fef863a..33513a77067 100644 --- a/common/maphash/maphash.go +++ b/common/maphash/maphash.go @@ -52,6 +52,19 @@ func (m *Map[V]) LoadOrStore(key []byte, value V) (actual V, loaded bool) { return m.m.LoadOrStore(h, value) } +// ReplaceIfPresent atomically overwrites an existing key, reporting whether it +// was there. It never inserts, so external counts need no accounting. +func (m *Map[V]) ReplaceIfPresent(key []byte, value V) bool { + h := Hash(key) + _, ok := m.m.Compute(h, func(old V, loaded bool) (V, xsync.ComputeOp) { + if !loaded { + return old, xsync.CancelOp + } + return value, xsync.UpdateOp + }) + return ok +} + // Delete removes a key from the map. func (m *Map[V]) Delete(key []byte) { h := Hash(key) diff --git a/execution/commitment/branch_cache.go b/execution/commitment/branch_cache.go index e2134c38c17..7ea19b9bb10 100644 --- a/execution/commitment/branch_cache.go +++ b/execution/commitment/branch_cache.go @@ -259,8 +259,10 @@ func adaptiveTrunkDepth(active int64) uint8 { // slot returns the fixed-array slot for a nibble path of length 0-3 (and length // 4 when the depth-4 array is present, i.e. the account trunk), or nil when the // path is deeper — the caller then uses deep (storage) or the tail (account). -func (t *trunk) slot(path []byte, forWrite bool) *atomic.Pointer[branchCacheEntry] { - switch len(path) { +// slot returns the cell for an n-nibble path, nil when that depth has no +// resident tier and the caller must use deep. +func (t *trunk) slot(path *[4]byte, n int, forWrite bool) *atomic.Pointer[branchCacheEntry] { + switch n { case 0: return &t.d0 case 1: @@ -422,43 +424,35 @@ func (c *BranchCache) trunkSlot(prefix []byte, forWrite bool) *atomic.Pointer[br return nil } -// storageRoute decodes a storage-trunk prefix (compact-hex of 64 account -// nibbles + S storage nibbles) into its contract storageTrunk and the -// storage-nibble path. Returns ok=false for non-storage prefixes (< 64 nibbles) -// so the caller falls through to the LRU tail. When create is true the -// contract's storageTrunk is allocated on demand (PinEntry path). nibBuf is -// caller-owned scratch space for the returned stor slice — storageRoute is too -// large to inline, so a value it returned a slice into would otherwise force -// a heap allocation on every call; letting the caller's own stack array back -// it instead keeps the common lookup allocation-free. -func (c *BranchCache) storageRoute(prefix []byte, create bool, nibBuf *[4]byte) (st *trunk, stor []byte, ok bool) { +// storageRoute resolves a storage prefix to its contract trunk and storage +// depth, allocating the trunk when create is set. ok=false means non-storage; +// the caller falls through to the tail. nibBuf is caller-owned scratch — +// storageRoute cannot inline, so a local would escape. +func (c *BranchCache) storageRoute(prefix []byte, create bool, nibBuf *[4]byte) (st *trunk, n int, ok bool) { + if len(prefix) < 33 { + return nil, 0, false + } + // Both decodes stay behind this check; ahead of it they are pure cost. + p := c.pinned.Load() + if !create && p == nil { + return nil, 0, false + } acctHash, ok := ContractHashFromPrefix(prefix) if !ok { - return nil, nil, false - } - // Nothing pinned and not creating: skip the stor decode that every - // >=64-nibble read would otherwise pay before finding no pins. - if !create && c.pinned.Load() == nil { - return nil, nil, false + return nil, 0, false } packed := acctHash[:] - n := storageNibbles(prefix, nibBuf) - if n > 4 { - stor = prefix // slot() only inspects len(path) past 4, never the content - } else { - stor = nibBuf[:n] - } - if p := c.pinned.Load(); p != nil { + if p != nil { if st, found := p.Get(packed); found { - return st, stor, true + return st, storageNibbles(prefix, nibBuf), true } } if !create { - return nil, stor, false + return nil, 0, false } - st = newStorageTrunk(c.maxDepth) - c.pinnedForWrite().Set(packed, st) - return st, stor, true + // Two slots of one contract can take different put stripes, so this races itself. + st, _ = c.pinnedForWrite().LoadOrStore(packed, newStorageTrunk(c.maxDepth)) + return st, storageNibbles(prefix, nibBuf), true } // pinnedForWrite returns the pinned-contract map, allocating it on first pin. @@ -496,37 +490,24 @@ func ContractHashFromPrefix(prefix []byte) (hash [32]byte, ok bool) { return hash, true } -// storageNibbles decodes the storage-trie path nibbles that follow the -// 64-nibble account hash in prefix into nib, matching -// nibbles.CompactToHex(prefix)[64:]. Only the first 4 are materialized (n -// reports the true count) since that is as deep as the fixed-array tier -// goes; n > 4 routes to the deep overflow map, which is keyed by prefix -// directly and never inspects these nibbles. Assumes prefix carries no -// terminator flag, true for every BranchCache prefix — a trie traversal -// path, never a full leaf key. +// storageNibbles decodes the storage nibbles after the 64-nibble account hash, +// matching nibbles.CompactToHex(prefix)[64:]. Only the first 4 are written; n is +// the true count. Assumes no terminator flag, which holds for traversal paths. func storageNibbles(prefix []byte, nib *[4]byte) (n int) { - odd := prefix[0]&0x10 != 0 - n = 2*len(prefix) - 2 - 64 - if odd { - n++ + if len(prefix) < 33 { + return 0 } - if n > 4 { - return n + off := 2 + if prefix[0]&0x10 != 0 { // odd: the account hash starts at the low nibble of byte 0 + off = 1 } - if odd { - for i := 0; i < n; i++ { - b := prefix[32+(i+1)/2] - if i&1 == 0 { - nib[i] = b & 0x0f - } else { - nib[i] = b >> 4 - } - } + n = 2*len(prefix) - 64 - off + if n > 4 { return n } - for i := 0; i < n; i++ { - b := prefix[33+i/2] - if i&1 == 0 { + for i := range n { + j := 64 + i + off + if b := prefix[j/2]; j&1 == 0 { nib[i] = b >> 4 } else { nib[i] = b & 0x0f @@ -611,9 +592,9 @@ func (c *BranchCache) lookup(prefix []byte) (*branchCacheEntry, bool) { // Only a lookup that actually routes to a pinned trunk counts toward the // pinned hit/miss stats; account-trie and tail-only prefixes are excluded. var nibBuf [4]byte - if st, stor, ok := c.storageRoute(prefix, false, &nibBuf); ok { + if st, n, ok := c.storageRoute(prefix, false, &nibBuf); ok { var entry *branchCacheEntry - if slot := st.slot(stor, false); slot != nil { + if slot := st.slot(&nibBuf, n, false); slot != nil { entry = slot.Load() } else { entry, _ = st.deep.Get(prefix) @@ -652,14 +633,14 @@ func (c *BranchCache) store(prefix []byte, entry *branchCacheEntry) { // Keep a prefix already pinned in a storage trunk in place across the // per-block invalidate+Put refresh rather than dropping it to the tail. var nibBuf [4]byte - if st, stor, ok := c.storageRoute(prefix, false, &nibBuf); ok { - if slot := st.slot(stor, false); slot != nil { - if slot.Load() != nil { - slot.Store(entry) - return + if st, n, ok := c.storageRoute(prefix, false, &nibBuf); ok { + if slot := st.slot(&nibBuf, n, false); slot != nil { + for cur := slot.Load(); cur != nil; cur = slot.Load() { + if slot.CompareAndSwap(cur, entry) { + return + } } - } else if _, exists := st.deep.Get(prefix); exists { - st.deep.Set(prefix, entry) + } else if st.deep.ReplaceIfPresent(prefix, entry) { return } } @@ -683,12 +664,12 @@ func (c *BranchCache) PinEntry(prefix []byte, data []byte, step, txN uint64) { entry := &branchCacheEntry{data: dataCopy, step: step, txN: txN, epoch: c.coh.Epoch()} var nibBuf [4]byte - st, stor, ok := c.storageRoute(prefix, true, &nibBuf) + st, n, ok := c.storageRoute(prefix, true, &nibBuf) if !ok { c.tailForWrite().Add(maphash.Hash(prefix), entry) return } - if slot := st.slot(stor, true); slot != nil { + if slot := st.slot(&nibBuf, n, true); slot != nil { if slot.Load() == nil { c.pinnedEntries.Add(1) } @@ -770,8 +751,8 @@ func (c *BranchCache) Invalidate(prefix []byte) { return } var nibBuf [4]byte - if st, stor, ok := c.storageRoute(prefix, false, &nibBuf); ok { - if slot := st.slot(stor, false); slot != nil { + if st, n, ok := c.storageRoute(prefix, false, &nibBuf); ok { + if slot := st.slot(&nibBuf, n, false); slot != nil { if slot.Swap(nil) != nil { c.pinnedEntries.Add(-1) } diff --git a/execution/commitment/branch_cache_test.go b/execution/commitment/branch_cache_test.go index fc476ca70ca..64fc5eae39d 100644 --- a/execution/commitment/branch_cache_test.go +++ b/execution/commitment/branch_cache_test.go @@ -17,6 +17,7 @@ package commitment import ( + "fmt" "math/rand" "runtime" "strings" @@ -492,6 +493,7 @@ func TestStorageNibbles_MatchesReference(t *testing.T) { // allocations on a storage-tier lookup, for both odd and even flag parity. func TestBranchCache_StorageRoute_ZeroAlloc(t *testing.T) { c := NewBranchCache(100) + defer c.Close() even := make([]byte, 33) // even flag, storLen=0 → storage trunk's d0 for i := 1; i < 33; i++ { @@ -514,3 +516,39 @@ func TestBranchCache_StorageRoute_ZeroAlloc(t *testing.T) { require.Zerof(t, allocs, "storageRoute must not allocate on a storage-tier lookup, prefix0=%#x", prefix[0]) } } + +// A right-nibbles/wrong-count decode routes the pin and the read to different +// depths — a permanent miss the reference comparison cannot see. +func TestBranchCache_StorageTrunkRoundTripAcrossDepths(t *testing.T) { + for _, oddFlag := range []bool{false, true} { + for storLen := range 9 { + // storLen storage nibbles after 64 account nibbles, in compact form. + total := 64 + storLen + if oddFlag { + total++ + } + prefix := make([]byte, total/2+1) + if oddFlag { + prefix[0] = 0x10 + } + for i := 1; i < len(prefix); i++ { + prefix[i] = byte(i*11 + storLen) + } + + c := NewBranchCache(100) + want := fmt.Sprintf("d%d-%v", storLen, oddFlag) + c.PinEntry(prefix, []byte(want), 0, 100) + + got, _, ok := c.Get(prefix) + require.Truef(t, ok, "pinned entry must read back, storLen=%d odd=%v", storLen, oddFlag) + require.Equal(t, want, string(got)) + require.Equalf(t, 1, c.PinnedCount(), "storLen=%d odd=%v", storLen, oddFlag) + + c.Invalidate(prefix) + _, _, ok = c.Get(prefix) + require.Falsef(t, ok, "invalidated entry must be gone, storLen=%d odd=%v", storLen, oddFlag) + require.Zerof(t, c.PinnedCount(), "storLen=%d odd=%v", storLen, oddFlag) + c.Close() + } + } +} From 15a71dcba84782252cdf25721e6a479a0916c4af Mon Sep 17 00:00:00 2001 From: awskii Date: Wed, 12 Aug 2026 15:16:47 +0700 Subject: [PATCH 4/6] execution/commitment: compare the preload duration with ulp slack TestRecordPreload_RecordsElapsedAndBytes differences a float64 counter, which lands an ulp under the bound however much real time elapsed. It reports 0.04999999999999999 against a 0.05 lower bound. Whether it fires depends on the magnitude the shared accumulator already carries, so it turns on test order. --- execution/commitment/adaptive_pin_test.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/execution/commitment/adaptive_pin_test.go b/execution/commitment/adaptive_pin_test.go index 3335f1138d2..d092c79b7a7 100644 --- a/execution/commitment/adaptive_pin_test.go +++ b/execution/commitment/adaptive_pin_test.go @@ -136,7 +136,8 @@ func TestRecordPreload_RecordsElapsedAndBytes(t *testing.T) { if got := mxPreloadBytesTotal.GetValue() - bytesBefore; got != tc.wantBytes { t.Errorf("commitment_trunk_preload_bytes_total advanced by %v, want %v", got, tc.wantBytes) } - if got := mxPreloadDurationSecondsTotal.GetValue() - secondsBefore; got < elapsed.Seconds() { + const ulpSlack = 1e-9 // differencing a float64 accumulator lands just under + if got := mxPreloadDurationSecondsTotal.GetValue() - secondsBefore; got+ulpSlack < elapsed.Seconds() { t.Errorf("commitment_trunk_preload_duration_seconds_total advanced by %v, want >= %v", got, elapsed.Seconds()) } }) From 6ba5abcc77243e45b5f2f8d8edc5e0a56c397239 Mon Sep 17 00:00:00 2001 From: awskii Date: Wed, 12 Aug 2026 16:13:36 +0700 Subject: [PATCH 5/6] execution/commitment, common/maphash: close the pinned-entry counter race and keep the deep probe lock-free MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PinEntry checked slot occupancy before publishing, but Invalidate and the stale-read path evict without taking a put stripe. An eviction landing between the check and the store skips the +1 while the -1 still applies, so PinnedCount drifts permanently below the resident count and the evicted prefix is served again. Both tiers now publish and read the prior occupancy in one step (atomic.Pointer.Swap, maphash.LoadAndStore). ReplaceIfPresent goes through xsync.Map.Compute, which passes noLoadOp to doCompute and so skips the lock-free pre-scan and takes the bucket mutex even on a miss. The deep miss is the common case on the Put path — those prefixes fall through to the tail — so it keeps a lock-free Get probe and only calls ReplaceIfPresent on a hit. Invalidate's LoadAndDelete keeps the pre-scan and needs no probe. storageRoute now refuses terminator-flagged prefixes. storageNibbles derives the depth from the odd flag alone, so a terminator would make it one short and route the entry to a neighbouring slot; refusing sends the key to the tail instead. The oracle test masked the flag away, so it could not see this. Test fixes: a require failure in the depth round-trip left activeBranchCaches leaked for every later iteration, which drops d3/d4 from every subsequent BranchCache; storLen did not name the storage-nibble count it claimed to; the preload-duration slack was a constant where the error scales with the accumulator (27/200 failures at -count=200, 0/200 after); ReplaceIfPresent had no direct test; the zero-alloc route test did not cover create=true. --- common/maphash/maphash.go | 8 ++ common/maphash/maphash_test.go | 27 ++++++ execution/commitment/adaptive_pin_test.go | 8 +- execution/commitment/branch_cache.go | 27 +++--- execution/commitment/branch_cache_test.go | 112 ++++++++++++++++++---- p2p/protocols/eth/protocol_test.go | 18 ++-- 6 files changed, 153 insertions(+), 47 deletions(-) diff --git a/common/maphash/maphash.go b/common/maphash/maphash.go index 33513a77067..ae4d5bcbbec 100644 --- a/common/maphash/maphash.go +++ b/common/maphash/maphash.go @@ -65,6 +65,14 @@ func (m *Map[V]) ReplaceIfPresent(key []byte, value V) bool { return ok } +// LoadAndStore stores value and returns the previous one, loaded reporting +// prior presence. Probing with a separate Get first races a concurrent delete, +// so external counters must key off loaded. +func (m *Map[V]) LoadAndStore(key []byte, value V) (previous V, loaded bool) { + h := Hash(key) + return m.m.LoadAndStore(h, value) +} + // Delete removes a key from the map. func (m *Map[V]) Delete(key []byte) { h := Hash(key) diff --git a/common/maphash/maphash_test.go b/common/maphash/maphash_test.go index 1da10a1f588..66179feeb60 100644 --- a/common/maphash/maphash_test.go +++ b/common/maphash/maphash_test.go @@ -204,6 +204,33 @@ func TestMapOverwrite(t *testing.T) { } } +// The bool reports presence, not insertion; callers count entries off it. +func TestMapReplaceIfPresent(t *testing.T) { + SetSeed(42) + m := NewMap[string]() + + if m.ReplaceIfPresent([]byte("absent"), "v") { + t.Error("ReplaceIfPresent on an absent key must report false") + } + if _, ok := m.Get([]byte("absent")); ok { + t.Error("ReplaceIfPresent must not insert") + } + if m.Len() != 0 { + t.Errorf("expected len 0, got %d", m.Len()) + } + + m.Set([]byte("key"), "first") + if !m.ReplaceIfPresent([]byte("key"), "second") { + t.Error("ReplaceIfPresent on a present key must report true") + } + if v, ok := m.Get([]byte("key")); !ok || v != "second" { + t.Errorf("expected (second, true), got (%s, %v)", v, ok) + } + if m.Len() != 1 { + t.Errorf("expected len 1, got %d", m.Len()) + } +} + func TestMapEmptyKey(t *testing.T) { SetSeed(42) m := NewMap[int]() diff --git a/execution/commitment/adaptive_pin_test.go b/execution/commitment/adaptive_pin_test.go index d092c79b7a7..b27ed4420cb 100644 --- a/execution/commitment/adaptive_pin_test.go +++ b/execution/commitment/adaptive_pin_test.go @@ -18,6 +18,7 @@ package commitment import ( "context" + "math" "testing" "time" @@ -136,8 +137,11 @@ func TestRecordPreload_RecordsElapsedAndBytes(t *testing.T) { if got := mxPreloadBytesTotal.GetValue() - bytesBefore; got != tc.wantBytes { t.Errorf("commitment_trunk_preload_bytes_total advanced by %v, want %v", got, tc.wantBytes) } - const ulpSlack = 1e-9 // differencing a float64 accumulator lands just under - if got := mxPreloadDurationSecondsTotal.GetValue() - secondsBefore; got+ulpSlack < elapsed.Seconds() { + // Differencing a growing accumulator loses up to half an ULP of its + // magnitude, so a constant slack stops covering it as the total rises. + secondsAfter := mxPreloadDurationSecondsTotal.GetValue() + slack := math.Nextafter(secondsAfter, math.Inf(1)) - secondsAfter + if got := secondsAfter - secondsBefore; got+slack < elapsed.Seconds() { t.Errorf("commitment_trunk_preload_duration_seconds_total advanced by %v, want >= %v", got, elapsed.Seconds()) } }) diff --git a/execution/commitment/branch_cache.go b/execution/commitment/branch_cache.go index 7ea19b9bb10..4538e2e564a 100644 --- a/execution/commitment/branch_cache.go +++ b/execution/commitment/branch_cache.go @@ -256,11 +256,8 @@ func adaptiveTrunkDepth(active int64) uint8 { return trunkDepthShallow } -// slot returns the fixed-array slot for a nibble path of length 0-3 (and length -// 4 when the depth-4 array is present, i.e. the account trunk), or nil when the -// path is deeper — the caller then uses deep (storage) or the tail (account). // slot returns the cell for an n-nibble path, nil when that depth has no -// resident tier and the caller must use deep. +// resident tier and the caller must use deep (storage) or the tail (account). func (t *trunk) slot(path *[4]byte, n int, forWrite bool) *atomic.Pointer[branchCacheEntry] { switch n { case 0: @@ -429,7 +426,9 @@ func (c *BranchCache) trunkSlot(prefix []byte, forWrite bool) *atomic.Pointer[br // the caller falls through to the tail. nibBuf is caller-owned scratch — // storageRoute cannot inline, so a local would escape. func (c *BranchCache) storageRoute(prefix []byte, create bool, nibBuf *[4]byte) (st *trunk, n int, ok bool) { - if len(prefix) < 33 { + // A terminator adds a nibble storageNibbles does not count, so the depth would + // be one short and route to a neighbouring slot. Refusing sends it to the tail. + if len(prefix) < 33 || prefix[0]&0x20 != 0 { return nil, 0, false } // Both decodes stay behind this check; ahead of it they are pure cost. @@ -492,11 +491,9 @@ func ContractHashFromPrefix(prefix []byte) (hash [32]byte, ok bool) { // storageNibbles decodes the storage nibbles after the 64-nibble account hash, // matching nibbles.CompactToHex(prefix)[64:]. Only the first 4 are written; n is -// the true count. Assumes no terminator flag, which holds for traversal paths. +// the true count. Undefined for terminator-flagged prefixes; storageRoute +// rejects those before calling. func storageNibbles(prefix []byte, nib *[4]byte) (n int) { - if len(prefix) < 33 { - return 0 - } off := 2 if prefix[0]&0x10 != 0 { // odd: the account hash starts at the low nibble of byte 0 off = 1 @@ -640,7 +637,9 @@ func (c *BranchCache) store(prefix []byte, entry *branchCacheEntry) { return } } - } else if st.deep.ReplaceIfPresent(prefix, entry) { + } else if _, present := st.deep.Get(prefix); present && st.deep.ReplaceIfPresent(prefix, entry) { + // Get is lock-free; ReplaceIfPresent locks the bucket even on a miss, + // and the miss is the common case here. return } } @@ -669,17 +668,17 @@ func (c *BranchCache) PinEntry(prefix []byte, data []byte, step, txN uint64) { c.tailForWrite().Add(maphash.Hash(prefix), entry) return } + // Eviction takes no put stripe, so publish and read the prior occupancy in one + // step — a separate check would let an eviction land in between and lose a count. if slot := st.slot(&nibBuf, n, true); slot != nil { - if slot.Load() == nil { + if slot.Swap(entry) == nil { c.pinnedEntries.Add(1) } - slot.Store(entry) return } - if _, exists := st.deep.Get(prefix); !exists { + if _, loaded := st.deep.LoadAndStore(prefix, entry); !loaded { c.pinnedEntries.Add(1) } - st.deep.Set(prefix, entry) } // PinnedCount returns the number of currently pinned storage-trunk entries. diff --git a/execution/commitment/branch_cache_test.go b/execution/commitment/branch_cache_test.go index 64fc5eae39d..0e73e125a41 100644 --- a/execution/commitment/branch_cache_test.go +++ b/execution/commitment/branch_cache_test.go @@ -60,6 +60,48 @@ func TestBranchCache_AccountTrunkRouting(t *testing.T) { require.False(t, ok, "trunk entry with txN=100 must drop at unwind floor 60") } +// A pin that checked occupancy before writing could skip its +1 while a racing +// Invalidate's -1 still lands, leaving PinnedCount below the resident count. +func TestBranchCache_PinnedCountSurvivesConcurrentInvalidate(t *testing.T) { + newPrefix := func(contract byte, storageNibbles int) []byte { + p := make([]byte, 33+(storageNibbles+1)/2) + if storageNibbles%2 == 1 { + p[0] = 0x10 + } + p[1] = contract + for i := 2; i < len(p); i++ { + p[i] = byte(i * 3) + } + return p + } + + // The window only opens on an already-occupied slot, hence the pin per round. + for _, storageNibbles := range []int{0, 6} { + t.Run(fmt.Sprintf("depth%d", storageNibbles), func(t *testing.T) { + prefix := newPrefix(1, storageNibbles) + + for range 20000 { + c := NewBranchCache(100) + c.PinEntry(prefix, []byte("v0"), 0, 100) + + var wg sync.WaitGroup + wg.Go(func() { c.PinEntry(prefix, []byte("v1"), 0, 100) }) + wg.Go(func() { c.Invalidate(prefix) }) + wg.Wait() + + _, _, resident := c.Get(prefix) + want := 0 + if resident { + want = 1 + } + require.Equalf(t, want, c.PinnedCount(), + "PinnedCount must match residency (resident=%v)", resident) + c.Close() + } + }) + } +} + // TestBranchCache_StorageTrunkPin verifies PinEntry routes a storage-trunk // prefix (>= 64 nibbles) into its per-contract storage trunk, is served from // the pinned tier, counts toward PinnedCount, and honors the unwind model. @@ -488,6 +530,28 @@ func TestStorageNibbles_MatchesReference(t *testing.T) { } } +// Terminator-flagged keys must be refused a trunk route, not routed one slot short. +func TestBranchCache_StorageRouteRejectsTerminator(t *testing.T) { + prefix := make([]byte, 34) + for i := 1; i < len(prefix); i++ { + prefix[i] = byte(i) + } + for _, flag := range []byte{0x20, 0x30} { + prefix[0] = flag + c := NewBranchCache(100) + var nibBuf [4]byte + _, _, routed := c.storageRoute(prefix, true, &nibBuf) + require.Falsef(t, routed, "terminator-flagged prefix (%#x) must fall through to the tail", flag) + + // Still cached, just on the slower tier — a refused route is not a drop. + c.PinEntry(prefix, []byte("v"), 0, 100) + got, _, ok := c.Get(prefix) + require.Truef(t, ok, "terminator-flagged prefix (%#x) must still round-trip", flag) + require.Equal(t, []byte("v"), got) + c.Close() + } +} + // TestBranchCache_StorageRoute_ZeroAlloc verifies storageRoute's account-hash // and storage-nibble decode no longer pay the CompactToHex + packed-key // allocations on a storage-tier lookup, for both odd and even flag parity. @@ -514,41 +578,47 @@ func TestBranchCache_StorageRoute_ZeroAlloc(t *testing.T) { _, _, _ = c.storageRoute(prefix, false, &nibBuf) }) require.Zerof(t, allocs, "storageRoute must not allocate on a storage-tier lookup, prefix0=%#x", prefix[0]) + + // create=true reaches LoadOrStore; if that ever retained the key, escape + // analysis would heap-promote the 32-byte hash on both routes. + allocs = testing.AllocsPerRun(1000, func() { + var nibBuf [4]byte + _, _, _ = c.storageRoute(prefix, true, &nibBuf) + }) + require.Zerof(t, allocs, "storageRoute must not allocate routing to a resident contract, prefix0=%#x", prefix[0]) } } // A right-nibbles/wrong-count decode routes the pin and the read to different // depths — a permanent miss the reference comparison cannot see. func TestBranchCache_StorageTrunkRoundTripAcrossDepths(t *testing.T) { - for _, oddFlag := range []bool{false, true} { - for storLen := range 9 { - // storLen storage nibbles after 64 account nibbles, in compact form. - total := 64 + storLen - if oddFlag { - total++ - } - prefix := make([]byte, total/2+1) - if oddFlag { - prefix[0] = 0x10 - } - for i := 1; i < len(prefix); i++ { - prefix[i] = byte(i*11 + storLen) - } + for depth := range 9 { + // The parity of 64+depth fixes the compact odd flag: one encoding per depth. + total := 64 + depth + oddFlag := total%2 == 1 + prefix := make([]byte, total/2+1) + if oddFlag { + prefix[0] = 0x10 + } + for i := 1; i < len(prefix); i++ { + prefix[i] = byte(i*11 + depth) + } + t.Run(fmt.Sprintf("depth%d", depth), func(t *testing.T) { c := NewBranchCache(100) - want := fmt.Sprintf("d%d-%v", storLen, oddFlag) + defer c.Close() // a require failure here must not leak activeBranchCaches + want := fmt.Sprintf("d%d", depth) c.PinEntry(prefix, []byte(want), 0, 100) got, _, ok := c.Get(prefix) - require.Truef(t, ok, "pinned entry must read back, storLen=%d odd=%v", storLen, oddFlag) + require.Truef(t, ok, "pinned entry must read back, depth=%d", depth) require.Equal(t, want, string(got)) - require.Equalf(t, 1, c.PinnedCount(), "storLen=%d odd=%v", storLen, oddFlag) + require.Equalf(t, 1, c.PinnedCount(), "depth=%d", depth) c.Invalidate(prefix) _, _, ok = c.Get(prefix) - require.Falsef(t, ok, "invalidated entry must be gone, storLen=%d odd=%v", storLen, oddFlag) - require.Zerof(t, c.PinnedCount(), "storLen=%d odd=%v", storLen, oddFlag) - c.Close() - } + require.Falsef(t, ok, "invalidated entry must be gone, depth=%d", depth) + require.Zerof(t, c.PinnedCount(), "depth=%d", depth) + }) } } diff --git a/p2p/protocols/eth/protocol_test.go b/p2p/protocols/eth/protocol_test.go index 26c7215367f..d9f50c50704 100644 --- a/p2p/protocols/eth/protocol_test.go +++ b/p2p/protocols/eth/protocol_test.go @@ -412,15 +412,13 @@ func TestHashOrNumberEncodeRLPPointerIsAllocFree(t *testing.T) { } valueAllocs := testing.AllocsPerRun(200, mustEncode(hn.Hash)) pointerAllocs := testing.AllocsPerRun(200, mustEncode(&hn.Hash)) - if !race.Enabled { - t.Logf("allocs/op: byValue=%v byPointer=%v", valueAllocs, pointerAllocs) - if pointerAllocs >= valueAllocs { - t.Errorf("pointer form should allocate less: value=%v pointer=%v", valueAllocs, pointerAllocs) - } - // encBuffer is pooled, and sync.Pool drops values under the race detector. - //goland:noinspection GoBoolExpressions - if pointerAllocs != 0 { - t.Errorf("pointer form should not allocate, got %v", pointerAllocs) - } + t.Logf("allocs/op: byValue=%v byPointer=%v", valueAllocs, pointerAllocs) + if pointerAllocs >= valueAllocs { + t.Errorf("pointer form should allocate less: value=%v pointer=%v", valueAllocs, pointerAllocs) + } + // encBuffer is pooled, and sync.Pool drops values under the race detector. + //goland:noinspection GoBoolExpressions + if !race.Enabled && pointerAllocs != 0 { + t.Errorf("pointer form should not allocate, got %v", pointerAllocs) } } From 2ae85f9aed6da8ff78b0c0489afc77ea8d6bb3c4 Mon Sep 17 00:00:00 2001 From: Alexey Sharov Date: Wed, 12 Aug 2026 17:36:01 +0700 Subject: [PATCH 6/6] kick ci