From 91ea245c758d1158a17d5b57504b9cbdb3afccb0 Mon Sep 17 00:00:00 2001 From: Gary Rong Date: Thu, 15 May 2025 20:26:50 +0800 Subject: [PATCH] triedb/pathdb: fix panic of concurrent map iteration and write fatal error: concurrent map iteration and map write goroutine 3503687081 [running]: internal/runtime/maps.fatal({0x1c0e410?, 0xc0d9bbe000?}) runtime/panic.go:1053 +0x18 internal/runtime/maps.(*Iter).Next(0xc003b47280?) internal/runtime/maps/table.go:683 +0x86 github.com/ethereum/go-ethereum/triedb/pathdb.(*stateSet).accountList.Keys[...].func1() maps/iter.go:27 +0x71 slices.AppendSeq[...](...) slices/iter.go:50 slices.Collect[...](...) slices/iter.go:58 slices.SortedFunc[...](0xc003b473d0, 0x1f96be0) slices/iter.go:72 +0xd0 github.com/ethereum/go-ethereum/triedb/pathdb.(*stateSet).accountList(0xc003d26120) github.com/ethereum/go-ethereum/triedb/pathdb/states.go:179 +0x13a github.com/ethereum/go-ethereum/triedb/pathdb.newDiffAccountIterator({0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...}, ...) --- triedb/pathdb/iterator.go | 18 +++++++--------- triedb/pathdb/iterator_binary.go | 36 ++++++++++++++++++++++++++------ triedb/pathdb/iterator_fast.go | 25 ++++++++++++++++++---- triedb/pathdb/iterator_test.go | 10 ++++----- 4 files changed, 64 insertions(+), 25 deletions(-) diff --git a/triedb/pathdb/iterator.go b/triedb/pathdb/iterator.go index 980f228cf5..84ea08ddd3 100644 --- a/triedb/pathdb/iterator.go +++ b/triedb/pathdb/iterator.go @@ -91,15 +91,14 @@ type diffAccountIterator struct { } // newDiffAccountIterator creates an account iterator over the given state set. -func newDiffAccountIterator(seek common.Hash, states *stateSet, fn loadAccount) AccountIterator { +func newDiffAccountIterator(seek common.Hash, accountList []common.Hash, fn loadAccount) AccountIterator { // Seek out the requested starting account - hashes := states.accountList() - index := sort.Search(len(hashes), func(i int) bool { - return bytes.Compare(seek[:], hashes[i][:]) <= 0 + index := sort.Search(len(accountList), func(i int) bool { + return bytes.Compare(seek[:], accountList[i][:]) <= 0 }) // Assemble and returned the already seeked iterator return &diffAccountIterator{ - keys: hashes[index:], + keys: accountList[index:], loadFn: fn, } } @@ -236,15 +235,14 @@ type diffStorageIterator struct { } // newDiffStorageIterator creates a storage iterator over a single diff layer. -func newDiffStorageIterator(account common.Hash, seek common.Hash, states *stateSet, fn loadStorage) StorageIterator { - hashes := states.storageList(account) - index := sort.Search(len(hashes), func(i int) bool { - return bytes.Compare(seek[:], hashes[i][:]) <= 0 +func newDiffStorageIterator(account common.Hash, seek common.Hash, storageList []common.Hash, fn loadStorage) StorageIterator { + index := sort.Search(len(storageList), func(i int) bool { + return bytes.Compare(seek[:], storageList[i][:]) <= 0 }) // Assemble and returned the already seeked iterator return &diffStorageIterator{ account: account, - keys: hashes[index:], + keys: storageList[index:], loadFn: fn, } } diff --git a/triedb/pathdb/iterator_binary.go b/triedb/pathdb/iterator_binary.go index dec31e8f3f..0620081d0c 100644 --- a/triedb/pathdb/iterator_binary.go +++ b/triedb/pathdb/iterator_binary.go @@ -45,6 +45,12 @@ type binaryIterator struct { // accounts in a slow, but easily verifiable way. Note this function is used // for initialization, use `newBinaryAccountIterator` as the API. func (dl *diskLayer) initBinaryAccountIterator(seek common.Hash) *binaryIterator { + // The state set in the disk layer is mutable, hold the lock before obtaining + // the account list to prevent concurrent map iteration and write. + dl.lock.RLock() + accountList := dl.buffer.states.accountList() + dl.lock.RUnlock() + // Create two iterators for state buffer and the persistent state in disk // respectively and combine them as a binary iterator. l := &binaryIterator{ @@ -54,7 +60,7 @@ func (dl *diskLayer) initBinaryAccountIterator(seek common.Hash) *binaryIterator // The account key list for iteration is deterministic once the iterator // is constructed, no matter the referenced disk layer is stale or not // later. - a: newDiffAccountIterator(seek, dl.buffer.states, nil), + a: newDiffAccountIterator(seek, accountList, nil), b: newDiskAccountIterator(dl.db.diskdb, seek), } l.aDone = !l.a.Next() @@ -68,6 +74,9 @@ func (dl *diskLayer) initBinaryAccountIterator(seek common.Hash) *binaryIterator func (dl *diffLayer) initBinaryAccountIterator(seek common.Hash) *binaryIterator { parent, ok := dl.parent.(*diffLayer) if !ok { + // The state set in diff layer is immutable and will never be stale, + // so the read lock protection is unnecessary. + accountList := dl.states.stateSet.accountList() l := &binaryIterator{ // The account loader function is unnecessary; the account key list // produced by the supplied state set alone is sufficient for iteration. @@ -75,13 +84,16 @@ func (dl *diffLayer) initBinaryAccountIterator(seek common.Hash) *binaryIterator // The account key list for iteration is deterministic once the iterator // is constructed, no matter the referenced disk layer is stale or not // later. - a: newDiffAccountIterator(seek, dl.states.stateSet, nil), + a: newDiffAccountIterator(seek, accountList, nil), b: dl.parent.(*diskLayer).initBinaryAccountIterator(seek), } l.aDone = !l.a.Next() l.bDone = !l.b.Next() return l } + // The state set in diff layer is immutable and will never be stale, + // so the read lock protection is unnecessary. + accountList := dl.states.stateSet.accountList() l := &binaryIterator{ // The account loader function is unnecessary; the account key list // produced by the supplied state set alone is sufficient for iteration. @@ -89,7 +101,7 @@ func (dl *diffLayer) initBinaryAccountIterator(seek common.Hash) *binaryIterator // The account key list for iteration is deterministic once the iterator // is constructed, no matter the referenced disk layer is stale or not // later. - a: newDiffAccountIterator(seek, dl.states.stateSet, nil), + a: newDiffAccountIterator(seek, accountList, nil), b: parent.initBinaryAccountIterator(seek), } l.aDone = !l.a.Next() @@ -101,6 +113,12 @@ func (dl *diffLayer) initBinaryAccountIterator(seek common.Hash) *binaryIterator // storage slots in a slow, but easily verifiable way. Note this function is used // for initialization, use `newBinaryStorageIterator` as the API. func (dl *diskLayer) initBinaryStorageIterator(account common.Hash, seek common.Hash) *binaryIterator { + // The state set in the disk layer is mutable, hold the lock before obtaining + // the storage list to prevent concurrent map iteration and write. + dl.lock.RLock() + storageList := dl.buffer.states.storageList(account) + dl.lock.RUnlock() + // Create two iterators for state buffer and the persistent state in disk // respectively and combine them as a binary iterator. l := &binaryIterator{ @@ -110,7 +128,7 @@ func (dl *diskLayer) initBinaryStorageIterator(account common.Hash, seek common. // The storage key list for iteration is deterministic once the iterator // is constructed, no matter the referenced disk layer is stale or not // later. - a: newDiffStorageIterator(account, seek, dl.buffer.states, nil), + a: newDiffStorageIterator(account, seek, storageList, nil), b: newDiskStorageIterator(dl.db.diskdb, account, seek), } l.aDone = !l.a.Next() @@ -124,6 +142,9 @@ func (dl *diskLayer) initBinaryStorageIterator(account common.Hash, seek common. func (dl *diffLayer) initBinaryStorageIterator(account common.Hash, seek common.Hash) *binaryIterator { parent, ok := dl.parent.(*diffLayer) if !ok { + // The state set in diff layer is immutable and will never be stale, + // so the read lock protection is unnecessary. + storageList := dl.states.stateSet.storageList(account) l := &binaryIterator{ // The storage loader function is unnecessary; the storage key list // produced by the supplied state set alone is sufficient for iteration. @@ -131,13 +152,16 @@ func (dl *diffLayer) initBinaryStorageIterator(account common.Hash, seek common. // The storage key list for iteration is deterministic once the iterator // is constructed, no matter the referenced disk layer is stale or not // later. - a: newDiffStorageIterator(account, seek, dl.states.stateSet, nil), + a: newDiffStorageIterator(account, seek, storageList, nil), b: dl.parent.(*diskLayer).initBinaryStorageIterator(account, seek), } l.aDone = !l.a.Next() l.bDone = !l.b.Next() return l } + // The state set in diff layer is immutable and will never be stale, + // so the read lock protection is unnecessary. + storageList := dl.states.stateSet.storageList(account) l := &binaryIterator{ // The storage loader function is unnecessary; the storage key list // produced by the supplied state set alone is sufficient for iteration. @@ -145,7 +169,7 @@ func (dl *diffLayer) initBinaryStorageIterator(account common.Hash, seek common. // The storage key list for iteration is deterministic once the iterator // is constructed, no matter the referenced disk layer is stale or not // later. - a: newDiffStorageIterator(account, seek, dl.states.stateSet, nil), + a: newDiffStorageIterator(account, seek, storageList, nil), b: parent.initBinaryStorageIterator(account, seek), } l.aDone = !l.a.Next() diff --git a/triedb/pathdb/iterator_fast.go b/triedb/pathdb/iterator_fast.go index 966e37a7cb..87b2dd567a 100644 --- a/triedb/pathdb/iterator_fast.go +++ b/triedb/pathdb/iterator_fast.go @@ -76,11 +76,17 @@ func newFastIterator(db *Database, root common.Hash, account common.Hash, seek c if accountIterator { switch dl := current.(type) { case *diskLayer: + // The state set in the disk layer is mutable, hold the lock before obtaining + // the account list to prevent concurrent map iteration and write. + dl.lock.RLock() + accountList := dl.buffer.states.accountList() + dl.lock.RUnlock() + fi.iterators = append(fi.iterators, &weightedIterator{ // The state set in the disk layer is mutable, and the entire state becomes stale // if a diff layer above is merged into it. Therefore, staleness must be checked, // and the storage slot should be retrieved with read lock protection. - it: newDiffAccountIterator(seek, dl.buffer.states, func(hash common.Hash) ([]byte, error) { + it: newDiffAccountIterator(seek, accountList, func(hash common.Hash) ([]byte, error) { dl.lock.RLock() defer dl.lock.RUnlock() @@ -98,19 +104,26 @@ func newFastIterator(db *Database, root common.Hash, account common.Hash, seek c case *diffLayer: // The state set in diff layer is immutable and will never be stale, // so the read lock protection is unnecessary. + accountList := dl.states.accountList() fi.iterators = append(fi.iterators, &weightedIterator{ - it: newDiffAccountIterator(seek, dl.states.stateSet, dl.states.mustAccount), + it: newDiffAccountIterator(seek, accountList, dl.states.mustAccount), priority: depth, }) } } else { switch dl := current.(type) { case *diskLayer: + // The state set in the disk layer is mutable, hold the lock before obtaining + // the storage list to prevent concurrent map iteration and write. + dl.lock.RLock() + storageList := dl.buffer.states.storageList(account) + dl.lock.RUnlock() + fi.iterators = append(fi.iterators, &weightedIterator{ // The state set in the disk layer is mutable, and the entire state becomes stale // if a diff layer above is merged into it. Therefore, staleness must be checked, // and the storage slot should be retrieved with read lock protection. - it: newDiffStorageIterator(account, seek, dl.buffer.states, func(addrHash common.Hash, slotHash common.Hash) ([]byte, error) { + it: newDiffStorageIterator(account, seek, storageList, func(addrHash common.Hash, slotHash common.Hash) ([]byte, error) { dl.lock.RLock() defer dl.lock.RUnlock() @@ -126,10 +139,14 @@ func newFastIterator(db *Database, root common.Hash, account common.Hash, seek c priority: depth + 1, }) case *diffLayer: + // The state set in diff layer is immutable and will never be stale, + // so the read lock protection is unnecessary. + storageList := dl.states.storageList(account) + // The state set in diff layer is immutable and will never be stale, // so the read lock protection is unnecessary. fi.iterators = append(fi.iterators, &weightedIterator{ - it: newDiffStorageIterator(account, seek, dl.states.stateSet, dl.states.mustStorage), + it: newDiffStorageIterator(account, seek, storageList, dl.states.mustStorage), priority: depth, }) } diff --git a/triedb/pathdb/iterator_test.go b/triedb/pathdb/iterator_test.go index 854b81ec2f..6517735d7e 100644 --- a/triedb/pathdb/iterator_test.go +++ b/triedb/pathdb/iterator_test.go @@ -132,7 +132,7 @@ func TestAccountIteratorBasics(t *testing.T) { } } states := newStates(accounts, storage, false) - it := newDiffAccountIterator(common.Hash{}, states, nil) + it := newDiffAccountIterator(common.Hash{}, states.accountList(), nil) verifyIterator(t, 100, it, verifyNothing) // Nil is allowed for single layer iterator db := rawdb.NewMemoryDatabase() @@ -170,7 +170,7 @@ func TestStorageIteratorBasics(t *testing.T) { } states := newStates(accounts, storage, false) for account := range accounts { - it := newDiffStorageIterator(account, common.Hash{}, states, nil) + it := newDiffStorageIterator(account, common.Hash{}, states.storageList(account), nil) verifyIterator(t, 100, it, verifyNothing) // Nil is allowed for single layer iterator } @@ -273,7 +273,7 @@ func TestAccountIteratorTraversal(t *testing.T) { head := db.tree.get(common.HexToHash("0x04")) // singleLayer: 0xcc, 0xf0, 0xff - it := newDiffAccountIterator(common.Hash{}, head.(*diffLayer).states.stateSet, nil) + it := newDiffAccountIterator(common.Hash{}, head.(*diffLayer).states.stateSet.accountList(), nil) verifyIterator(t, 3, it, verifyNothing) // binaryIterator: 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xf0, 0xff @@ -317,7 +317,7 @@ func TestStorageIteratorTraversal(t *testing.T) { head := db.tree.get(common.HexToHash("0x04")) // singleLayer: 0x1, 0x2, 0x3 - diffIter := newDiffStorageIterator(common.HexToHash("0xaa"), common.Hash{}, head.(*diffLayer).states.stateSet, nil) + diffIter := newDiffStorageIterator(common.HexToHash("0xaa"), common.Hash{}, head.(*diffLayer).states.stateSet.storageList(common.HexToHash("0xaa")), nil) verifyIterator(t, 3, diffIter, verifyNothing) // binaryIterator: 0x1, 0x2, 0x3, 0x4, 0x5, 0x6 @@ -606,7 +606,7 @@ func TestAccountIteratorLargeTraversal(t *testing.T) { } // Iterate the entire stack and ensure everything is hit only once head := db.tree.get(common.HexToHash("0x80")) - verifyIterator(t, 200, newDiffAccountIterator(common.Hash{}, head.(*diffLayer).states.stateSet, nil), verifyNothing) + verifyIterator(t, 200, newDiffAccountIterator(common.Hash{}, head.(*diffLayer).states.stateSet.accountList(), nil), verifyNothing) verifyIterator(t, 200, head.(*diffLayer).newBinaryAccountIterator(common.Hash{}), verifyAccount) it, _ := db.AccountIterator(common.HexToHash("0x80"), common.Hash{})