-
Notifications
You must be signed in to change notification settings - Fork 20.5k
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
Conversation
dc470bf
to
dda1f91
Compare
@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 { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
LGTM! |
Just one comment, otherwise LGTM |
@rjl493456442 I fixed the data race by passing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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 theminer.getWork
endpoint.Supersedes #17324.