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: copy pending state so we don't accidentally modify it #3162

Merged
merged 1 commit into from
Oct 18, 2016

Conversation

Arachnid
Copy link
Contributor

No description provided.

@mention-bot
Copy link

@Arachnid, thanks for your PR! By analyzing the history of the files in this pull request, we identified @zsfelfoldi, @karalabe and @fjl to be potential reviewers.

@@ -74,7 +74,7 @@ func (b *EthApiBackend) StateAndHeaderByNumber(blockNr rpc.BlockNumber) (ethapi.
// Pending state is only known by the miner
if blockNr == rpc.PendingBlockNumber {
block, state := b.eth.miner.Pending()
return EthApiState{state}, block.Header(), nil
return EthApiState{state.Copy()}, block.Header(), nil
Copy link
Member

Choose a reason for hiding this comment

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

The copy can also be made in miner.Pending() itself. That would prevents similar errors when other parts of the code base try to (mis)use the pending state.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems it is making a copy there. I can't explain why that doesn't fix the issue, only confirm that adding the copy here definitely does.

Copy link
Member

Choose a reason for hiding this comment

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

It makes only a copy when the node is mining. Otherwise it doesn't. I'm not sure why this distinction was made in the past.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spotting. I've fixed the miner instead.

@fjl fjl merged commit c9471e7 into ethereum:develop Oct 18, 2016
@fjl fjl removed the review label Oct 18, 2016
@fjl fjl changed the title eth: Make a copy of pending state so we don't accidentally modify it miner: copy pending state so we don't accidentally modify it Oct 18, 2016
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

6 participants