From c6bfbdef7c61bd6944bd3b6322a39d31d3e92122 Mon Sep 17 00:00:00 2001 From: Gary Rong Date: Fri, 9 May 2025 15:31:47 +0800 Subject: [PATCH] triedb/pathdb: polish code --- triedb/pathdb/database.go | 24 +++++++++++++++++------- triedb/pathdb/disklayer.go | 17 +++++++++++------ triedb/pathdb/flush.go | 14 ++++++++------ triedb/pathdb/journal.go | 29 ++++++++++++++++------------- 4 files changed, 52 insertions(+), 32 deletions(-) diff --git a/triedb/pathdb/database.go b/triedb/pathdb/database.go index 91429424f7..83df04e264 100644 --- a/triedb/pathdb/database.go +++ b/triedb/pathdb/database.go @@ -255,8 +255,9 @@ func New(diskdb ethdb.Database, config *Config, isVerkle bool) *Database { // mandatory. This ensures that uncovered flat states are not accessed, // even if background generation is not allowed. If permitted, the generation // might be scheduled. - db.setStateGenerator() - + if err := db.setStateGenerator(); err != nil { + log.Crit("Failed to setup the generator", "err", err) + } fields := config.fields() if db.isVerkle { fields = append(fields, "verkle", true) @@ -316,10 +317,13 @@ func (db *Database) repairHistory() error { // setStateGenerator loads the state generation progress marker and potentially // resume the state generation if it's permitted. -func (db *Database) setStateGenerator() { +func (db *Database) setStateGenerator() error { // Load the state snapshot generation progress marker to prevent access // to uncovered states. - generator, root := loadGenerator(db.diskdb) + generator, root, err := loadGenerator(db.diskdb, db.hasher) + if err != nil { + return err + } if generator == nil { // Initialize an empty generator to rebuild the state snapshot from scratch generator = &journalGenerator{ @@ -330,7 +334,7 @@ func (db *Database) setStateGenerator() { // The generator will be left as nil in disk layer for representing the whole // state snapshot is available for accessing. if generator.Done { - return + return nil } var origin uint64 if len(generator.Marker) >= 8 { @@ -345,18 +349,24 @@ func (db *Database) setStateGenerator() { } dl := db.tree.bottom() + // Disable the background snapshot building in these circumstances: + // - the database is opened in read only mode + // - the snapshot build is explicitly disabled + // - the database is opened in verkle tree mode + noBuild := db.readOnly || db.config.SnapshotNoBuild || db.isVerkle + // Construct the generator and link it to the disk layer, ensuring that the // generation progress is resolved to prevent accessing uncovered states // regardless of whether background state snapshot generation is allowed. - noBuild := db.readOnly || db.config.SnapshotNoBuild || db.isVerkle dl.setGenerator(newGenerator(db.diskdb, noBuild, generator.Marker, stats)) // Short circuit if the background generation is not permitted if noBuild || db.waitSync { - return + return nil } stats.log("Starting snapshot generation", root, generator.Marker) dl.generator.run(root) + return nil } // Update adds a new layer into the tree, if that can be linked to an existing diff --git a/triedb/pathdb/disklayer.go b/triedb/pathdb/disklayer.go index deed6a34a9..28c6217c4b 100644 --- a/triedb/pathdb/disklayer.go +++ b/triedb/pathdb/disklayer.go @@ -30,14 +30,19 @@ import ( // diskLayer is a low level persistent layer built on top of a key-value store. type diskLayer struct { - root common.Hash // Immutable, root hash to which this layer was made for - id uint64 // Immutable, corresponding state id - db *Database // Path-based trie database + root common.Hash // Immutable, root hash to which this layer was made for + id uint64 // Immutable, corresponding state id + db *Database // Path-based trie database + + // These two caches must be maintained separately, because the key + // for the root node of the storage trie (accountHash) is identical + // to the key for the account data. nodes *fastcache.Cache // GC friendly memory cache of clean nodes states *fastcache.Cache // GC friendly memory cache of clean states - buffer *buffer // Dirty buffer to aggregate writes of nodes and states - stale bool // Signals that the layer became stale (state progressed) - lock sync.RWMutex // Lock used to protect stale flag and genMarker + + buffer *buffer // Dirty buffer to aggregate writes of nodes and states + stale bool // Signals that the layer became stale (state progressed) + lock sync.RWMutex // Lock used to protect stale flag and genMarker // The generator is set if the state snapshot was not fully completed generator *generator diff --git a/triedb/pathdb/flush.go b/triedb/pathdb/flush.go index 334d6f5800..6563dbccff 100644 --- a/triedb/pathdb/flush.go +++ b/triedb/pathdb/flush.go @@ -80,9 +80,10 @@ func writeStates(batch ethdb.Batch, genMarker []byte, accountData map[common.Has slots int ) for addrHash, blob := range accountData { - // Skip any account not covered yet by the snapshot. The account - // at generation position (addrHash == genMarker[:common.HashLength]) - // should be updated. + // Skip any account not yet covered by the snapshot. The account + // at the generation marker position (addrHash == genMarker[:common.HashLength]) + // should still be updated, as it would be skipped in the next + // generation cycle. if genMarker != nil && bytes.Compare(addrHash[:], genMarker) > 0 { continue } @@ -107,9 +108,10 @@ func writeStates(batch ethdb.Batch, genMarker []byte, accountData map[common.Has midAccount := genMarker != nil && bytes.Equal(addrHash[:], genMarker[:common.HashLength]) for storageHash, blob := range storages { - // Skip any slot not covered yet by the snapshot. The storage slot - // at generation position (addrHash == genMarker[:common.HashLength] - // and storageHash == genMarker[common.HashLength:]) should be updated. + // Skip any storage slot not yet covered by the snapshot. The storage slot + // at the generation marker position (addrHash == genMarker[:common.HashLength] + // and storageHash == genMarker[common.HashLength:]) should still be updated, + // as it would be skipped in the next generation cycle. if midAccount && bytes.Compare(storageHash[:], genMarker[common.HashLength:]) > 0 { continue } diff --git a/triedb/pathdb/journal.go b/triedb/pathdb/journal.go index 95c19c25a3..5dc6da92b8 100644 --- a/triedb/pathdb/journal.go +++ b/triedb/pathdb/journal.go @@ -26,7 +26,6 @@ import ( "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/core/rawdb" "github.com/ethereum/go-ethereum/core/types" - "github.com/ethereum/go-ethereum/crypto" "github.com/ethereum/go-ethereum/ethdb" "github.com/ethereum/go-ethereum/log" "github.com/ethereum/go-ethereum/rlp" @@ -106,36 +105,40 @@ type journalGenerator struct { } // loadGenerator loads the state generation progress marker from the database. -func loadGenerator(db ethdb.KeyValueReader) (*journalGenerator, common.Hash) { - trieRoot := types.EmptyRootHash - if blob := rawdb.ReadAccountTrieNode(db, nil); len(blob) > 0 { - trieRoot = crypto.Keccak256Hash(blob) +func loadGenerator(db ethdb.KeyValueReader, hash nodeHasher) (*journalGenerator, common.Hash, error) { + trieRoot, err := hash(rawdb.ReadAccountTrieNode(db, nil)) + if err != nil { + return nil, common.Hash{}, err } // State generation progress marker is lost, rebuild it blob := rawdb.ReadSnapshotGenerator(db) if len(blob) == 0 { log.Info("State snapshot generator is not found") - return nil, trieRoot + return nil, trieRoot, nil } // State generation progress marker is not compatible, rebuild it var generator journalGenerator if err := rlp.DecodeBytes(blob, &generator); err != nil { log.Info("State snapshot generator is not compatible") - return nil, trieRoot + return nil, trieRoot, nil } - // The state snapshot is inconsistent with the trie data and needs to be rebuilt. - // Note: The SnapshotRoot and SnapshotGenerator are always consistent with each - // other, no matter in the legacy state snapshot or the path database. + // The state snapshot is inconsistent with the trie data and must + // be rebuilt. + // + // Note: The SnapshotRoot and SnapshotGenerator are always consistent + // with each other, both in the legacy state snapshot and the path database. + // Therefore, if the SnapshotRoot does not match the trie root, + // the entire generator is considered stale and must be discarded. stateRoot := rawdb.ReadSnapshotRoot(db) if trieRoot != stateRoot { - log.Info("State snapshot is not consistent with trie", "trie", trieRoot, "state", stateRoot) - return nil, trieRoot + log.Info("State snapshot is not consistent", "trie", trieRoot, "state", stateRoot) + return nil, trieRoot, nil } // Slice null-ness is lost after rlp decoding, reset it back to empty if !generator.Done && generator.Marker == nil { generator.Marker = []byte{} } - return &generator, trieRoot + return &generator, trieRoot, nil } // loadLayers loads a pre-existing state layer backed by a key-value store.