Skip to content

miner: avoid data race in miner #24349

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

Merged
holiman merged 1 commit into from
Feb 7, 2022
+3 −0
Merged

miner: avoid data race in miner #24349

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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 miner/worker.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1134,6 +1134,9 @@ func (w *worker) commit(env *environment, interval func(), update bool, start ti
if interval != nil {
interval()
}
// Create a local environment copy, avoid the data race with snapshot state.
// https://github.com/ethereum/go-ethereum/issues/24299
env := env.copy()
Copy link
Copy Markdown
Contributor

@holiman holiman Feb 7, 2022

Choose a reason for hiding this comment

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

A few lines up, at line 1118, we already do w.commit(work.copy(), w.fullTaskHook, true, start). Are you sure this fixes it?

Copy link
Copy Markdown
Member Author

@rjl493456442 rjl493456442 Feb 7, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yup, in line 1118, we indeed create the env copy for commit function to isolate the mutation between the commit function and other logic in miner.

However, in the commit function, there are two places can read/write the passed env in the same time. One is updateSnapshot which inits the pending state snapshot from the given env. Another is resultLoop for mutating the state root of the cached env.

Copy link
Copy Markdown
Contributor

@holiman holiman Feb 7, 2022

Choose a reason for hiding this comment

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

I still don't see how this fixes it. Don't the two still use the same instance, only this time a copy of the one that was passed in?

Copy link
Copy Markdown
Member Author

@rjl493456442 rjl493456442 Feb 7, 2022

Choose a reason for hiding this comment

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

If mining is enabled, then the copy of passed env is passed to the sealer and the passed env is passed to the pending state.

block, err := w.engine.FinalizeAndAssemble(w.chain, env.header, env.state, env.txs, env.unclelist(), env.receipts)
if err != nil {
return err
Expand Down