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

miner: avoid data race in miner #24349

Merged
merged 1 commit into from Feb 7, 2022

Conversation

rjl493456442
Copy link
Member

@rjl493456442 rjl493456442 commented Feb 7, 2022

This PR fixes the #24299. It's an alternative fix of #24301

@@ -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
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
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.

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
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
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.

holiman
holiman approved these changes Feb 7, 2022
Copy link
Contributor

@holiman holiman left a comment

LGTM

@holiman holiman added this to the 1.10.16 milestone Feb 7, 2022
@holiman holiman merged commit 2d20fed into ethereum:master Feb 7, 2022
1 check passed
sidhujag pushed a commit to syscoin/go-ethereum that referenced this pull request Feb 11, 2022
MariusVanDerWijden pushed a commit to holiman/go-ethereum that referenced this pull request Feb 25, 2022
JacekGlen pushed a commit to JacekGlen/go-ethereum that referenced this pull request May 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants