Skip to content

Commit 91ea245

Browse files
rjl493456442rjl493456442
committed
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, ...}, ...)
1 parent b4d7413 commit 91ea245

File tree

4 files changed

+64
-25
lines changed

4 files changed

+64
-25
lines changed

‎triedb/pathdb/iterator.go

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -91,15 +91,14 @@ type diffAccountIterator struct {
9191
}
9292

9393
// newDiffAccountIterator creates an account iterator over the given state set.
94-
func newDiffAccountIterator(seek common.Hash, states *stateSet, fn loadAccount) AccountIterator {
94+
func newDiffAccountIterator(seek common.Hash, accountList []common.Hash, fn loadAccount) AccountIterator {
9595
// Seek out the requested starting account
96-
hashes := states.accountList()
97-
index := sort.Search(len(hashes), func(i int) bool {
98-
return bytes.Compare(seek[:], hashes[i][:]) <= 0
96+
index := sort.Search(len(accountList), func(i int) bool {
97+
return bytes.Compare(seek[:], accountList[i][:]) <= 0
9998
})
10099
// Assemble and returned the already seeked iterator
101100
return &diffAccountIterator{
102-
keys: hashes[index:],
101+
keys: accountList[index:],
103102
loadFn: fn,
104103
}
105104
}
@@ -236,15 +235,14 @@ type diffStorageIterator struct {
236235
}
237236

238237
// newDiffStorageIterator creates a storage iterator over a single diff layer.
239-
func newDiffStorageIterator(account common.Hash, seek common.Hash, states *stateSet, fn loadStorage) StorageIterator {
240-
hashes := states.storageList(account)
241-
index := sort.Search(len(hashes), func(i int) bool {
242-
return bytes.Compare(seek[:], hashes[i][:]) <= 0
238+
func newDiffStorageIterator(account common.Hash, seek common.Hash, storageList []common.Hash, fn loadStorage) StorageIterator {
239+
index := sort.Search(len(storageList), func(i int) bool {
240+
return bytes.Compare(seek[:], storageList[i][:]) <= 0
243241
})
244242
// Assemble and returned the already seeked iterator
245243
return &diffStorageIterator{
246244
account: account,
247-
keys: hashes[index:],
245+
keys: storageList[index:],
248246
loadFn: fn,
249247
}
250248
}

‎triedb/pathdb/iterator_binary.go

Lines changed: 30 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,12 @@ type binaryIterator struct {
4545
// accounts in a slow, but easily verifiable way. Note this function is used
4646
// for initialization, use `newBinaryAccountIterator` as the API.
4747
func (dl *diskLayer) initBinaryAccountIterator(seek common.Hash) *binaryIterator {
48+
// The state set in the disk layer is mutable, hold the lock before obtaining
49+
// the account list to prevent concurrent map iteration and write.
50+
dl.lock.RLock()
51+
accountList := dl.buffer.states.accountList()
52+
dl.lock.RUnlock()
53+
4854
// Create two iterators for state buffer and the persistent state in disk
4955
// respectively and combine them as a binary iterator.
5056
l := &binaryIterator{
@@ -54,7 +60,7 @@ func (dl *diskLayer) initBinaryAccountIterator(seek common.Hash) *binaryIterator
5460
// The account key list for iteration is deterministic once the iterator
5561
// is constructed, no matter the referenced disk layer is stale or not
5662
// later.
57-
a: newDiffAccountIterator(seek, dl.buffer.states, nil),
63+
a: newDiffAccountIterator(seek, accountList, nil),
5864
b: newDiskAccountIterator(dl.db.diskdb, seek),
5965
}
6066
l.aDone = !l.a.Next()
@@ -68,28 +74,34 @@ func (dl *diskLayer) initBinaryAccountIterator(seek common.Hash) *binaryIterator
6874
func (dl *diffLayer) initBinaryAccountIterator(seek common.Hash) *binaryIterator {
6975
parent, ok := dl.parent.(*diffLayer)
7076
if !ok {
77+
// The state set in diff layer is immutable and will never be stale,
78+
// so the read lock protection is unnecessary.
79+
accountList := dl.states.stateSet.accountList()
7180
l := &binaryIterator{
7281
// The account loader function is unnecessary; the account key list
7382
// produced by the supplied state set alone is sufficient for iteration.
7483
//
7584
// The account key list for iteration is deterministic once the iterator
7685
// is constructed, no matter the referenced disk layer is stale or not
7786
// later.
78-
a: newDiffAccountIterator(seek, dl.states.stateSet, nil),
87+
a: newDiffAccountIterator(seek, accountList, nil),
7988
b: dl.parent.(*diskLayer).initBinaryAccountIterator(seek),
8089
}
8190
l.aDone = !l.a.Next()
8291
l.bDone = !l.b.Next()
8392
return l
8493
}
94+
// The state set in diff layer is immutable and will never be stale,
95+
// so the read lock protection is unnecessary.
96+
accountList := dl.states.stateSet.accountList()
8597
l := &binaryIterator{
8698
// The account loader function is unnecessary; the account key list
8799
// produced by the supplied state set alone is sufficient for iteration.
88100
//
89101
// The account key list for iteration is deterministic once the iterator
90102
// is constructed, no matter the referenced disk layer is stale or not
91103
// later.
92-
a: newDiffAccountIterator(seek, dl.states.stateSet, nil),
104+
a: newDiffAccountIterator(seek, accountList, nil),
93105
b: parent.initBinaryAccountIterator(seek),
94106
}
95107
l.aDone = !l.a.Next()
@@ -101,6 +113,12 @@ func (dl *diffLayer) initBinaryAccountIterator(seek common.Hash) *binaryIterator
101113
// storage slots in a slow, but easily verifiable way. Note this function is used
102114
// for initialization, use `newBinaryStorageIterator` as the API.
103115
func (dl *diskLayer) initBinaryStorageIterator(account common.Hash, seek common.Hash) *binaryIterator {
116+
// The state set in the disk layer is mutable, hold the lock before obtaining
117+
// the storage list to prevent concurrent map iteration and write.
118+
dl.lock.RLock()
119+
storageList := dl.buffer.states.storageList(account)
120+
dl.lock.RUnlock()
121+
104122
// Create two iterators for state buffer and the persistent state in disk
105123
// respectively and combine them as a binary iterator.
106124
l := &binaryIterator{
@@ -110,7 +128,7 @@ func (dl *diskLayer) initBinaryStorageIterator(account common.Hash, seek common.
110128
// The storage key list for iteration is deterministic once the iterator
111129
// is constructed, no matter the referenced disk layer is stale or not
112130
// later.
113-
a: newDiffStorageIterator(account, seek, dl.buffer.states, nil),
131+
a: newDiffStorageIterator(account, seek, storageList, nil),
114132
b: newDiskStorageIterator(dl.db.diskdb, account, seek),
115133
}
116134
l.aDone = !l.a.Next()
@@ -124,28 +142,34 @@ func (dl *diskLayer) initBinaryStorageIterator(account common.Hash, seek common.
124142
func (dl *diffLayer) initBinaryStorageIterator(account common.Hash, seek common.Hash) *binaryIterator {
125143
parent, ok := dl.parent.(*diffLayer)
126144
if !ok {
145+
// The state set in diff layer is immutable and will never be stale,
146+
// so the read lock protection is unnecessary.
147+
storageList := dl.states.stateSet.storageList(account)
127148
l := &binaryIterator{
128149
// The storage loader function is unnecessary; the storage key list
129150
// produced by the supplied state set alone is sufficient for iteration.
130151
//
131152
// The storage key list for iteration is deterministic once the iterator
132153
// is constructed, no matter the referenced disk layer is stale or not
133154
// later.
134-
a: newDiffStorageIterator(account, seek, dl.states.stateSet, nil),
155+
a: newDiffStorageIterator(account, seek, storageList, nil),
135156
b: dl.parent.(*diskLayer).initBinaryStorageIterator(account, seek),
136157
}
137158
l.aDone = !l.a.Next()
138159
l.bDone = !l.b.Next()
139160
return l
140161
}
162+
// The state set in diff layer is immutable and will never be stale,
163+
// so the read lock protection is unnecessary.
164+
storageList := dl.states.stateSet.storageList(account)
141165
l := &binaryIterator{
142166
// The storage loader function is unnecessary; the storage key list
143167
// produced by the supplied state set alone is sufficient for iteration.
144168
//
145169
// The storage key list for iteration is deterministic once the iterator
146170
// is constructed, no matter the referenced disk layer is stale or not
147171
// later.
148-
a: newDiffStorageIterator(account, seek, dl.states.stateSet, nil),
172+
a: newDiffStorageIterator(account, seek, storageList, nil),
149173
b: parent.initBinaryStorageIterator(account, seek),
150174
}
151175
l.aDone = !l.a.Next()

‎triedb/pathdb/iterator_fast.go

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -76,11 +76,17 @@ func newFastIterator(db *Database, root common.Hash, account common.Hash, seek c
7676
if accountIterator {
7777
switch dl := current.(type) {
7878
case *diskLayer:
79+
// The state set in the disk layer is mutable, hold the lock before obtaining
80+
// the account list to prevent concurrent map iteration and write.
81+
dl.lock.RLock()
82+
accountList := dl.buffer.states.accountList()
83+
dl.lock.RUnlock()
84+
7985
fi.iterators = append(fi.iterators, &weightedIterator{
8086
// The state set in the disk layer is mutable, and the entire state becomes stale
8187
// if a diff layer above is merged into it. Therefore, staleness must be checked,
8288
// and the storage slot should be retrieved with read lock protection.
83-
it: newDiffAccountIterator(seek, dl.buffer.states, func(hash common.Hash) ([]byte, error) {
89+
it: newDiffAccountIterator(seek, accountList, func(hash common.Hash) ([]byte, error) {
8490
dl.lock.RLock()
8591
defer dl.lock.RUnlock()
8692

@@ -98,19 +104,26 @@ func newFastIterator(db *Database, root common.Hash, account common.Hash, seek c
98104
case *diffLayer:
99105
// The state set in diff layer is immutable and will never be stale,
100106
// so the read lock protection is unnecessary.
107+
accountList := dl.states.accountList()
101108
fi.iterators = append(fi.iterators, &weightedIterator{
102-
it: newDiffAccountIterator(seek, dl.states.stateSet, dl.states.mustAccount),
109+
it: newDiffAccountIterator(seek, accountList, dl.states.mustAccount),
103110
priority: depth,
104111
})
105112
}
106113
} else {
107114
switch dl := current.(type) {
108115
case *diskLayer:
116+
// The state set in the disk layer is mutable, hold the lock before obtaining
117+
// the storage list to prevent concurrent map iteration and write.
118+
dl.lock.RLock()
119+
storageList := dl.buffer.states.storageList(account)
120+
dl.lock.RUnlock()
121+
109122
fi.iterators = append(fi.iterators, &weightedIterator{
110123
// The state set in the disk layer is mutable, and the entire state becomes stale
111124
// if a diff layer above is merged into it. Therefore, staleness must be checked,
112125
// and the storage slot should be retrieved with read lock protection.
113-
it: newDiffStorageIterator(account, seek, dl.buffer.states, func(addrHash common.Hash, slotHash common.Hash) ([]byte, error) {
126+
it: newDiffStorageIterator(account, seek, storageList, func(addrHash common.Hash, slotHash common.Hash) ([]byte, error) {
114127
dl.lock.RLock()
115128
defer dl.lock.RUnlock()
116129

@@ -126,10 +139,14 @@ func newFastIterator(db *Database, root common.Hash, account common.Hash, seek c
126139
priority: depth + 1,
127140
})
128141
case *diffLayer:
142+
// The state set in diff layer is immutable and will never be stale,
143+
// so the read lock protection is unnecessary.
144+
storageList := dl.states.storageList(account)
145+
129146
// The state set in diff layer is immutable and will never be stale,
130147
// so the read lock protection is unnecessary.
131148
fi.iterators = append(fi.iterators, &weightedIterator{
132-
it: newDiffStorageIterator(account, seek, dl.states.stateSet, dl.states.mustStorage),
149+
it: newDiffStorageIterator(account, seek, storageList, dl.states.mustStorage),
133150
priority: depth,
134151
})
135152
}

‎triedb/pathdb/iterator_test.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ func TestAccountIteratorBasics(t *testing.T) {
132132
}
133133
}
134134
states := newStates(accounts, storage, false)
135-
it := newDiffAccountIterator(common.Hash{}, states, nil)
135+
it := newDiffAccountIterator(common.Hash{}, states.accountList(), nil)
136136
verifyIterator(t, 100, it, verifyNothing) // Nil is allowed for single layer iterator
137137

138138
db := rawdb.NewMemoryDatabase()
@@ -170,7 +170,7 @@ func TestStorageIteratorBasics(t *testing.T) {
170170
}
171171
states := newStates(accounts, storage, false)
172172
for account := range accounts {
173-
it := newDiffStorageIterator(account, common.Hash{}, states, nil)
173+
it := newDiffStorageIterator(account, common.Hash{}, states.storageList(account), nil)
174174
verifyIterator(t, 100, it, verifyNothing) // Nil is allowed for single layer iterator
175175
}
176176

@@ -273,7 +273,7 @@ func TestAccountIteratorTraversal(t *testing.T) {
273273
head := db.tree.get(common.HexToHash("0x04"))
274274

275275
// singleLayer: 0xcc, 0xf0, 0xff
276-
it := newDiffAccountIterator(common.Hash{}, head.(*diffLayer).states.stateSet, nil)
276+
it := newDiffAccountIterator(common.Hash{}, head.(*diffLayer).states.stateSet.accountList(), nil)
277277
verifyIterator(t, 3, it, verifyNothing)
278278

279279
// binaryIterator: 0xaa, 0xbb, 0xcc, 0xdd, 0xee, 0xf0, 0xff
@@ -317,7 +317,7 @@ func TestStorageIteratorTraversal(t *testing.T) {
317317
head := db.tree.get(common.HexToHash("0x04"))
318318

319319
// singleLayer: 0x1, 0x2, 0x3
320-
diffIter := newDiffStorageIterator(common.HexToHash("0xaa"), common.Hash{}, head.(*diffLayer).states.stateSet, nil)
320+
diffIter := newDiffStorageIterator(common.HexToHash("0xaa"), common.Hash{}, head.(*diffLayer).states.stateSet.storageList(common.HexToHash("0xaa")), nil)
321321
verifyIterator(t, 3, diffIter, verifyNothing)
322322

323323
// binaryIterator: 0x1, 0x2, 0x3, 0x4, 0x5, 0x6
@@ -606,7 +606,7 @@ func TestAccountIteratorLargeTraversal(t *testing.T) {
606606
}
607607
// Iterate the entire stack and ensure everything is hit only once
608608
head := db.tree.get(common.HexToHash("0x80"))
609-
verifyIterator(t, 200, newDiffAccountIterator(common.Hash{}, head.(*diffLayer).states.stateSet, nil), verifyNothing)
609+
verifyIterator(t, 200, newDiffAccountIterator(common.Hash{}, head.(*diffLayer).states.stateSet.accountList(), nil), verifyNothing)
610610
verifyIterator(t, 200, head.(*diffLayer).newBinaryAccountIterator(common.Hash{}), verifyAccount)
611611

612612
it, _ := db.AccountIterator(common.HexToHash("0x80"), common.Hash{})

0 commit comments

Comments
 (0)