diff --git a/common/maphash/maphash.go b/common/maphash/maphash.go index 0ef8fef863a..ae4d5bcbbec 100644 --- a/common/maphash/maphash.go +++ b/common/maphash/maphash.go @@ -52,6 +52,27 @@ 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 +} + +// 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 3335f1138d2..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,7 +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) } - if got := mxPreloadDurationSecondsTotal.GetValue() - secondsBefore; got < 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 c05e135f87e..4538e2e564a 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 @@ -257,11 +256,10 @@ 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). -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 (storage) or the tail (account). +func (t *trunk) slot(path *[4]byte, n int, forWrite bool) *atomic.Pointer[branchCacheEntry] { + switch n { case 0: return &t.d0 case 1: @@ -423,41 +421,37 @@ 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). 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 - } - // Nothing pinned and not creating: skip the CompactToHex + packed-key alloc - // that every >=64-nibble read would otherwise pay before finding no pins. - if !create && c.pinned.Load() == nil { - return nil, nil, nil, false +// 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) { + // 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 } - nib := nibbles.CompactToHex(prefix) - if len(nib) < 64 { - return nil, nil, nil, 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 } - packed := make([]byte, 32) - for i := range 32 { - packed[i] = nib[2*i]<<4 | nib[2*i+1] + acctHash, ok := ContractHashFromPrefix(prefix) + if !ok { + return nil, 0, false } - stor = nib[64:] - if p := c.pinned.Load(); p != nil { + packed := acctHash[:] + if p != nil { if st, found := p.Get(packed); found { - return st, packed, stor, true + return st, storageNibbles(prefix, nibBuf), true } } if !create { - return nil, packed, stor, false + return nil, 0, false } - st = newStorageTrunk(c.maxDepth) - c.pinnedForWrite().Set(packed, st) - return st, packed, 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. @@ -495,6 +489,30 @@ func ContractHashFromPrefix(prefix []byte) (hash [32]byte, ok bool) { return hash, true } +// 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. Undefined for terminator-flagged prefixes; storageRoute +// rejects those before calling. +func storageNibbles(prefix []byte, nib *[4]byte) (n int) { + off := 2 + if prefix[0]&0x10 != 0 { // odd: the account hash starts at the low nibble of byte 0 + off = 1 + } + n = 2*len(prefix) - 64 - off + if n > 4 { + return n + } + 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 + } + } + 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). @@ -570,9 +588,10 @@ 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, 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) @@ -610,14 +629,17 @@ 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 { - if slot := st.slot(stor, false); slot != nil { - if slot.Load() != nil { - slot.Store(entry) - return + var nibBuf [4]byte + 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 _, 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 } } @@ -640,22 +662,23 @@ 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, 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.Load() == nil { + // 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.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. @@ -726,13 +749,13 @@ func (c *BranchCache) Invalidate(prefix []byte) { slot.Store(nil) return } - if st, _, stor, ok := c.storageRoute(prefix, false); ok { - if slot := st.slot(stor, false); slot != nil { + var nibBuf [4]byte + 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) } - } 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..0e73e125a41 100644 --- a/execution/commitment/branch_cache_test.go +++ b/execution/commitment/branch_cache_test.go @@ -17,12 +17,16 @@ package commitment import ( + "fmt" + "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 @@ -56,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. @@ -449,3 +495,130 @@ 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]) + } + } +} + +// 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. +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++ { + 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]) + + // 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 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) + 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, depth=%d", depth) + require.Equal(t, want, string(got)) + 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, depth=%d", depth) + require.Zerof(t, c.PinnedCount(), "depth=%d", depth) + }) + } +}