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.

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

Fix Data Race in Epoch Boundary #13680

Merged
merged 2 commits into from
Mar 1, 2024
Merged

Fix Data Race in Epoch Boundary #13680

merged 2 commits into from
Mar 1, 2024

Conversation

nisdas
Copy link
Member

@nisdas nisdas commented Mar 1, 2024

What type of PR is this?

Bug Fix

What does this PR do? Why is it needed?

WARNING: DATA RACE
Write at 0x00c0b095b278 by goroutine 70382:
  github.com/prysmaticlabs/prysm/v5/beacon-chain/blockchain.(*Service).updateEpochBoundaryCaches()
      beacon-chain/blockchain/process_block.go:329 +0x509
  github.com/prysmaticlabs/prysm/v5/beacon-chain/blockchain.(*Service).handleEpochBoundary()
      beacon-chain/blockchain/process_block.go:360 +0x29a
  github.com/prysmaticlabs/prysm/v5/beacon-chain/blockchain.(*Service).lateBlockTasks()
      beacon-chain/blockchain/process_block.go:611 +0x74a
  github.com/prysmaticlabs/prysm/v5/beacon-chain/blockchain.(*Service).runLateBlockTasks()
      beacon-chain/blockchain/process_block.go:480 +0x3a4
  github.com/prysmaticlabs/prysm/v5/beacon-chain/blockchain.(*Service).Start.func2()
      beacon-chain/blockchain/service.go:214 +0x33

Previous read at 0x00c0b095b278 by goroutine 571785:
  github.com/prysmaticlabs/prysm/v5/beacon-chain/blockchain.(*Service).updateEpochBoundaryCaches.func1()
      beacon-chain/blockchain/process_block.go:316 +0xb9

Goroutine 70382 (running) created at:
  github.com/prysmaticlabs/prysm/v5/beacon-chain/blockchain.(*Service).Start()
      beacon-chain/blockchain/service.go:214 +0x304
  github.com/prysmaticlabs/prysm/v5/runtime.(*ServiceRegistry).StartAll.func1()
      runtime/service_registry.go:46 +0x42

Goroutine 571785 (running) created at:
  github.com/prysmaticlabs/prysm/v5/beacon-chain/blockchain.(*Service).updateEpochBoundaryCaches()
      beacon-chain/blockchain/process_block.go:310 +0x257
  github.com/prysmaticlabs/prysm/v5/beacon-chain/blockchain.(*Service).handleEpochBoundary()
      beacon-chain/blockchain/process_block.go:360 +0x29a
  github.com/prysmaticlabs/prysm/v5/beacon-chain/blockchain.(*Service).lateBlockTasks()
      beacon-chain/blockchain/process_block.go:611 +0x74a
  github.com/prysmaticlabs/prysm/v5/beacon-chain/blockchain.(*Service).runLateBlockTasks()
      beacon-chain/blockchain/process_block.go:480 +0x3a4
  github.com/prysmaticlabs/prysm/v5/beacon-chain/blockchain.(*Service).Start.func2()
      beacon-chain/blockchain/service.go:214 +0x33

Which issues(s) does this PR fix?

There have been reports of odd deadlocks or high resource consumption on prysm nodes. In the above reported race, it is possible for the main goroutine to execute before the child goroutine, leading to the epoch being 1 lesser than the one we want. If this happens we will miss caching the next epoch's shuffling.

Other notes for review

@nisdas nisdas added the Ready For Review A pull request ready for code review label Mar 1, 2024
@nisdas nisdas requested a review from a team as a code owner March 1, 2024 09:00
@nisdas nisdas enabled auto-merge March 1, 2024 09:00
@nisdas nisdas added this pull request to the merge queue Mar 1, 2024
Copy link
Contributor

@potuz potuz left a comment

Choose a reason for hiding this comment

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

LGTM

Merged via the queue into develop with commit 68b78dd Mar 1, 2024
17 checks passed
@nisdas nisdas deleted the fixDataRace branch March 1, 2024 09:36
prestonvanloon pushed a commit that referenced this pull request Mar 1, 2024
(cherry picked from commit 68b78dd)
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants