From 424c2a5f584aacf71bee5fe724924c50034f962b Mon Sep 17 00:00:00 2001 From: Bashmunta Date: Tue, 30 Dec 2025 22:14:26 +0200 Subject: [PATCH 1/3] core/state/snapshot: fix StorageList memory accounting --- core/state/snapshot/difflayer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/state/snapshot/difflayer.go b/core/state/snapshot/difflayer.go index 28957051d446..1286ded7e1a5 100644 --- a/core/state/snapshot/difflayer.go +++ b/core/state/snapshot/difflayer.go @@ -465,6 +465,6 @@ func (dl *diffLayer) StorageList(accountHash common.Hash) []common.Hash { storageList := slices.SortedFunc(maps.Keys(dl.storageData[accountHash]), common.Hash.Cmp) dl.storageList[accountHash] = storageList - dl.memory += uint64(len(dl.storageList)*common.HashLength + common.HashLength) + dl.memory += uint64(len(storageList)*common.HashLength + common.HashLength) return storageList } From 02f6abc1f98538b300cb70f248b0175164871435 Mon Sep 17 00:00:00 2001 From: Bashmunta Date: Tue, 30 Dec 2025 22:14:38 +0200 Subject: [PATCH 2/3] add test --- core/state/snapshot/difflayer_test.go | 674 +++++++++++++++----------- 1 file changed, 395 insertions(+), 279 deletions(-) diff --git a/core/state/snapshot/difflayer_test.go b/core/state/snapshot/difflayer_test.go index 2c868b30106a..1286ded7e1a5 100644 --- a/core/state/snapshot/difflayer_test.go +++ b/core/state/snapshot/difflayer_test.go @@ -17,338 +17,454 @@ package snapshot import ( - "bytes" - crand "crypto/rand" + "encoding/binary" + "fmt" + "maps" + "math" "math/rand" - "testing" + "slices" + "sync" + "sync/atomic" + "time" - "github.com/VictoriaMetrics/fastcache" "github.com/ethereum/go-ethereum/common" - "github.com/ethereum/go-ethereum/crypto" - "github.com/ethereum/go-ethereum/ethdb/memorydb" + "github.com/ethereum/go-ethereum/core/types" + "github.com/ethereum/go-ethereum/rlp" + bloomfilter "github.com/holiman/bloomfilter/v2" ) -func copyAccounts(accounts map[common.Hash][]byte) map[common.Hash][]byte { - copy := make(map[common.Hash][]byte) - for hash, blob := range accounts { - copy[hash] = blob - } - return copy +var ( + // aggregatorMemoryLimit is the maximum size of the bottom-most diff layer + // that aggregates the writes from above until it's flushed into the disk + // layer. + // + // Note, bumping this up might drastically increase the size of the bloom + // filters that's stored in every diff layer. Don't do that without fully + // understanding all the implications. + aggregatorMemoryLimit = uint64(4 * 1024 * 1024) + + // aggregatorItemLimit is an approximate number of items that will end up + // in the aggregator layer before it's flushed out to disk. A plain account + // weighs around 14B (+hash), a storage slot 32B (+hash), a deleted slot + // 0B (+hash). Slots are mostly set/unset in lockstep, so that average at + // 16B (+hash). All in all, the average entry seems to be 15+32=47B. Use a + // smaller number to be on the safe side. + aggregatorItemLimit = aggregatorMemoryLimit / 42 + + // bloomTargetError is the target false positive rate when the aggregator + // layer is at its fullest. The actual value will probably move around up + // and down from this number, it's mostly a ballpark figure. + // + // Note, dropping this down might drastically increase the size of the bloom + // filters that's stored in every diff layer. Don't do that without fully + // understanding all the implications. + bloomTargetError = 0.02 + + // bloomSize is the ideal bloom filter size given the maximum number of items + // it's expected to hold and the target false positive error rate. + bloomSize = math.Ceil(float64(aggregatorItemLimit) * math.Log(bloomTargetError) / math.Log(1/math.Pow(2, math.Log(2)))) + + // bloomFuncs is the ideal number of bits a single entry should set in the + // bloom filter to keep its size to a minimum (given it's size and maximum + // entry count). + bloomFuncs = math.Round((bloomSize / float64(aggregatorItemLimit)) * math.Log(2)) + + // the bloom offsets are runtime constants which determines which part of the + // account/storage hash the hasher functions looks at, to determine the + // bloom key for an account/slot. This is randomized at init(), so that the + // global population of nodes do not all display the exact same behaviour with + // regards to bloom content + bloomAccountHasherOffset = 0 + bloomStorageHasherOffset = 0 +) + +func init() { + // Init the bloom offsets in the range [0:24] (requires 8 bytes) + bloomAccountHasherOffset = rand.Intn(25) + bloomStorageHasherOffset = rand.Intn(25) } -func copyStorage(storage map[common.Hash]map[common.Hash][]byte) map[common.Hash]map[common.Hash][]byte { - copy := make(map[common.Hash]map[common.Hash][]byte) - for accHash, slots := range storage { - copy[accHash] = make(map[common.Hash][]byte) - for slotHash, blob := range slots { - copy[accHash][slotHash] = blob - } - } - return copy +// diffLayer represents a collection of modifications made to a state snapshot +// after running a block on top. It contains one sorted list for the account trie +// and one-one list for each storage tries. +// +// The goal of a diff layer is to act as a journal, tracking recent modifications +// made to the state, that have not yet graduated into a semi-immutable state. +type diffLayer struct { + origin *diskLayer // Base disk layer to directly use on bloom misses + parent snapshot // Parent snapshot modified by this one, never nil + memory uint64 // Approximate guess as to how much memory we use + + root common.Hash // Root hash to which this snapshot diff belongs to + stale atomic.Bool // Signals that the layer became stale (state progressed) + + accountData map[common.Hash][]byte // Keyed accounts for direct retrieval (nil means deleted) + storageData map[common.Hash]map[common.Hash][]byte // Keyed storage slots for direct retrieval. one per account (nil means deleted) + accountList []common.Hash // List of account for iteration. If it exists, it's sorted, otherwise it's nil + storageList map[common.Hash][]common.Hash // List of storage slots for iterated retrievals, one per account. Any existing lists are sorted if non-nil + + diffed *bloomfilter.Filter // Bloom filter tracking all the diffed items up to the disk layer + + lock sync.RWMutex } -// TestMergeBasics tests some simple merges -func TestMergeBasics(t *testing.T) { - var ( - accounts = make(map[common.Hash][]byte) - storage = make(map[common.Hash]map[common.Hash][]byte) - ) - // Fill up a parent - for i := 0; i < 100; i++ { - h := randomHash() - data := randomAccount() - - accounts[h] = data - if rand.Intn(4) == 0 { - accounts[h] = nil - } - if rand.Intn(2) == 0 { - accStorage := make(map[common.Hash][]byte) - value := make([]byte, 32) - crand.Read(value) - accStorage[randomHash()] = value - storage[h] = accStorage - } +// accountBloomHash is used to convert an account hash into a 64 bit mini hash. +func accountBloomHash(h common.Hash) uint64 { + return binary.BigEndian.Uint64(h[bloomAccountHasherOffset : bloomAccountHasherOffset+8]) +} + +// storageBloomHash is used to convert an account hash and a storage hash into a 64 bit mini hash. +func storageBloomHash(h0, h1 common.Hash) uint64 { + return binary.BigEndian.Uint64(h0[bloomStorageHasherOffset:bloomStorageHasherOffset+8]) ^ + binary.BigEndian.Uint64(h1[bloomStorageHasherOffset:bloomStorageHasherOffset+8]) +} + +// newDiffLayer creates a new diff on top of an existing snapshot, whether that's a low +// level persistent database or a hierarchical diff already. +func newDiffLayer(parent snapshot, root common.Hash, accounts map[common.Hash][]byte, storage map[common.Hash]map[common.Hash][]byte) *diffLayer { + // Create the new layer with some pre-allocated data segments + dl := &diffLayer{ + parent: parent, + root: root, + accountData: accounts, + storageData: storage, + storageList: make(map[common.Hash][]common.Hash), } - // Add some (identical) layers on top - parent := newDiffLayer(emptyLayer(), common.Hash{}, copyAccounts(accounts), copyStorage(storage)) - child := newDiffLayer(parent, common.Hash{}, copyAccounts(accounts), copyStorage(storage)) - child = newDiffLayer(child, common.Hash{}, copyAccounts(accounts), copyStorage(storage)) - child = newDiffLayer(child, common.Hash{}, copyAccounts(accounts), copyStorage(storage)) - child = newDiffLayer(child, common.Hash{}, copyAccounts(accounts), copyStorage(storage)) - - // And flatten - merged := (child.flatten()).(*diffLayer) - - { // Check account lists - if have, want := len(merged.accountList), 0; have != want { - t.Errorf("accountList wrong: have %v, want %v", have, want) - } - if have, want := len(merged.AccountList()), len(accounts); have != want { - t.Errorf("AccountList() wrong: have %v, want %v", have, want) - } - if have, want := len(merged.accountList), len(accounts); have != want { - t.Errorf("accountList [2] wrong: have %v, want %v", have, want) - } + switch parent := parent.(type) { + case *diskLayer: + dl.rebloom(parent) + case *diffLayer: + dl.rebloom(parent.origin) + default: + panic("unknown parent type") } - { // Check storage lists - i := 0 - for aHash, sMap := range storage { - if have, want := len(merged.storageList), i; have != want { - t.Errorf("[1] storageList wrong: have %v, want %v", have, want) - } - list := merged.StorageList(aHash) - if have, want := len(list), len(sMap); have != want { - t.Errorf("[2] StorageList() wrong: have %v, want %v", have, want) - } - if have, want := len(merged.storageList[aHash]), len(sMap); have != want { - t.Errorf("storageList wrong: have %v, want %v", have, want) - } - i++ + // Sanity check that accounts or storage slots are never nil + for _, blob := range accounts { + // Determine memory size and track the dirty writes + dl.memory += uint64(common.HashLength + len(blob)) + snapshotDirtyAccountWriteMeter.Mark(int64(len(blob))) + } + for accountHash, slots := range storage { + if slots == nil { + panic(fmt.Sprintf("storage %#x nil", accountHash)) + } + // Determine memory size and track the dirty writes + for _, data := range slots { + dl.memory += uint64(common.HashLength + len(data)) + snapshotDirtyStorageWriteMeter.Mark(int64(len(data))) } } + return dl } -// TestMergeDelete tests some deletion -func TestMergeDelete(t *testing.T) { - storage := make(map[common.Hash]map[common.Hash][]byte) +// rebloom discards the layer's current bloom and rebuilds it from scratch based +// on the parent's and the local diffs. +func (dl *diffLayer) rebloom(origin *diskLayer) { + dl.lock.Lock() + defer dl.lock.Unlock() - // Fill up a parent - h1 := common.HexToHash("0x01") - h2 := common.HexToHash("0x02") + defer func(start time.Time) { + snapshotBloomIndexTimer.Update(time.Since(start)) + }(time.Now()) - flip := func() map[common.Hash][]byte { - return map[common.Hash][]byte{ - h1: randomAccount(), - h2: nil, - } - } - flop := func() map[common.Hash][]byte { - return map[common.Hash][]byte{ - h1: nil, - h2: randomAccount(), - } + // Inject the new origin that triggered the rebloom + dl.origin = origin + + // Retrieve the parent bloom or create a fresh empty one + if parent, ok := dl.parent.(*diffLayer); ok { + parent.lock.RLock() + dl.diffed, _ = parent.diffed.Copy() + parent.lock.RUnlock() + } else { + dl.diffed, _ = bloomfilter.New(uint64(bloomSize), uint64(bloomFuncs)) } - // Add some flipAccs-flopping layers on top - parent := newDiffLayer(emptyLayer(), common.Hash{}, flip(), storage) - child := parent.Update(common.Hash{}, flop(), storage) - child = child.Update(common.Hash{}, flip(), storage) - child = child.Update(common.Hash{}, flop(), storage) - child = child.Update(common.Hash{}, flip(), storage) - child = child.Update(common.Hash{}, flop(), storage) - child = child.Update(common.Hash{}, flip(), storage) - - if data, _ := child.Account(h1); data == nil { - t.Errorf("last diff layer: expected %x account to be non-nil", h1) + for hash := range dl.accountData { + dl.diffed.AddHash(accountBloomHash(hash)) } - if data, _ := child.Account(h2); data != nil { - t.Errorf("last diff layer: expected %x account to be nil", h2) + for accountHash, slots := range dl.storageData { + for storageHash := range slots { + dl.diffed.AddHash(storageBloomHash(accountHash, storageHash)) + } } + // Calculate the current false positive rate and update the error rate meter. + // This is a bit cheating because subsequent layers will overwrite it, but it + // should be fine, we're only interested in ballpark figures. + k := float64(dl.diffed.K()) + n := float64(dl.diffed.N()) + m := float64(dl.diffed.M()) + snapshotBloomErrorGauge.Update(math.Pow(1.0-math.Exp((-k)*(n+0.5)/(m-1)), k)) +} + +// Root returns the root hash for which this snapshot was made. +func (dl *diffLayer) Root() common.Hash { + return dl.root +} - // And flatten - merged := (child.flatten()).(*diffLayer) +// Parent returns the subsequent layer of a diff layer. +func (dl *diffLayer) Parent() snapshot { + dl.lock.RLock() + defer dl.lock.RUnlock() - if data, _ := merged.Account(h1); data == nil { - t.Errorf("merged layer: expected %x account to be non-nil", h1) + return dl.parent +} + +// Stale return whether this layer has become stale (was flattened across) or if +// it's still live. +func (dl *diffLayer) Stale() bool { + return dl.stale.Load() +} + +// Account directly retrieves the account associated with a particular hash in +// the snapshot slim data format. +func (dl *diffLayer) Account(hash common.Hash) (*types.SlimAccount, error) { + data, err := dl.AccountRLP(hash) + if err != nil { + return nil, err } - if data, _ := merged.Account(h2); data != nil { - t.Errorf("merged layer: expected %x account to be nil", h2) + if len(data) == 0 { // can be both nil and []byte{} + return nil, nil } - // If we add more granular metering of memory, we can enable this again, - // but it's not implemented for now - //if have, want := merged.memory, child.memory; have != want { - // t.Errorf("mem wrong: have %d, want %d", have, want) - //} + account := new(types.SlimAccount) + if err := rlp.DecodeBytes(data, account); err != nil { + panic(err) + } + return account, nil } -// This tests that if we create a new account, and set a slot, and then merge -// it, the lists will be correct. -func TestInsertAndMerge(t *testing.T) { - // Fill up a parent - var ( - acc = common.HexToHash("0x01") - slot = common.HexToHash("0x02") - parent *diffLayer - child *diffLayer - ) - { - var ( - accounts = make(map[common.Hash][]byte) - storage = make(map[common.Hash]map[common.Hash][]byte) - ) - parent = newDiffLayer(emptyLayer(), common.Hash{}, accounts, storage) +// AccountRLP directly retrieves the account RLP associated with a particular +// hash in the snapshot slim data format. +// +// Note the returned account is not a copy, please don't modify it. +func (dl *diffLayer) AccountRLP(hash common.Hash) ([]byte, error) { + // Check staleness before reaching further. + dl.lock.RLock() + if dl.Stale() { + dl.lock.RUnlock() + return nil, ErrSnapshotStale } - { - var ( - accounts = make(map[common.Hash][]byte) - storage = make(map[common.Hash]map[common.Hash][]byte) - ) - accounts[acc] = randomAccount() - storage[acc] = make(map[common.Hash][]byte) - storage[acc][slot] = []byte{0x01} - child = newDiffLayer(parent, common.Hash{}, accounts, storage) + // Check the bloom filter first whether there's even a point in reaching into + // all the maps in all the layers below + var origin *diskLayer + hit := dl.diffed.ContainsHash(accountBloomHash(hash)) + if !hit { + origin = dl.origin // extract origin while holding the lock } - // And flatten - merged := (child.flatten()).(*diffLayer) - { // Check that slot value is present - have, _ := merged.Storage(acc, slot) - if want := []byte{0x01}; !bytes.Equal(have, want) { - t.Errorf("merged slot value wrong: have %x, want %x", have, want) - } + dl.lock.RUnlock() + + // If the bloom filter misses, don't even bother with traversing the memory + // diff layers, reach straight into the bottom persistent disk layer + if origin != nil { + snapshotBloomAccountMissMeter.Mark(1) + return origin.AccountRLP(hash) } + // The bloom filter hit, start poking in the internal maps + return dl.accountRLP(hash, 0) } -func emptyLayer() *diskLayer { - return &diskLayer{ - diskdb: memorydb.New(), - cache: fastcache.New(500 * 1024), +// accountRLP is an internal version of AccountRLP that skips the bloom filter +// checks and uses the internal maps to try and retrieve the data. It's meant +// to be used if a higher layer's bloom filter hit already. +func (dl *diffLayer) accountRLP(hash common.Hash, depth int) ([]byte, error) { + dl.lock.RLock() + defer dl.lock.RUnlock() + + // If the layer was flattened into, consider it invalid (any live reference to + // the original should be marked as unusable). + if dl.Stale() { + return nil, ErrSnapshotStale + } + // If the account is known locally, return it + if data, ok := dl.accountData[hash]; ok { + snapshotDirtyAccountHitMeter.Mark(1) + snapshotDirtyAccountHitDepthHist.Update(int64(depth)) + if n := len(data); n > 0 { + snapshotDirtyAccountReadMeter.Mark(int64(n)) + } else { + snapshotDirtyAccountInexMeter.Mark(1) + } + snapshotBloomAccountTrueHitMeter.Mark(1) + return data, nil } + // Account unknown to this diff, resolve from parent + if diff, ok := dl.parent.(*diffLayer); ok { + return diff.accountRLP(hash, depth+1) + } + // Failed to resolve through diff layers, mark a bloom error and use the disk + snapshotBloomAccountFalseHitMeter.Mark(1) + return dl.parent.AccountRLP(hash) } -// BenchmarkSearch checks how long it takes to find a non-existing key -// BenchmarkSearch-6 200000 10481 ns/op (1K per layer) -// BenchmarkSearch-6 200000 10760 ns/op (10K per layer) -// BenchmarkSearch-6 100000 17866 ns/op +// Storage directly retrieves the storage data associated with a particular hash, +// within a particular account. If the slot is unknown to this diff, it's parent +// is consulted. // -// BenchmarkSearch-6 500000 3723 ns/op (10k per layer, only top-level RLock() -func BenchmarkSearch(b *testing.B) { - // First, we set up 128 diff layers, with 1K items each - fill := func(parent snapshot) *diffLayer { - var ( - accounts = make(map[common.Hash][]byte) - storage = make(map[common.Hash]map[common.Hash][]byte) - ) - for i := 0; i < 10000; i++ { - accounts[randomHash()] = randomAccount() - } - return newDiffLayer(parent, common.Hash{}, accounts, storage) +// Note the returned slot is not a copy, please don't modify it. +func (dl *diffLayer) Storage(accountHash, storageHash common.Hash) ([]byte, error) { + // Check the bloom filter first whether there's even a point in reaching into + // all the maps in all the layers below + dl.lock.RLock() + // Check staleness before reaching further. + if dl.Stale() { + dl.lock.RUnlock() + return nil, ErrSnapshotStale } - var layer snapshot - layer = emptyLayer() - for i := 0; i < 128; i++ { - layer = fill(layer) + var origin *diskLayer + hit := dl.diffed.ContainsHash(storageBloomHash(accountHash, storageHash)) + if !hit { + origin = dl.origin // extract origin while holding the lock } - key := crypto.Keccak256Hash([]byte{0x13, 0x38}) - for b.Loop() { - layer.AccountRLP(key) + dl.lock.RUnlock() + + // If the bloom filter misses, don't even bother with traversing the memory + // diff layers, reach straight into the bottom persistent disk layer + if origin != nil { + snapshotBloomStorageMissMeter.Mark(1) + return origin.Storage(accountHash, storageHash) } + // The bloom filter hit, start poking in the internal maps + return dl.storage(accountHash, storageHash, 0) } -// BenchmarkSearchSlot checks how long it takes to find a non-existing key -// - Number of layers: 128 -// - Each layers contains the account, with a couple of storage slots -// BenchmarkSearchSlot-6 100000 14554 ns/op -// BenchmarkSearchSlot-6 100000 22254 ns/op (when checking parent root using mutex) -// BenchmarkSearchSlot-6 100000 14551 ns/op (when checking parent number using atomic) -// With bloom filter: -// BenchmarkSearchSlot-6 3467835 351 ns/op -func BenchmarkSearchSlot(b *testing.B) { - // First, we set up 128 diff layers, with 1K items each - accountKey := crypto.Keccak256Hash([]byte{0x13, 0x37}) - storageKey := crypto.Keccak256Hash([]byte{0x13, 0x37}) - accountRLP := randomAccount() - fill := func(parent snapshot) *diffLayer { - var ( - accounts = make(map[common.Hash][]byte) - storage = make(map[common.Hash]map[common.Hash][]byte) - ) - accounts[accountKey] = accountRLP - - accStorage := make(map[common.Hash][]byte) - for i := 0; i < 5; i++ { - value := make([]byte, 32) - crand.Read(value) - accStorage[randomHash()] = value - storage[accountKey] = accStorage - } - return newDiffLayer(parent, common.Hash{}, accounts, storage) +// storage is an internal version of Storage that skips the bloom filter checks +// and uses the internal maps to try and retrieve the data. It's meant to be +// used if a higher layer's bloom filter hit already. +func (dl *diffLayer) storage(accountHash, storageHash common.Hash, depth int) ([]byte, error) { + dl.lock.RLock() + defer dl.lock.RUnlock() + + // If the layer was flattened into, consider it invalid (any live reference to + // the original should be marked as unusable). + if dl.Stale() { + return nil, ErrSnapshotStale } - var layer snapshot - layer = emptyLayer() - for i := 0; i < 128; i++ { - layer = fill(layer) + // If the account is known locally, try to resolve the slot locally + if storage, ok := dl.storageData[accountHash]; ok { + if data, ok := storage[storageHash]; ok { + snapshotDirtyStorageHitMeter.Mark(1) + snapshotDirtyStorageHitDepthHist.Update(int64(depth)) + if n := len(data); n > 0 { + snapshotDirtyStorageReadMeter.Mark(int64(n)) + } else { + snapshotDirtyStorageInexMeter.Mark(1) + } + snapshotBloomStorageTrueHitMeter.Mark(1) + return data, nil + } } - for b.Loop() { - layer.Storage(accountKey, storageKey) + // Storage slot unknown to this diff, resolve from parent + if diff, ok := dl.parent.(*diffLayer); ok { + return diff.storage(accountHash, storageHash, depth+1) } + // Failed to resolve through diff layers, mark a bloom error and use the disk + snapshotBloomStorageFalseHitMeter.Mark(1) + return dl.parent.Storage(accountHash, storageHash) } -// With accountList and sorting -// BenchmarkFlatten-6 50 29890856 ns/op -// -// Without sorting and tracking accountList -// BenchmarkFlatten-6 300 5511511 ns/op -func BenchmarkFlatten(b *testing.B) { - fill := func(parent snapshot) *diffLayer { - var ( - accounts = make(map[common.Hash][]byte) - storage = make(map[common.Hash]map[common.Hash][]byte) - ) - for i := 0; i < 100; i++ { - accountKey := randomHash() - accounts[accountKey] = randomAccount() - - accStorage := make(map[common.Hash][]byte) - for i := 0; i < 20; i++ { - value := make([]byte, 32) - crand.Read(value) - accStorage[randomHash()] = value - } - storage[accountKey] = accStorage - } - return newDiffLayer(parent, common.Hash{}, accounts, storage) +// Update creates a new layer on top of the existing snapshot diff tree with +// the specified data items. +func (dl *diffLayer) Update(blockRoot common.Hash, accounts map[common.Hash][]byte, storage map[common.Hash]map[common.Hash][]byte) *diffLayer { + return newDiffLayer(dl, blockRoot, accounts, storage) +} + +// flatten pushes all data from this point downwards, flattening everything into +// a single diff at the bottom. Since usually the lowermost diff is the largest, +// the flattening builds up from there in reverse. +func (dl *diffLayer) flatten() snapshot { + // If the parent is not diff, we're the first in line, return unmodified + parent, ok := dl.parent.(*diffLayer) + if !ok { + return dl } - for b.Loop() { - var layer snapshot - layer = emptyLayer() - for i := 1; i < 128; i++ { - layer = fill(layer) - } - b.StartTimer() + // Parent is a diff, flatten it first (note, apart from weird corned cases, + // flatten will realistically only ever merge 1 layer, so there's no need to + // be smarter about grouping flattens together). + parent = parent.flatten().(*diffLayer) - for i := 1; i < 128; i++ { - dl, ok := layer.(*diffLayer) - if !ok { - break - } - layer = dl.flatten() + parent.lock.Lock() + defer parent.lock.Unlock() + + // Before actually writing all our data to the parent, first ensure that the + // parent hasn't been 'corrupted' by someone else already flattening into it + if parent.stale.Swap(true) { + panic("parent diff layer is stale") // we've flattened into the same parent from two children, boo + } + for hash, data := range dl.accountData { + parent.accountData[hash] = data + } + // Overwrite all the updated storage slots (individually) + for accountHash, storage := range dl.storageData { + // If storage didn't exist (or was deleted) in the parent, overwrite blindly + if _, ok := parent.storageData[accountHash]; !ok { + parent.storageData[accountHash] = storage + continue } - b.StopTimer() + // Storage exists in both parent and child, merge the slots + maps.Copy(parent.storageData[accountHash], storage) + } + // Return the combo parent + return &diffLayer{ + parent: parent.parent, + origin: parent.origin, + root: dl.root, + accountData: parent.accountData, + storageData: parent.storageData, + storageList: make(map[common.Hash][]common.Hash), + diffed: dl.diffed, + memory: parent.memory + dl.memory, } } -// This test writes ~324M of diff layers to disk, spread over -// - 128 individual layers, -// - each with 200 accounts -// - containing 200 slots +// AccountList returns a sorted list of all accounts in this diffLayer, including +// the deleted ones. // -// BenchmarkJournal-6 1 1471373923 ns/ops -// BenchmarkJournal-6 1 1208083335 ns/op // bufio writer -func BenchmarkJournal(b *testing.B) { - fill := func(parent snapshot) *diffLayer { - var ( - accounts = make(map[common.Hash][]byte) - storage = make(map[common.Hash]map[common.Hash][]byte) - ) - for i := 0; i < 200; i++ { - accountKey := randomHash() - accounts[accountKey] = randomAccount() - - accStorage := make(map[common.Hash][]byte) - for i := 0; i < 200; i++ { - value := make([]byte, 32) - crand.Read(value) - accStorage[randomHash()] = value - } - storage[accountKey] = accStorage - } - return newDiffLayer(parent, common.Hash{}, accounts, storage) +// Note, the returned slice is not a copy, so do not modify it. +func (dl *diffLayer) AccountList() []common.Hash { + // If an old list already exists, return it + dl.lock.RLock() + list := dl.accountList + dl.lock.RUnlock() + + if list != nil { + return list } - layer := snapshot(emptyLayer()) - for i := 1; i < 128; i++ { - layer = fill(layer) + // No old sorted account list exists, generate a new one + dl.lock.Lock() + defer dl.lock.Unlock() + + dl.accountList = slices.SortedFunc(maps.Keys(dl.accountData), common.Hash.Cmp) + dl.memory += uint64(len(dl.accountList) * common.HashLength) + return dl.accountList +} + +// StorageList returns a sorted list of all storage slot hashes in this diffLayer +// for the given account. If the whole storage is destructed in this layer, then +// an additional flag *destructed = true* will be returned, otherwise the flag is +// false. Besides, the returned list will include the hash of deleted storage slot. +// Note a special case is an account is deleted in a prior tx but is recreated in +// the following tx with some storage slots set. In this case the returned list is +// not empty but the flag is true. +// +// Note, the returned slice is not a copy, so do not modify it. +func (dl *diffLayer) StorageList(accountHash common.Hash) []common.Hash { + dl.lock.RLock() + if _, ok := dl.storageData[accountHash]; !ok { + // Account not tracked by this layer + dl.lock.RUnlock() + return nil } - for b.Loop() { - layer.Journal(new(bytes.Buffer)) + // If an old list already exists, return it + if list, exist := dl.storageList[accountHash]; exist { + dl.lock.RUnlock() + return list // the cached list can't be nil } + dl.lock.RUnlock() + + // No old sorted account list exists, generate a new one + dl.lock.Lock() + defer dl.lock.Unlock() + + storageList := slices.SortedFunc(maps.Keys(dl.storageData[accountHash]), common.Hash.Cmp) + dl.storageList[accountHash] = storageList + dl.memory += uint64(len(storageList)*common.HashLength + common.HashLength) + return storageList } From db7410407fadcaf97f1b27b7e32bb1b62bb6ef92 Mon Sep 17 00:00:00 2001 From: Bashmunta Date: Tue, 30 Dec 2025 22:15:40 +0200 Subject: [PATCH 3/3] fix copy mistake --- core/state/snapshot/difflayer_test.go | 697 ++++++++++++-------------- 1 file changed, 307 insertions(+), 390 deletions(-) diff --git a/core/state/snapshot/difflayer_test.go b/core/state/snapshot/difflayer_test.go index 1286ded7e1a5..90a265645d9b 100644 --- a/core/state/snapshot/difflayer_test.go +++ b/core/state/snapshot/difflayer_test.go @@ -17,454 +17,371 @@ package snapshot import ( - "encoding/binary" - "fmt" - "maps" - "math" + "bytes" + crand "crypto/rand" "math/rand" - "slices" - "sync" - "sync/atomic" - "time" + "testing" + "github.com/VictoriaMetrics/fastcache" "github.com/ethereum/go-ethereum/common" - "github.com/ethereum/go-ethereum/core/types" - "github.com/ethereum/go-ethereum/rlp" - bloomfilter "github.com/holiman/bloomfilter/v2" + "github.com/ethereum/go-ethereum/crypto" + "github.com/ethereum/go-ethereum/ethdb/memorydb" ) -var ( - // aggregatorMemoryLimit is the maximum size of the bottom-most diff layer - // that aggregates the writes from above until it's flushed into the disk - // layer. - // - // Note, bumping this up might drastically increase the size of the bloom - // filters that's stored in every diff layer. Don't do that without fully - // understanding all the implications. - aggregatorMemoryLimit = uint64(4 * 1024 * 1024) - - // aggregatorItemLimit is an approximate number of items that will end up - // in the aggregator layer before it's flushed out to disk. A plain account - // weighs around 14B (+hash), a storage slot 32B (+hash), a deleted slot - // 0B (+hash). Slots are mostly set/unset in lockstep, so that average at - // 16B (+hash). All in all, the average entry seems to be 15+32=47B. Use a - // smaller number to be on the safe side. - aggregatorItemLimit = aggregatorMemoryLimit / 42 - - // bloomTargetError is the target false positive rate when the aggregator - // layer is at its fullest. The actual value will probably move around up - // and down from this number, it's mostly a ballpark figure. - // - // Note, dropping this down might drastically increase the size of the bloom - // filters that's stored in every diff layer. Don't do that without fully - // understanding all the implications. - bloomTargetError = 0.02 - - // bloomSize is the ideal bloom filter size given the maximum number of items - // it's expected to hold and the target false positive error rate. - bloomSize = math.Ceil(float64(aggregatorItemLimit) * math.Log(bloomTargetError) / math.Log(1/math.Pow(2, math.Log(2)))) - - // bloomFuncs is the ideal number of bits a single entry should set in the - // bloom filter to keep its size to a minimum (given it's size and maximum - // entry count). - bloomFuncs = math.Round((bloomSize / float64(aggregatorItemLimit)) * math.Log(2)) - - // the bloom offsets are runtime constants which determines which part of the - // account/storage hash the hasher functions looks at, to determine the - // bloom key for an account/slot. This is randomized at init(), so that the - // global population of nodes do not all display the exact same behaviour with - // regards to bloom content - bloomAccountHasherOffset = 0 - bloomStorageHasherOffset = 0 -) - -func init() { - // Init the bloom offsets in the range [0:24] (requires 8 bytes) - bloomAccountHasherOffset = rand.Intn(25) - bloomStorageHasherOffset = rand.Intn(25) -} - -// diffLayer represents a collection of modifications made to a state snapshot -// after running a block on top. It contains one sorted list for the account trie -// and one-one list for each storage tries. -// -// The goal of a diff layer is to act as a journal, tracking recent modifications -// made to the state, that have not yet graduated into a semi-immutable state. -type diffLayer struct { - origin *diskLayer // Base disk layer to directly use on bloom misses - parent snapshot // Parent snapshot modified by this one, never nil - memory uint64 // Approximate guess as to how much memory we use - - root common.Hash // Root hash to which this snapshot diff belongs to - stale atomic.Bool // Signals that the layer became stale (state progressed) - - accountData map[common.Hash][]byte // Keyed accounts for direct retrieval (nil means deleted) - storageData map[common.Hash]map[common.Hash][]byte // Keyed storage slots for direct retrieval. one per account (nil means deleted) - accountList []common.Hash // List of account for iteration. If it exists, it's sorted, otherwise it's nil - storageList map[common.Hash][]common.Hash // List of storage slots for iterated retrievals, one per account. Any existing lists are sorted if non-nil - - diffed *bloomfilter.Filter // Bloom filter tracking all the diffed items up to the disk layer - - lock sync.RWMutex -} - -// accountBloomHash is used to convert an account hash into a 64 bit mini hash. -func accountBloomHash(h common.Hash) uint64 { - return binary.BigEndian.Uint64(h[bloomAccountHasherOffset : bloomAccountHasherOffset+8]) +func copyAccounts(accounts map[common.Hash][]byte) map[common.Hash][]byte { + copy := make(map[common.Hash][]byte) + for hash, blob := range accounts { + copy[hash] = blob + } + return copy } -// storageBloomHash is used to convert an account hash and a storage hash into a 64 bit mini hash. -func storageBloomHash(h0, h1 common.Hash) uint64 { - return binary.BigEndian.Uint64(h0[bloomStorageHasherOffset:bloomStorageHasherOffset+8]) ^ - binary.BigEndian.Uint64(h1[bloomStorageHasherOffset:bloomStorageHasherOffset+8]) +func copyStorage(storage map[common.Hash]map[common.Hash][]byte) map[common.Hash]map[common.Hash][]byte { + copy := make(map[common.Hash]map[common.Hash][]byte) + for accHash, slots := range storage { + copy[accHash] = make(map[common.Hash][]byte) + for slotHash, blob := range slots { + copy[accHash][slotHash] = blob + } + } + return copy } -// newDiffLayer creates a new diff on top of an existing snapshot, whether that's a low -// level persistent database or a hierarchical diff already. -func newDiffLayer(parent snapshot, root common.Hash, accounts map[common.Hash][]byte, storage map[common.Hash]map[common.Hash][]byte) *diffLayer { - // Create the new layer with some pre-allocated data segments - dl := &diffLayer{ - parent: parent, - root: root, - accountData: accounts, - storageData: storage, - storageList: make(map[common.Hash][]common.Hash), - } - switch parent := parent.(type) { - case *diskLayer: - dl.rebloom(parent) - case *diffLayer: - dl.rebloom(parent.origin) - default: - panic("unknown parent type") - } - // Sanity check that accounts or storage slots are never nil - for _, blob := range accounts { - // Determine memory size and track the dirty writes - dl.memory += uint64(common.HashLength + len(blob)) - snapshotDirtyAccountWriteMeter.Mark(int64(len(blob))) +// TestMergeBasics tests some simple merges +func TestMergeBasics(t *testing.T) { + var ( + accounts = make(map[common.Hash][]byte) + storage = make(map[common.Hash]map[common.Hash][]byte) + ) + // Fill up a parent + for i := 0; i < 100; i++ { + h := randomHash() + data := randomAccount() + + accounts[h] = data + if rand.Intn(4) == 0 { + accounts[h] = nil + } + if rand.Intn(2) == 0 { + accStorage := make(map[common.Hash][]byte) + value := make([]byte, 32) + crand.Read(value) + accStorage[randomHash()] = value + storage[h] = accStorage + } } - for accountHash, slots := range storage { - if slots == nil { - panic(fmt.Sprintf("storage %#x nil", accountHash)) + // Add some (identical) layers on top + parent := newDiffLayer(emptyLayer(), common.Hash{}, copyAccounts(accounts), copyStorage(storage)) + child := newDiffLayer(parent, common.Hash{}, copyAccounts(accounts), copyStorage(storage)) + child = newDiffLayer(child, common.Hash{}, copyAccounts(accounts), copyStorage(storage)) + child = newDiffLayer(child, common.Hash{}, copyAccounts(accounts), copyStorage(storage)) + child = newDiffLayer(child, common.Hash{}, copyAccounts(accounts), copyStorage(storage)) + + // And flatten + merged := (child.flatten()).(*diffLayer) + + { // Check account lists + if have, want := len(merged.accountList), 0; have != want { + t.Errorf("accountList wrong: have %v, want %v", have, want) + } + if have, want := len(merged.AccountList()), len(accounts); have != want { + t.Errorf("AccountList() wrong: have %v, want %v", have, want) } - // Determine memory size and track the dirty writes - for _, data := range slots { - dl.memory += uint64(common.HashLength + len(data)) - snapshotDirtyStorageWriteMeter.Mark(int64(len(data))) + if have, want := len(merged.accountList), len(accounts); have != want { + t.Errorf("accountList [2] wrong: have %v, want %v", have, want) + } + } + { // Check storage lists + i := 0 + for aHash, sMap := range storage { + if have, want := len(merged.storageList), i; have != want { + t.Errorf("[1] storageList wrong: have %v, want %v", have, want) + } + list := merged.StorageList(aHash) + if have, want := len(list), len(sMap); have != want { + t.Errorf("[2] StorageList() wrong: have %v, want %v", have, want) + } + if have, want := len(merged.storageList[aHash]), len(sMap); have != want { + t.Errorf("storageList wrong: have %v, want %v", have, want) + } + i++ } } - return dl } -// rebloom discards the layer's current bloom and rebuilds it from scratch based -// on the parent's and the local diffs. -func (dl *diffLayer) rebloom(origin *diskLayer) { - dl.lock.Lock() - defer dl.lock.Unlock() +// TestMergeDelete tests some deletion +func TestMergeDelete(t *testing.T) { + storage := make(map[common.Hash]map[common.Hash][]byte) - defer func(start time.Time) { - snapshotBloomIndexTimer.Update(time.Since(start)) - }(time.Now()) + // Fill up a parent + h1 := common.HexToHash("0x01") + h2 := common.HexToHash("0x02") - // Inject the new origin that triggered the rebloom - dl.origin = origin - - // Retrieve the parent bloom or create a fresh empty one - if parent, ok := dl.parent.(*diffLayer); ok { - parent.lock.RLock() - dl.diffed, _ = parent.diffed.Copy() - parent.lock.RUnlock() - } else { - dl.diffed, _ = bloomfilter.New(uint64(bloomSize), uint64(bloomFuncs)) - } - for hash := range dl.accountData { - dl.diffed.AddHash(accountBloomHash(hash)) + flip := func() map[common.Hash][]byte { + return map[common.Hash][]byte{ + h1: randomAccount(), + h2: nil, + } } - for accountHash, slots := range dl.storageData { - for storageHash := range slots { - dl.diffed.AddHash(storageBloomHash(accountHash, storageHash)) + flop := func() map[common.Hash][]byte { + return map[common.Hash][]byte{ + h1: nil, + h2: randomAccount(), } } - // Calculate the current false positive rate and update the error rate meter. - // This is a bit cheating because subsequent layers will overwrite it, but it - // should be fine, we're only interested in ballpark figures. - k := float64(dl.diffed.K()) - n := float64(dl.diffed.N()) - m := float64(dl.diffed.M()) - snapshotBloomErrorGauge.Update(math.Pow(1.0-math.Exp((-k)*(n+0.5)/(m-1)), k)) -} - -// Root returns the root hash for which this snapshot was made. -func (dl *diffLayer) Root() common.Hash { - return dl.root -} - -// Parent returns the subsequent layer of a diff layer. -func (dl *diffLayer) Parent() snapshot { - dl.lock.RLock() - defer dl.lock.RUnlock() - - return dl.parent -} + // Add some flipAccs-flopping layers on top + parent := newDiffLayer(emptyLayer(), common.Hash{}, flip(), storage) + child := parent.Update(common.Hash{}, flop(), storage) + child = child.Update(common.Hash{}, flip(), storage) + child = child.Update(common.Hash{}, flop(), storage) + child = child.Update(common.Hash{}, flip(), storage) + child = child.Update(common.Hash{}, flop(), storage) + child = child.Update(common.Hash{}, flip(), storage) + + if data, _ := child.Account(h1); data == nil { + t.Errorf("last diff layer: expected %x account to be non-nil", h1) + } + if data, _ := child.Account(h2); data != nil { + t.Errorf("last diff layer: expected %x account to be nil", h2) + } -// Stale return whether this layer has become stale (was flattened across) or if -// it's still live. -func (dl *diffLayer) Stale() bool { - return dl.stale.Load() -} + // And flatten + merged := (child.flatten()).(*diffLayer) -// Account directly retrieves the account associated with a particular hash in -// the snapshot slim data format. -func (dl *diffLayer) Account(hash common.Hash) (*types.SlimAccount, error) { - data, err := dl.AccountRLP(hash) - if err != nil { - return nil, err + if data, _ := merged.Account(h1); data == nil { + t.Errorf("merged layer: expected %x account to be non-nil", h1) } - if len(data) == 0 { // can be both nil and []byte{} - return nil, nil + if data, _ := merged.Account(h2); data != nil { + t.Errorf("merged layer: expected %x account to be nil", h2) } - account := new(types.SlimAccount) - if err := rlp.DecodeBytes(data, account); err != nil { - panic(err) - } - return account, nil + // If we add more granular metering of memory, we can enable this again, + // but it's not implemented for now + //if have, want := merged.memory, child.memory; have != want { + // t.Errorf("mem wrong: have %d, want %d", have, want) + //} } -// AccountRLP directly retrieves the account RLP associated with a particular -// hash in the snapshot slim data format. -// -// Note the returned account is not a copy, please don't modify it. -func (dl *diffLayer) AccountRLP(hash common.Hash) ([]byte, error) { - // Check staleness before reaching further. - dl.lock.RLock() - if dl.Stale() { - dl.lock.RUnlock() - return nil, ErrSnapshotStale +// This tests that if we create a new account, and set a slot, and then merge +// it, the lists will be correct. +func TestInsertAndMerge(t *testing.T) { + // Fill up a parent + var ( + acc = common.HexToHash("0x01") + slot = common.HexToHash("0x02") + parent *diffLayer + child *diffLayer + ) + { + var ( + accounts = make(map[common.Hash][]byte) + storage = make(map[common.Hash]map[common.Hash][]byte) + ) + parent = newDiffLayer(emptyLayer(), common.Hash{}, accounts, storage) } - // Check the bloom filter first whether there's even a point in reaching into - // all the maps in all the layers below - var origin *diskLayer - hit := dl.diffed.ContainsHash(accountBloomHash(hash)) - if !hit { - origin = dl.origin // extract origin while holding the lock + { + var ( + accounts = make(map[common.Hash][]byte) + storage = make(map[common.Hash]map[common.Hash][]byte) + ) + accounts[acc] = randomAccount() + storage[acc] = make(map[common.Hash][]byte) + storage[acc][slot] = []byte{0x01} + child = newDiffLayer(parent, common.Hash{}, accounts, storage) } - dl.lock.RUnlock() - - // If the bloom filter misses, don't even bother with traversing the memory - // diff layers, reach straight into the bottom persistent disk layer - if origin != nil { - snapshotBloomAccountMissMeter.Mark(1) - return origin.AccountRLP(hash) + // And flatten + merged := (child.flatten()).(*diffLayer) + { // Check that slot value is present + have, _ := merged.Storage(acc, slot) + if want := []byte{0x01}; !bytes.Equal(have, want) { + t.Errorf("merged slot value wrong: have %x, want %x", have, want) + } } - // The bloom filter hit, start poking in the internal maps - return dl.accountRLP(hash, 0) } -// accountRLP is an internal version of AccountRLP that skips the bloom filter -// checks and uses the internal maps to try and retrieve the data. It's meant -// to be used if a higher layer's bloom filter hit already. -func (dl *diffLayer) accountRLP(hash common.Hash, depth int) ([]byte, error) { - dl.lock.RLock() - defer dl.lock.RUnlock() +// TestStorageListMemoryAccounting ensures that StorageList increases dl.memory +// proportionally to the number of storage slots in the requested account and +// does not change memory usage on repeated calls for the same account. +func TestStorageListMemoryAccounting(t *testing.T) { + parent := newDiffLayer(emptyLayer(), common.Hash{}, nil, nil) + account := common.HexToHash("0x01") - // If the layer was flattened into, consider it invalid (any live reference to - // the original should be marked as unusable). - if dl.Stale() { - return nil, ErrSnapshotStale + slots := make(map[common.Hash][]byte) + for i := 0; i < 3; i++ { + slots[randomHash()] = []byte{0x01} } - // If the account is known locally, return it - if data, ok := dl.accountData[hash]; ok { - snapshotDirtyAccountHitMeter.Mark(1) - snapshotDirtyAccountHitDepthHist.Update(int64(depth)) - if n := len(data); n > 0 { - snapshotDirtyAccountReadMeter.Mark(int64(n)) - } else { - snapshotDirtyAccountInexMeter.Mark(1) - } - snapshotBloomAccountTrueHitMeter.Mark(1) - return data, nil + storage := map[common.Hash]map[common.Hash][]byte{ + account: slots, } - // Account unknown to this diff, resolve from parent - if diff, ok := dl.parent.(*diffLayer); ok { - return diff.accountRLP(hash, depth+1) - } - // Failed to resolve through diff layers, mark a bloom error and use the disk - snapshotBloomAccountFalseHitMeter.Mark(1) - return dl.parent.AccountRLP(hash) -} + dl := newDiffLayer(parent, common.Hash{}, nil, storage) -// Storage directly retrieves the storage data associated with a particular hash, -// within a particular account. If the slot is unknown to this diff, it's parent -// is consulted. -// -// Note the returned slot is not a copy, please don't modify it. -func (dl *diffLayer) Storage(accountHash, storageHash common.Hash) ([]byte, error) { - // Check the bloom filter first whether there's even a point in reaching into - // all the maps in all the layers below - dl.lock.RLock() - // Check staleness before reaching further. - if dl.Stale() { - dl.lock.RUnlock() - return nil, ErrSnapshotStale + before := dl.memory + list := dl.StorageList(account) + if have, want := len(list), len(slots); have != want { + t.Fatalf("StorageList length mismatch: have %d, want %d", have, want) } - var origin *diskLayer - hit := dl.diffed.ContainsHash(storageBloomHash(accountHash, storageHash)) - if !hit { - origin = dl.origin // extract origin while holding the lock + expectedDelta := uint64(len(list)*common.HashLength + common.HashLength) + if have, want := dl.memory-before, expectedDelta; have != want { + t.Fatalf("StorageList memory delta mismatch: have %d, want %d", have, want) } - dl.lock.RUnlock() - // If the bloom filter misses, don't even bother with traversing the memory - // diff layers, reach straight into the bottom persistent disk layer - if origin != nil { - snapshotBloomStorageMissMeter.Mark(1) - return origin.Storage(accountHash, storageHash) + before = dl.memory + _ = dl.StorageList(account) + if dl.memory != before { + t.Fatalf("StorageList changed memory on cached call: have %d, want %d", dl.memory, before) } - // The bloom filter hit, start poking in the internal maps - return dl.storage(accountHash, storageHash, 0) } -// storage is an internal version of Storage that skips the bloom filter checks -// and uses the internal maps to try and retrieve the data. It's meant to be -// used if a higher layer's bloom filter hit already. -func (dl *diffLayer) storage(accountHash, storageHash common.Hash, depth int) ([]byte, error) { - dl.lock.RLock() - defer dl.lock.RUnlock() - - // If the layer was flattened into, consider it invalid (any live reference to - // the original should be marked as unusable). - if dl.Stale() { - return nil, ErrSnapshotStale - } - // If the account is known locally, try to resolve the slot locally - if storage, ok := dl.storageData[accountHash]; ok { - if data, ok := storage[storageHash]; ok { - snapshotDirtyStorageHitMeter.Mark(1) - snapshotDirtyStorageHitDepthHist.Update(int64(depth)) - if n := len(data); n > 0 { - snapshotDirtyStorageReadMeter.Mark(int64(n)) - } else { - snapshotDirtyStorageInexMeter.Mark(1) - } - snapshotBloomStorageTrueHitMeter.Mark(1) - return data, nil - } +func emptyLayer() *diskLayer { + return &diskLayer{ + diskdb: memorydb.New(), + cache: fastcache.New(500 * 1024), } - // Storage slot unknown to this diff, resolve from parent - if diff, ok := dl.parent.(*diffLayer); ok { - return diff.storage(accountHash, storageHash, depth+1) - } - // Failed to resolve through diff layers, mark a bloom error and use the disk - snapshotBloomStorageFalseHitMeter.Mark(1) - return dl.parent.Storage(accountHash, storageHash) -} - -// Update creates a new layer on top of the existing snapshot diff tree with -// the specified data items. -func (dl *diffLayer) Update(blockRoot common.Hash, accounts map[common.Hash][]byte, storage map[common.Hash]map[common.Hash][]byte) *diffLayer { - return newDiffLayer(dl, blockRoot, accounts, storage) } -// flatten pushes all data from this point downwards, flattening everything into -// a single diff at the bottom. Since usually the lowermost diff is the largest, -// the flattening builds up from there in reverse. -func (dl *diffLayer) flatten() snapshot { - // If the parent is not diff, we're the first in line, return unmodified - parent, ok := dl.parent.(*diffLayer) - if !ok { - return dl +// BenchmarkSearch checks how long it takes to find a non-existing key +// BenchmarkSearch-6 200000 10481 ns/op (1K per layer) +// BenchmarkSearch-6 200000 10760 ns/op (10K per layer) +// BenchmarkSearch-6 100000 17866 ns/op +// +// BenchmarkSearch-6 500000 3723 ns/op (10k per layer, only top-level RLock() +func BenchmarkSearch(b *testing.B) { + // First, we set up 128 diff layers, with 1K items each + fill := func(parent snapshot) *diffLayer { + var ( + accounts = make(map[common.Hash][]byte) + storage = make(map[common.Hash]map[common.Hash][]byte) + ) + for i := 0; i < 10000; i++ { + accounts[randomHash()] = randomAccount() + } + return newDiffLayer(parent, common.Hash{}, accounts, storage) } - // Parent is a diff, flatten it first (note, apart from weird corned cases, - // flatten will realistically only ever merge 1 layer, so there's no need to - // be smarter about grouping flattens together). - parent = parent.flatten().(*diffLayer) - - parent.lock.Lock() - defer parent.lock.Unlock() - - // Before actually writing all our data to the parent, first ensure that the - // parent hasn't been 'corrupted' by someone else already flattening into it - if parent.stale.Swap(true) { - panic("parent diff layer is stale") // we've flattened into the same parent from two children, boo + var layer snapshot + layer = emptyLayer() + for i := 0; i < 128; i++ { + layer = fill(layer) } - for hash, data := range dl.accountData { - parent.accountData[hash] = data + key := crypto.Keccak256Hash([]byte{0x13, 0x38}) + for b.Loop() { + layer.AccountRLP(key) } - // Overwrite all the updated storage slots (individually) - for accountHash, storage := range dl.storageData { - // If storage didn't exist (or was deleted) in the parent, overwrite blindly - if _, ok := parent.storageData[accountHash]; !ok { - parent.storageData[accountHash] = storage - continue +} + +// BenchmarkSearchSlot checks how long it takes to find a non-existing key +// - Number of layers: 128 +// - Each layers contains the account, with a couple of storage slots +// BenchmarkSearchSlot-6 100000 14554 ns/op +// BenchmarkSearchSlot-6 100000 22254 ns/op (when checking parent root using mutex) +// BenchmarkSearchSlot-6 100000 14551 ns/op (when checking parent number using atomic) +// With bloom filter: +// BenchmarkSearchSlot-6 3467835 351 ns/op +func BenchmarkSearchSlot(b *testing.B) { + // First, we set up 128 diff layers, with 1K items each + accountKey := crypto.Keccak256Hash([]byte{0x13, 0x37}) + storageKey := crypto.Keccak256Hash([]byte{0x13, 0x37}) + accountRLP := randomAccount() + fill := func(parent snapshot) *diffLayer { + var ( + accounts = make(map[common.Hash][]byte) + storage = make(map[common.Hash]map[common.Hash][]byte) + ) + accounts[accountKey] = accountRLP + + accStorage := make(map[common.Hash][]byte) + for i := 0; i < 5; i++ { + value := make([]byte, 32) + crand.Read(value) + accStorage[randomHash()] = value + storage[accountKey] = accStorage } - // Storage exists in both parent and child, merge the slots - maps.Copy(parent.storageData[accountHash], storage) + return newDiffLayer(parent, common.Hash{}, accounts, storage) } - // Return the combo parent - return &diffLayer{ - parent: parent.parent, - origin: parent.origin, - root: dl.root, - accountData: parent.accountData, - storageData: parent.storageData, - storageList: make(map[common.Hash][]common.Hash), - diffed: dl.diffed, - memory: parent.memory + dl.memory, + var layer snapshot + layer = emptyLayer() + for i := 0; i < 128; i++ { + layer = fill(layer) + } + for b.Loop() { + layer.Storage(accountKey, storageKey) } } -// AccountList returns a sorted list of all accounts in this diffLayer, including -// the deleted ones. +// With accountList and sorting +// BenchmarkFlatten-6 50 29890856 ns/op // -// Note, the returned slice is not a copy, so do not modify it. -func (dl *diffLayer) AccountList() []common.Hash { - // If an old list already exists, return it - dl.lock.RLock() - list := dl.accountList - dl.lock.RUnlock() - - if list != nil { - return list +// Without sorting and tracking accountList +// BenchmarkFlatten-6 300 5511511 ns/op +func BenchmarkFlatten(b *testing.B) { + fill := func(parent snapshot) *diffLayer { + var ( + accounts = make(map[common.Hash][]byte) + storage = make(map[common.Hash]map[common.Hash][]byte) + ) + for i := 0; i < 100; i++ { + accountKey := randomHash() + accounts[accountKey] = randomAccount() + + accStorage := make(map[common.Hash][]byte) + for i := 0; i < 20; i++ { + value := make([]byte, 32) + crand.Read(value) + accStorage[randomHash()] = value + } + storage[accountKey] = accStorage + } + return newDiffLayer(parent, common.Hash{}, accounts, storage) } - // No old sorted account list exists, generate a new one - dl.lock.Lock() - defer dl.lock.Unlock() + for b.Loop() { + var layer snapshot + layer = emptyLayer() + for i := 1; i < 128; i++ { + layer = fill(layer) + } + b.StartTimer() - dl.accountList = slices.SortedFunc(maps.Keys(dl.accountData), common.Hash.Cmp) - dl.memory += uint64(len(dl.accountList) * common.HashLength) - return dl.accountList + for i := 1; i < 128; i++ { + dl, ok := layer.(*diffLayer) + if !ok { + break + } + layer = dl.flatten() + } + b.StopTimer() + } } -// StorageList returns a sorted list of all storage slot hashes in this diffLayer -// for the given account. If the whole storage is destructed in this layer, then -// an additional flag *destructed = true* will be returned, otherwise the flag is -// false. Besides, the returned list will include the hash of deleted storage slot. -// Note a special case is an account is deleted in a prior tx but is recreated in -// the following tx with some storage slots set. In this case the returned list is -// not empty but the flag is true. +// This test writes ~324M of diff layers to disk, spread over +// - 128 individual layers, +// - each with 200 accounts +// - containing 200 slots // -// Note, the returned slice is not a copy, so do not modify it. -func (dl *diffLayer) StorageList(accountHash common.Hash) []common.Hash { - dl.lock.RLock() - if _, ok := dl.storageData[accountHash]; !ok { - // Account not tracked by this layer - dl.lock.RUnlock() - return nil +// BenchmarkJournal-6 1 1471373923 ns/ops +// BenchmarkJournal-6 1 1208083335 ns/op // bufio writer +func BenchmarkJournal(b *testing.B) { + fill := func(parent snapshot) *diffLayer { + var ( + accounts = make(map[common.Hash][]byte) + storage = make(map[common.Hash]map[common.Hash][]byte) + ) + for i := 0; i < 200; i++ { + accountKey := randomHash() + accounts[accountKey] = randomAccount() + + accStorage := make(map[common.Hash][]byte) + for i := 0; i < 200; i++ { + value := make([]byte, 32) + crand.Read(value) + accStorage[randomHash()] = value + } + storage[accountKey] = accStorage + } + return newDiffLayer(parent, common.Hash{}, accounts, storage) } - // If an old list already exists, return it - if list, exist := dl.storageList[accountHash]; exist { - dl.lock.RUnlock() - return list // the cached list can't be nil + layer := snapshot(emptyLayer()) + for i := 1; i < 128; i++ { + layer = fill(layer) + } + for b.Loop() { + layer.Journal(new(bytes.Buffer)) } - dl.lock.RUnlock() - - // No old sorted account list exists, generate a new one - dl.lock.Lock() - defer dl.lock.Unlock() - - storageList := slices.SortedFunc(maps.Keys(dl.storageData[accountHash]), common.Hash.Cmp) - dl.storageList[accountHash] = storageList - dl.memory += uint64(len(storageList)*common.HashLength + common.HashLength) - return storageList }