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

Add getnetworkhashps to get the estimated network hashrate #2888

Merged
merged 1 commit into from Oct 1, 2013

Conversation

Projects
None yet
8 participants
@coblee
Contributor

coblee commented Aug 9, 2013

This call is useful to mining pools and miners. It lets you get the estimated network hashrate at the current block (or any previous block) with a specified number of blocks to use to calculate the estimate.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Aug 9, 2013

Member

This can be implemented much more accurately by computing the nChainWork difference divided by time difference.

Member

sipa commented Aug 9, 2013

This can be implemented much more accurately by computing the nChainWork difference divided by time difference.

@wtogami

This comment has been minimized.

Show comment
Hide comment
@wtogami

wtogami Aug 9, 2013

Contributor

Should this also be squashed into a single commit for Bitcoin?

Contributor

wtogami commented Aug 9, 2013

Should this also be squashed into a single commit for Bitcoin?

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Aug 20, 2013

Member

I think the commits here look logical (and don't need to be squished)

Member

luke-jr commented Aug 20, 2013

I think the commits here look logical (and don't need to be squished)

@wtogami

This comment has been minimized.

Show comment
Hide comment
@wtogami

wtogami Aug 20, 2013

Contributor

Does @sipa insist on the rewrite to use nChainWork difference?

Contributor

wtogami commented Aug 20, 2013

Does @sipa insist on the rewrite to use nChainWork difference?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Aug 23, 2013

Member

@wtogami Yes, I think it's trivial to do right.

Member

sipa commented Aug 23, 2013

@wtogami Yes, I think it's trivial to do right.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Aug 23, 2013

Member

Beyond also sharing pieter's concerns wrt doing the calculation with pow() instead of directly. I think the default (no parameter) call having a variable integration window is weird. This means if you call it right after a difficulty change you'll get some insane random number. Had I implemented this I likely would have just had a default of 2016 blocks and no options.

Member

gmaxwell commented Aug 23, 2013

Beyond also sharing pieter's concerns wrt doing the calculation with pow() instead of directly. I think the default (no parameter) call having a variable integration window is weird. This means if you call it right after a difficulty change you'll get some insane random number. Had I implemented this I likely would have just had a default of 2016 blocks and no options.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Aug 25, 2013

Contributor

Agree w/ feedback. Would like to see this merged, after revision.

Contributor

jgarzik commented Aug 25, 2013

Agree w/ feedback. Would like to see this merged, after revision.

@Diapolo

This comment has been minimized.

Show comment
Hide comment
@Diapolo

Diapolo Aug 29, 2013

@coblee Any progress on this? Seems to be a nice feature.

Diapolo commented Aug 29, 2013

@coblee Any progress on this? Seems to be a nice feature.

@coblee

This comment has been minimized.

Show comment
Hide comment
@coblee

coblee Aug 29, 2013

Contributor

I will get to it in a few days. Thanks for all the feedback.

Contributor

coblee commented Aug 29, 2013

I will get to it in a few days. Thanks for all the feedback.

@wtogami

This comment has been minimized.

Show comment
Hide comment
@wtogami

wtogami Aug 29, 2013

Contributor

litecoin-project#69
@pooler has implemented sipa's requested change. coblee wants to write tests before submitting this to Bitcoin. I suggest squashing it a bit to have fewer, cleaner commits for Bitcoin.

Contributor

wtogami commented Aug 29, 2013

litecoin-project#69
@pooler has implemented sipa's requested change. coblee wants to write tests before submitting this to Bitcoin. I suggest squashing it a bit to have fewer, cleaner commits for Bitcoin.

@coblee

This comment has been minimized.

Show comment
Hide comment
@coblee

coblee Sep 13, 2013

Contributor

Sorry for the delay, the change is tested and pushed. Please take another look.

@sipa, thanks for the suggestion.

@gmaxwell, the default behavior is to use the previous 120 blocks.

Thanks @pooler for the fix.

Contributor

coblee commented Sep 13, 2013

Sorry for the delay, the change is tested and pushed. Please take another look.

@sipa, thanks for the suggestion.

@gmaxwell, the default behavior is to use the previous 120 blocks.

Thanks @pooler for the fix.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Sep 13, 2013

Member

Code change looks good.

One idea (to be discussed, perhaps): shouldn't be use max(block.time for block in blocks) - min(block.time for block in blocks), to avoid weird edge cases with small numbers of blocks and non-causal timestamps?

Member

sipa commented Sep 13, 2013

Code change looks good.

One idea (to be discussed, perhaps): shouldn't be use max(block.time for block in blocks) - min(block.time for block in blocks), to avoid weird edge cases with small numbers of blocks and non-causal timestamps?

@coblee

This comment has been minimized.

Show comment
Hide comment
@coblee

coblee Sep 13, 2013

Contributor

That's a fair point. I will make that fix. Will also need to handle a divide by zero edge case

Contributor

coblee commented Sep 13, 2013

That's a fair point. I will make that fix. Will also need to handle a divide by zero edge case

@coblee

This comment has been minimized.

Show comment
Hide comment
@coblee

coblee Sep 13, 2013

Contributor

Hmm, I made the change, but looking at it now, I'm not sure it's a good change. Making sure we get correct results for weird edge cases is probably not worth the extra cost in keeping tracking of a min/max over all the blocks, since people won't be calling this method with a small number of blocks. @sipa, what do you think? We can always just do a sanity check to make sure min < max.

Contributor

coblee commented Sep 13, 2013

Hmm, I made the change, but looking at it now, I'm not sure it's a good change. Making sure we get correct results for weird edge cases is probably not worth the extra cost in keeping tracking of a min/max over all the blocks, since people won't be calling this method with a small number of blocks. @sipa, what do you think? We can always just do a sanity check to make sure min < max.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Sep 15, 2013

Member

Another idea (just up for discussion, don't rush implementing this): use the median-of-11 as timestamp for the begin and end block, which guarantees monotonicity (by network rule) and is likely a better estimator for actual time as well. The question is of course which window-of-11 to use; if you use 5-before-until-5-after, it's not usable for the last 5 blocks, but likely unbiased. If you use 10-before-0-after, you get a slight bias if blocks are speeding up/slowing down, but I doubt that'd be noticable on such a short time frame.

Member

sipa commented Sep 15, 2013

Another idea (just up for discussion, don't rush implementing this): use the median-of-11 as timestamp for the begin and end block, which guarantees monotonicity (by network rule) and is likely a better estimator for actual time as well. The question is of course which window-of-11 to use; if you use 5-before-until-5-after, it's not usable for the last 5 blocks, but likely unbiased. If you use 10-before-0-after, you get a slight bias if blocks are speeding up/slowing down, but I doubt that'd be noticable on such a short time frame.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Sep 16, 2013

Member

@sipa How does the estimator you use on sipa.be work?

Member

gmaxwell commented Sep 16, 2013

@sipa How does the estimator you use on sipa.be work?

@coblee

This comment has been minimized.

Show comment
Hide comment
@coblee

coblee Sep 17, 2013

Contributor

I still think there's no need to be so precise with getting the time right since this is just an estimate. With a large enough block count, the difference between each of these solutions (just GetBlockTime, min/max, median-of-11) will be minimal. So not worth the extra cpu cycle to get it "right".

Contributor

coblee commented Sep 17, 2013

I still think there's no need to be so precise with getting the time right since this is just an estimate. With a large enough block count, the difference between each of these solutions (just GetBlockTime, min/max, median-of-11) will be minimal. So not worth the extra cpu cycle to get it "right".

@BitcoinPullTester

This comment has been minimized.

Show comment
Hide comment
@BitcoinPullTester

BitcoinPullTester Sep 17, 2013

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/d64eef48a45d1bbbbab5adf39d6ba1fe4537cea1 for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

BitcoinPullTester commented Sep 17, 2013

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/d64eef48a45d1bbbbab5adf39d6ba1fe4537cea1 for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Sep 29, 2013

Member

@gmaxwell Exponential window that measures rate and average timestamp of past blocks (which seems to be what you need for a most likelyhood estimator for an poisson process whose rate is an exponential function; I'm certainly not able to do the math for that again, but the resulting formula's became ridiculously simple), which is used to guess the parameters of the hashrate growth curve in every point, which on its turn is used to extrapolate the the speed at the current time. Way too complex (and probably too unstable) for using inside the program.

Member

sipa commented Sep 29, 2013

@gmaxwell Exponential window that measures rate and average timestamp of past blocks (which seems to be what you need for a most likelyhood estimator for an poisson process whose rate is an exponential function; I'm certainly not able to do the math for that again, but the resulting formula's became ridiculously simple), which is used to guess the parameters of the hashrate growth curve in every point, which on its turn is used to extrapolate the the speed at the current time. Way too complex (and probably too unstable) for using inside the program.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Oct 1, 2013

Member

ACK

Member

sipa commented Oct 1, 2013

ACK

jgarzik pushed a commit that referenced this pull request Oct 1, 2013

Jeff Garzik
Merge pull request #2888 from litecoin-project/getnetworkhashps
Add getnetworkhashps to get the estimated network hashrate

@jgarzik jgarzik merged commit 19c415b into bitcoin:master Oct 1, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment