From a4a1388b5e50e47b74a45dc970b8ed0f471db46f Mon Sep 17 00:00:00 2001 From: Maciej Kulawik Date: Fri, 17 Nov 2023 16:20:58 +0000 Subject: [PATCH 1/2] trie/triedb/hashdb: lock db lock when accessing dirties cache Add read locking of db lock around access to dirties cache in hashdb.Database to prevent data race versus hashdb.Database.dereference which can modify the dirities map by deleting an item. Fixes #28541 --- trie/triedb/hashdb/database.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/trie/triedb/hashdb/database.go b/trie/triedb/hashdb/database.go index 764ab24ec8dd..a90c100ebcb7 100644 --- a/trie/triedb/hashdb/database.go +++ b/trie/triedb/hashdb/database.go @@ -352,20 +352,25 @@ func (db *Database) Cap(limit common.StorageSize) error { // outside code doesn't see an inconsistent state (referenced data removed from // memory cache during commit but not yet in persistent storage). This is ensured // by only uncaching existing data when the database write finalizes. - nodes, storage, start := len(db.dirties), db.dirtiesSize, time.Now() + start := time.Now() batch := db.diskdb.NewBatch() + db.lock.RLock() + nodes, storage := len(db.dirties), db.dirtiesSize // db.dirtiesSize only contains the useful data in the cache, but when reporting // the total memory consumption, the maintenance metadata is also needed to be // counted. size := db.dirtiesSize + common.StorageSize(len(db.dirties)*cachedNodeSize) + db.lock.RUnlock() size += db.childrenSize // Keep committing nodes from the flush-list until we're below allowance oldest := db.oldest for size > limit && oldest != (common.Hash{}) { // Fetch the oldest referenced node and push into the batch + db.lock.RLock() node := db.dirties[oldest] + db.lock.RUnlock() rawdb.WriteLegacyTrieNode(batch, oldest, node.node) // If we exceeded the ideal batch size, commit and reset @@ -436,7 +441,9 @@ func (db *Database) Commit(node common.Hash, report bool) error { batch := db.diskdb.NewBatch() // Move the trie itself into the batch, flushing if enough data is accumulated + db.lock.RLock() nodes, storage := len(db.dirties), db.dirtiesSize + db.lock.RUnlock() uncacher := &cleaner{db} if err := db.commit(node, batch, uncacher); err != nil { @@ -478,7 +485,9 @@ func (db *Database) Commit(node common.Hash, report bool) error { // commit is the private locked version of Commit. func (db *Database) commit(hash common.Hash, batch ethdb.Batch, uncacher *cleaner) error { // If the node does not exist, it's a previously committed node + db.lock.RLock() node, ok := db.dirties[hash] + db.lock.RUnlock() if !ok { return nil } From 586d4d7519b47358b90379c06f7da622172c39bc Mon Sep 17 00:00:00 2001 From: Gary Rong Date: Mon, 20 Nov 2023 01:12:10 +0800 Subject: [PATCH 2/2] trie/triedb/hashdb: simplify the lock management in hashdb --- trie/database.go | 11 ----- trie/triedb/hashdb/database.go | 78 ++++++++++------------------------ 2 files changed, 23 insertions(+), 66 deletions(-) diff --git a/trie/database.go b/trie/database.go index 321b4f8955b7..e20f7ef903b0 100644 --- a/trie/database.go +++ b/trie/database.go @@ -240,17 +240,6 @@ func (db *Database) Dereference(root common.Hash) error { return nil } -// Node retrieves the rlp-encoded node blob with provided node hash. It's -// only supported by hash-based database and will return an error for others. -// Note, this function should be deprecated once ETH66 is deprecated. -func (db *Database) Node(hash common.Hash) ([]byte, error) { - hdb, ok := db.backend.(*hashdb.Database) - if !ok { - return nil, errors.New("not supported") - } - return hdb.Node(hash) -} - // Recover rollbacks the database to a specified historical point. The state is // supported as the rollback destination only if it's canonical state and the // corresponding trie histories are existent. It's only supported by path-based diff --git a/trie/triedb/hashdb/database.go b/trie/triedb/hashdb/database.go index a90c100ebcb7..e45ccdba32ca 100644 --- a/trie/triedb/hashdb/database.go +++ b/trie/triedb/hashdb/database.go @@ -82,11 +82,6 @@ var Defaults = &Config{ // Database is an intermediate write layer between the trie data structures and // the disk database. The aim is to accumulate trie writes in-memory and only // periodically flush a couple tries to disk, garbage collecting the remainder. -// -// Note, the trie Database is **not** thread safe in its mutations, but it **is** -// thread safe in providing individual, independent node access. The rationale -// behind this split design is to provide read access to RPC handlers and sync -// servers even while the trie is executing expensive garbage collection. type Database struct { diskdb ethdb.Database // Persistent storage for matured trie nodes resolver ChildResolver // The handler to resolve children of nodes @@ -113,7 +108,7 @@ type Database struct { // cachedNode is all the information we know about a single cached trie node // in the memory database write layer. type cachedNode struct { - node []byte // Encoded node blob + node []byte // Encoded node blob, immutable parents uint32 // Number of live nodes referencing this one external map[common.Hash]struct{} // The set of external children flushPrev common.Hash // Previous node in the flush-list @@ -152,9 +147,9 @@ func New(diskdb ethdb.Database, config *Config, resolver ChildResolver) *Databas } } -// insert inserts a simplified trie node into the memory database. -// All nodes inserted by this function will be reference tracked -// and in theory should only used for **trie nodes** insertion. +// insert inserts a trie node into the memory database. All nodes inserted by +// this function will be reference tracked. This function assumes the lock is +// already held. func (db *Database) insert(hash common.Hash, node []byte) { // If the node's already cached, skip if _, ok := db.dirties[hash]; ok { @@ -183,9 +178,9 @@ func (db *Database) insert(hash common.Hash, node []byte) { db.dirtiesSize += common.StorageSize(common.HashLength + len(node)) } -// Node retrieves an encoded cached trie node from memory. If it cannot be found +// node retrieves an encoded cached trie node from memory. If it cannot be found // cached, the method queries the persistent database for the content. -func (db *Database) Node(hash common.Hash) ([]byte, error) { +func (db *Database) node(hash common.Hash) ([]byte, error) { // It doesn't make sense to retrieve the metaroot if hash == (common.Hash{}) { return nil, errors.New("not found") @@ -198,11 +193,14 @@ func (db *Database) Node(hash common.Hash) ([]byte, error) { return enc, nil } } - // Retrieve the node from the dirty cache if available + // Retrieve the node from the dirty cache if available. db.lock.RLock() dirty := db.dirties[hash] db.lock.RUnlock() + // Return the cached node if it's found in the dirty set. + // The dirty.node field is immutable and safe to read it + // even without lock guard. if dirty != nil { memcacheDirtyHitMeter.Mark(1) memcacheDirtyReadMeter.Mark(int64(len(dirty.node))) @@ -223,20 +221,6 @@ func (db *Database) Node(hash common.Hash) ([]byte, error) { return nil, errors.New("not found") } -// Nodes retrieves the hashes of all the nodes cached within the memory database. -// This method is extremely expensive and should only be used to validate internal -// states in test code. -func (db *Database) Nodes() []common.Hash { - db.lock.RLock() - defer db.lock.RUnlock() - - var hashes = make([]common.Hash, 0, len(db.dirties)) - for hash := range db.dirties { - hashes = append(hashes, hash) - } - return hashes -} - // Reference adds a new reference from a parent node to a child node. // This function is used to add reference between internal trie node // and external node(e.g. storage trie root), all internal trie nodes @@ -344,33 +328,28 @@ func (db *Database) dereference(hash common.Hash) { // Cap iteratively flushes old but still referenced trie nodes until the total // memory usage goes below the given threshold. -// -// Note, this method is a non-synchronized mutator. It is unsafe to call this -// concurrently with other mutators. func (db *Database) Cap(limit common.StorageSize) error { + db.lock.Lock() + defer db.lock.Unlock() + // Create a database batch to flush persistent data out. It is important that // outside code doesn't see an inconsistent state (referenced data removed from // memory cache during commit but not yet in persistent storage). This is ensured // by only uncaching existing data when the database write finalizes. - start := time.Now() batch := db.diskdb.NewBatch() - db.lock.RLock() - nodes, storage := len(db.dirties), db.dirtiesSize + nodes, storage, start := len(db.dirties), db.dirtiesSize, time.Now() // db.dirtiesSize only contains the useful data in the cache, but when reporting // the total memory consumption, the maintenance metadata is also needed to be // counted. size := db.dirtiesSize + common.StorageSize(len(db.dirties)*cachedNodeSize) - db.lock.RUnlock() size += db.childrenSize // Keep committing nodes from the flush-list until we're below allowance oldest := db.oldest for size > limit && oldest != (common.Hash{}) { // Fetch the oldest referenced node and push into the batch - db.lock.RLock() node := db.dirties[oldest] - db.lock.RUnlock() rawdb.WriteLegacyTrieNode(batch, oldest, node.node) // If we exceeded the ideal batch size, commit and reset @@ -396,9 +375,6 @@ func (db *Database) Cap(limit common.StorageSize) error { return err } // Write successful, clear out the flushed data - db.lock.Lock() - defer db.lock.Unlock() - for db.oldest != oldest { node := db.dirties[db.oldest] delete(db.dirties, db.oldest) @@ -429,10 +405,10 @@ func (db *Database) Cap(limit common.StorageSize) error { // Commit iterates over all the children of a particular node, writes them out // to disk, forcefully tearing down all references in both directions. As a side // effect, all pre-images accumulated up to this point are also written. -// -// Note, this method is a non-synchronized mutator. It is unsafe to call this -// concurrently with other mutators. func (db *Database) Commit(node common.Hash, report bool) error { + db.lock.Lock() + defer db.lock.Unlock() + // Create a database batch to flush persistent data out. It is important that // outside code doesn't see an inconsistent state (referenced data removed from // memory cache during commit but not yet in persistent storage). This is ensured @@ -441,9 +417,7 @@ func (db *Database) Commit(node common.Hash, report bool) error { batch := db.diskdb.NewBatch() // Move the trie itself into the batch, flushing if enough data is accumulated - db.lock.RLock() nodes, storage := len(db.dirties), db.dirtiesSize - db.lock.RUnlock() uncacher := &cleaner{db} if err := db.commit(node, batch, uncacher); err != nil { @@ -456,8 +430,6 @@ func (db *Database) Commit(node common.Hash, report bool) error { return err } // Uncache any leftovers in the last batch - db.lock.Lock() - defer db.lock.Unlock() if err := batch.Replay(uncacher); err != nil { return err } @@ -485,9 +457,7 @@ func (db *Database) Commit(node common.Hash, report bool) error { // commit is the private locked version of Commit. func (db *Database) commit(hash common.Hash, batch ethdb.Batch, uncacher *cleaner) error { // If the node does not exist, it's a previously committed node - db.lock.RLock() node, ok := db.dirties[hash] - db.lock.RUnlock() if !ok { return nil } @@ -508,13 +478,11 @@ func (db *Database) commit(hash common.Hash, batch ethdb.Batch, uncacher *cleane if err := batch.Write(); err != nil { return err } - db.lock.Lock() err := batch.Replay(uncacher) - batch.Reset() - db.lock.Unlock() if err != nil { return err } + batch.Reset() } return nil } @@ -583,7 +551,7 @@ func (db *Database) Initialized(genesisRoot common.Hash) bool { func (db *Database) Update(root common.Hash, parent common.Hash, block uint64, nodes *trienode.MergedNodeSet, states *triestate.Set) error { // Ensure the parent state is present and signal a warning if not. if parent != types.EmptyRootHash { - if blob, _ := db.Node(parent); len(blob) == 0 { + if blob, _ := db.node(parent); len(blob) == 0 { log.Error("parent state is not present") } } @@ -664,7 +632,7 @@ func (db *Database) Scheme() string { // Reader retrieves a node reader belonging to the given state root. // An error will be returned if the requested state is not available. func (db *Database) Reader(root common.Hash) (*reader, error) { - if _, err := db.Node(root); err != nil { + if _, err := db.node(root); err != nil { return nil, fmt.Errorf("state %#x is not available, %v", root, err) } return &reader{db: db}, nil @@ -675,9 +643,9 @@ type reader struct { db *Database } -// Node retrieves the trie node with the given node hash. -// No error will be returned if the node is not found. +// Node retrieves the trie node with the given node hash. No error will be +// returned if the node is not found. func (reader *reader) Node(owner common.Hash, path []byte, hash common.Hash) ([]byte, error) { - blob, _ := reader.db.Node(hash) + blob, _ := reader.db.node(hash) return blob, nil }