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

Projects
None yet
3 participants
@karalabe
Copy link
Member

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 Aug 8, 2018

@karalabe karalabe force-pushed the karalabe:miner-notify branch 2 times, most recently from dc470bf to dda1f91 Aug 9, 2018

@karalabe karalabe added this to the 1.8.14 milestone Aug 9, 2018

@karalabe

This comment has been minimized.

Copy link
Member Author

karalabe commented Aug 9, 2018

// 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 {

This comment has been minimized.

@rjl493456442

rjl493456442 Aug 9, 2018

Member

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

This comment has been minimized.

@karalabe

karalabe Aug 9, 2018

Author Member

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

@ppratscher

This comment has been minimized.

Copy link
Contributor

ppratscher commented Aug 9, 2018

LGTM!

@rjl493456442

This comment has been minimized.

Copy link
Member

rjl493456442 commented Aug 9, 2018

Just one comment, otherwise LGTM

@karalabe karalabe force-pushed the karalabe:miner-notify branch from dda1f91 to f099841 Aug 10, 2018

@karalabe

This comment has been minimized.

Copy link
Member Author

karalabe commented Aug 10, 2018

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

@rjl493456442
Copy link
Member

rjl493456442 left a comment

LGTM

@karalabe karalabe merged commit d8328a9 into ethereum:master Aug 13, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
commit-message-check/gitcop All commit messages are valid
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment