mirror of
https://github.com/ethereum/go-ethereum.git
synced 2026-07-26 06:36:43 +00:00
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, ...}, ...)
This commit is contained in:
Gary Rong
parent
b4d741355f
commit
91ea245c75
4 changed files with 64 additions and 25 deletions
|
|
@ -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,
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
})
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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{})
|
||||
|
|
|
|||
Loading…
Reference in a new issue