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

cmd, consensus/ethash, eth: miner push notifications #17347

Merged
merged 1 commit into from
Aug 13, 2018

Conversation

karalabe
Copy link
Member

@karalabe karalabe commented Aug 8, 2018

This PR introduces a new flag --miner.notify, which accepts a comma-separated list of URLs to push miner work packages to. It's main use case is for mining pools to be pinged by new blocks as fast as possible, without requiring to constantly poll the miner.getWork endpoint.

Supersedes #17324.

@karalabe karalabe requested a review from zsfelfoldi as a code owner August 8, 2018 09:16
@karalabe karalabe force-pushed the miner-notify branch 2 times, most recently from dc470bf to dda1f91 Compare August 9, 2018 09:23
@karalabe karalabe added this to the 1.8.14 milestone Aug 9, 2018
@karalabe
Copy link
Member Author

karalabe commented Aug 9, 2018

@ppratscher @rjl493456442 PTAL

// getWork returns a work package for external miner.
for i, url := range notify {
// Terminate any previously pending request and create the new work
if notifyReqs[i] != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Should we set the notifyReq[i] as nil if the request has been sent successfully?

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be racey. To be fair however, this current code might be racey too. I'll think about it.

@peterbitfly
Copy link
Contributor

LGTM!

@rjl493456442
Copy link
Member

Just one comment, otherwise LGTM

@karalabe
Copy link
Member Author

@rjl493456442 I fixed the data race by passing notifyReqs[i] as a parameter to the closure instead of only the index. This way the remote goroutine retains ownership over notifyReqs. I've also added a tests that pushes a lot of work packets fast. Previously the race detector was whining, now it's happy. PTAL.

Copy link
Member

@rjl493456442 rjl493456442 left a comment

Choose a reason for hiding this comment

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

LGTM

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.

3 participants