Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core/state/snapshot: fix race condition #24685

Merged
merged 6 commits into from
May 6, 2022
Merged

core/state/snapshot: fix race condition #24685

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ea16199
core/state/snapshot: fix race condition
MariusVanDerWijden Apr 12, 2022
8aaca0c
eth/protocols/snap: fix race condition
MariusVanDerWijden Apr 12, 2022
16d8d70
core/state/snapshot: f whitespace
MariusVanDerWijden Apr 12, 2022
6d4f686
eth/protocols/snap: fix race condition (properly)
MariusVanDerWijden Apr 13, 2022
da5dc9c
eth/protocols/snap: properly fix race on s.Progress
MariusVanDerWijden Apr 27, 2022
08f4188
eth/protocols/snap: nits
MariusVanDerWijden May 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions core/state/snapshot/difflayer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,9 @@ func (dl *diffLayer) Root() common.Hash {

// Parent returns the subsequent layer of a diff layer.
func (dl *diffLayer) Parent() snapshot {
dl.lock.RLock()
defer dl.lock.RUnlock()

return dl.parent
}

Expand Down
37 changes: 23 additions & 14 deletions eth/protocols/snap/sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,8 @@ type Syncer struct {
storageSynced uint64 // Number of storage slots downloaded
storageBytes common.StorageSize // Number of storage trie bytes persisted to disk

extProgress *SyncProgress // progress that can be exposed to external caller.

// Request tracking during healing phase
trienodeHealIdlers map[string]struct{} // Peers that aren't serving trie node requests
bytecodeHealIdlers map[string]struct{} // Peers that aren't serving bytecode requests
Expand Down Expand Up @@ -477,6 +479,8 @@ func NewSyncer(db ethdb.KeyValueStore) *Syncer {
trienodeHealReqs: make(map[uint64]*trienodeHealRequest),
bytecodeHealReqs: make(map[uint64]*bytecodeHealRequest),
stateWriter: db.NewBatch(),

extProgress: new(SyncProgress),
}
}

Expand Down Expand Up @@ -633,6 +637,21 @@ func (s *Syncer) Sync(root common.Hash, cancel chan struct{}) error {
s.assignTrienodeHealTasks(trienodeHealResps, trienodeHealReqFails, cancel)
s.assignBytecodeHealTasks(bytecodeHealResps, bytecodeHealReqFails, cancel)
}
// Update sync progress
s.lock.Lock()
s.extProgress = &SyncProgress{
AccountSynced: s.accountSynced,
AccountBytes: s.accountBytes,
BytecodeSynced: s.bytecodeSynced,
BytecodeBytes: s.bytecodeBytes,
StorageSynced: s.storageSynced,
StorageBytes: s.storageBytes,
TrienodeHealSynced: s.trienodeHealSynced,
TrienodeHealBytes: s.trienodeHealBytes,
BytecodeHealSynced: s.bytecodeHealSynced,
BytecodeHealBytes: s.bytecodeHealBytes,
}
s.lock.Unlock()
MariusVanDerWijden marked this conversation as resolved.
Show resolved Hide resolved
// Wait for something to happen
select {
case <-s.update:
Expand Down Expand Up @@ -705,6 +724,9 @@ func (s *Syncer) loadSyncStatus() {
}
}
}
s.lock.Lock()
defer s.lock.Unlock()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't fix the issue unfortunately. These fields below are updated throughout the snap sync process locklessly. They were meant to be modified only by the syner loop and not accessed from the outside. Adding a lock everywhere will become super brittle.

Trying to figure out what the best approach would be..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Using atomic would mess up the struct internals as they need to be 64 byte aligned not to crach on 32 bit platforms.
  • Using locks everywhere to update these fields are yuck
  • Requesting the progress via the syncer via a channel would lead to arbitrary timings since the syncer might be hanging on some leveldb-compaction write.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy it out onto a second one (intended for external exposure) every once in a while, and use locks on that one?


s.snapped = len(s.tasks) == 0

s.accountSynced = progress.AccountSynced
Expand Down Expand Up @@ -802,25 +824,12 @@ func (s *Syncer) saveSyncStatus() {
func (s *Syncer) Progress() (*SyncProgress, *SyncPending) {
s.lock.Lock()
defer s.lock.Unlock()

progress := &SyncProgress{
AccountSynced: s.accountSynced,
AccountBytes: s.accountBytes,
BytecodeSynced: s.bytecodeSynced,
BytecodeBytes: s.bytecodeBytes,
StorageSynced: s.storageSynced,
StorageBytes: s.storageBytes,
TrienodeHealSynced: s.trienodeHealSynced,
TrienodeHealBytes: s.trienodeHealBytes,
BytecodeHealSynced: s.bytecodeHealSynced,
BytecodeHealBytes: s.bytecodeHealBytes,
}
pending := new(SyncPending)
if s.healer != nil {
pending.TrienodeHeal = uint64(len(s.healer.trieTasks))
pending.BytecodeHeal = uint64(len(s.healer.codeTasks))
}
return progress, pending
return s.extProgress, pending
}

// cleanAccountTasks removes account range retrieval tasks that have already been
Expand Down