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

RPC: Get rid of the internal miner's hashmeter #5599

Merged
merged 1 commit into from Jan 24, 2015
Merged

Conversation

jtimon
Copy link
Contributor

@jtimon jtimon commented Jan 4, 2015

Removes "hashespersec" in "getmininginfo" and also removes "gethashespersec".
That also allows the removal of some code in miner.cpp, simplifying ScanHash() and BitcoinMiner().

@jtimon jtimon changed the title Get rid of the internal miner's hashmeter RPC: Get rid of the internal miner's hashmeter Jan 4, 2015
@gavinandresen
Copy link
Contributor

Code review ACK, but there is a functional change: with this code the internal miner will never search the nonce space above 0xfff.

Also: how did you test this?

@jtimon
Copy link
Contributor Author

jtimon commented Jan 5, 2015

We discussed this in #4793 . @sipa said in some outdated commit that removing if ((nNonce & 0xffff) == 0) was safe but I can't remember the reason why it was that if instead of the later if ((nNonce & 0xfff) == 0) what can be removed.
Now that you're saying it I think it is a mistake and what can be removed is just:

if ((nNonce & 0xfff) == 0)
    boost::this_thread::interruption_point();

@sipa
Copy link
Member

sipa commented Jan 5, 2015

@gavinandresen I haven't tested, but it's just ScanHash that returns after 0x1000 iterations - the mining loop continues with the same nonce until 0xffff0000 is reached (which could actually become 0xfffff000 then, but who cares).

@sipa
Copy link
Member

sipa commented Jan 6, 2015

Tested ACK: adding a LogPrintf("Nonce: %x\n", nNonce); in the loop that calls ScanHash, and running setgenerate true in -testnet mode confirms higher nonces are being tried.

@jtimon
Copy link
Contributor Author

jtimon commented Jan 7, 2015

Oh, here it was the missing piece of earlier conversation: jtimon@9d12ff7#diff-4a59b408ad3778278c3aeffa7da33c3cR384

@laanwj laanwj added the Mining label Jan 8, 2015
@laanwj
Copy link
Member

laanwj commented Jan 8, 2015

@luke-jr this is mining related, can you ACK?

if ((nNonce & 0xfff) == 0)
boost::this_thread::interruption_point();
return false;
Copy link
Member

Choose a reason for hiding this comment

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

I think this is okay, but seems unrelated from the PR description?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@maaku suggested on #4793 when I changed this boost::this_thread::interruption_point() to return false that the change belonged to the removal of the hashmeter as it was the reason for the interruption point to be there in the first place.

@luke-jr
Copy link
Member

luke-jr commented Jan 8, 2015

ACK, only tested on regtest since testnet-in-a-box style apparently refuses to mine in master... :/

@luke-jr
Copy link
Member

luke-jr commented Jan 8, 2015

ACK on testnet via gdb testing

@gavinandresen
Copy link
Contributor

Better is better... so ACK.

But after removing the hashmeter, and assuming we no longer care about performance of the CPU miner (we don't) it looks to me like the code could be simplified a lot by getting rid of ScanHash and just having the mining loop be:

   while true:
       ... create block...
       ... serialize header without nonce..
       set nonce to zero
       do:
           compute hash with nonce
           ++nonce
           if hash <= target: BLOCK FOUND
           if nonce%0xfff == 0 : interruption point
        while nonce < 0xffffffff
        Increment extraNonce
  end while true

@jtimon
Copy link
Contributor Author

jtimon commented Jan 9, 2015

Yes, more simplifications can be done later.
This is what I had in mind for next steps: jtimon@a029a67
jtimon@da36fb5
Different things can be done (my preference is that we share as much code as possible with regtest, optimizing only for regtest), but most of the options I have considered imply the removal of the hashmeter as a first step, so here it is isolated from other decisions that people seem to care less about.

@jaromil
Copy link
Contributor

jaromil commented Jan 24, 2015

ut ACK.

lovely to have this part of the code simplified.

@laanwj laanwj merged commit 0cc0d8d into bitcoin:master Jan 24, 2015
laanwj added a commit that referenced this pull request Jan 24, 2015
0cc0d8d Get rid of the internal miner's hashmeter (jtimon)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants