Pow: Refactor: Encapsulate miner-related pow in GenerateProof (was ScanHash) and use it for regtest mining #4793

jtimon · 2014-08-30T10:10:31Z

Continues #4506.
Following @laanwj 's advice of only moving ScanHash, I found another solution to encapsulate the miner's pow without removing the miner's optimization, which was rejected in #4423
But I would like to slightly change the behavior.
Commit "Replace proof.nNonce >= 0xffff000 with an iteration counter" should be either rejected or squashed into "proof.nNonce >= 0xffff0000 -> proof.OutOfRangeSolution()" (and of course that 1000 in the for is open for bike-shedding).
"Small miner optimization" also should be rejected or rebased in "Remove testnet's special case from miner.cpp".
I think these 2 functional changes are meaningless enough that can be safely accepted.
I would also be happy to just remove the hashmeter and the resulting PR would be simpler.

jtimon · 2014-09-01T19:20:37Z

Closing as it is included in #4794 with only one commit more.

jtimon · 2014-09-02T11:28:14Z

Reopened as independent from #4506

BitcoinPullTester · 2014-09-02T11:42:27Z

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4793_cf07ea708c5822687ea9ef9c70a37324843a43b4/ 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 · 2014-10-08T22:58:58Z

src/miner.cpp

Why turn this into a global? It's ugly, and I don't think it will actually work in case of multiple hasher threads...

I don't know of any other way to encapsulate the hashmeter inside the function (which will later be a method of CProof). I already asked this and nobody answered, but would anyone oppose to just removing the hashmeter? It would be simpler and cleaner.

Internal miner should only be used for testing, so I vote to simplify and remove the hash meter.

jtimon · 2014-10-10T09:26:45Z

@gavinandresen that's the reason why I wanted to get rid of the optimization (which doesn't really help much in regtest mode) in #4423 but I'm happy removing the hashmeter.

jtimon · 2014-10-10T20:34:51Z

The last commit (Remove gethashespersec() from rpcmining) is completely optional and can be moved to another PR.

sipa · 2014-10-10T20:47:52Z

Concept ACK and code looks good.

We may want to deprecation period for gethashespersec() though, and just having it return 0 is not really acceptable. Having it return an error would be fine to me as nobody should really be depending on it anymore.

TheBlueMatt · 2014-10-11T07:38:45Z

utACK. I dont think we need a deprecation period for anything related to the internal miner.

jtimon · 2014-10-11T17:20:06Z

Waiting to get more consensus before changing the rpc stuff (in case we decide to leave it as it is now).
I prefer to just remove it but I have no strong opinion on this.

jtimon · 2014-10-14T08:01:03Z

So counting opinions on the RPC stuff...

A) Just remove gethashespersec RPC call in this PR ( TheBlueMatt, jtimon ) total 2
B) Leave gethashespersec but throw an error when somebody calls it (sipa) total 1
C) Let gethashespersec return always 0 and leave the RPC for another PR (nobody) total 0

I don't really have a strong opinion on this. I'm not in a hurry to remove the RPC call.
Please, more opinions are greatly appreciated.

gmaxwell · 2014-10-14T08:05:41Z

I think gethashespersec is more or less worthless. I don't think I've heard of anyone using it in a long time. (IIRC it was even broken for a while). I don't really have a strong opinion, but I think this is an unimportant enough RPC that removing it shoudl be fine.

@jgarzik has had opinions about the internal miner before. @luke-jr often has mining opinions

Diapolo · 2014-10-14T08:52:14Z

Remove it gethashespersec, as long as the internal miner is working I'm fine with that ;).

laanwj · 2014-10-14T08:56:56Z

As said before, sometimes I wonder why we don't just nuke the internal miner, instead of death by a thousand cuts.

Anyhow - yes I'm fine with removing gethashespersec.

sipa · 2014-10-14T22:54:30Z

ut ACK

jtimon · 2014-10-22T01:22:14Z

Pushed one additional commit to avoid using boost on ScanHash().
Instead it returns false, and just after that boost::this_thread::interruption_point() was called anyway.

maaku · 2014-10-28T17:36:02Z

utACK

jtimon · 2015-03-26T16:25:59Z

Needed rebase

sipa · 2015-04-07T22:08:34Z

@jtimon #5957 includes something similar to this (but goes further). I forgot you had a PR still open for this.

jtimon · 2015-04-08T22:12:03Z

Closing in favor of #5957, although waiting for some nits to be solved.
I plan to remove the CheckProofOfWork() warnings as part of the libconsensus work to expose VerifyBlockHeader() soon.

jtimon · 2015-04-10T09:34:02Z

src/miner.cpp

This log could be removed too for additional regtest performance.

jtimon · 2015-04-10T09:48:25Z

Rebased and reopened.

jtimon · 2015-04-20T20:15:54Z

Needed rebase

laanwj · 2015-05-06T14:58:31Z

src/miner.cpp

What is this "1000" magic value based on?

laanwj · 2015-05-06T15:05:50Z

Most of the comments in this PR do not seem to apply to the actual code change here anymore.

What is the motivation for this change?

jtimon · 2015-05-06T15:37:48Z

@laanwj instead of checking for the overflow of the nNonce, the magic 1000 is used. This does not check all possible values of the nonce before changing the extra nonce, but that shouldn't be a problem.
It can be make a constant, sure. I was hoping that @sipa @luke-jr or someone less lazy than me for base conversions and hexadecimal would say something like "since ScanHash returns false when pblock->nNonce & 0xfff) == 0 you can safely change the 1000 for X and we will still exhaust all the possibilities with the 32 bits of the nonce"..
We should probably make that 0xfff a constant as well. Any suggestions for the constants are welcomed.

The PR has many comments about the removal of the hashmeter, which was originally part of this, but was already merged separately.
This was part of the effort to encapsulate the proof of work and it reduces the number of places in mining where nBits and nNonce are used.
It should also make regtest slightly faster, but I haven't tested that. It also makes regtest and non-regtest mining more similar.

jtimon · 2015-06-25T19:25:49Z

ping

…nst CBlockHeader*, uint32_t&, uint256*) and use it in regtest Also, stop using multiple threads for mining in with regtest

jtimon · 2015-07-04T10:07:16Z

Needed rebase. Also, I stopped using multiple threads for mining with regtest.

jgarzik · 2015-07-23T18:11:43Z

Closing this old PR. This was a very confusing PR as @laanwj notes, as the associated commits keep changing, garnering ACKs, getting closed, getting re-opened.

Please open a new PR with a specific focused change, and a fresh PR summary description.

jtimon force-pushed the proof2 branch from 0e6f541 to 4321a72 Compare August 30, 2014 10:39

jtimon mentioned this pull request Aug 30, 2014

Finish Proof class #4794

Closed

jtimon force-pushed the proof2 branch 2 times, most recently from 69edb4f to c4c4b5a Compare September 1, 2014 00:57

jtimon closed this Sep 1, 2014

jtimon deleted the proof2 branch September 1, 2014 19:20

jtimon changed the title ~~Encapsulate miner-related pow in Proof class~~ Encapsulate miner-related pow in ScanHash Sep 2, 2014

jtimon restored the proof2 branch September 2, 2014 11:07

jtimon reopened this Sep 2, 2014

jtimon force-pushed the proof2 branch from c4c4b5a to cf07ea7 Compare September 2, 2014 11:28

sipa reviewed Oct 8, 2014
View reviewed changes

jtimon force-pushed the proof2 branch 2 times, most recently from 72dd03d to 0810550 Compare October 10, 2014 20:33

jtimon mentioned this pull request Oct 21, 2014

Create Proof(OfWork) class #4506

Closed

laanwj added the Refactoring label Oct 29, 2014

jtimon changed the title ~~Pow: Refactor: Encapsulate miner-related pow in GenerateProof (was ScanHash) amd use it for regtest mining~~ Pow: Refactor: Encapsulate miner-related pow in GenerateProof (was ScanHash) and use it for regtest mining Apr 6, 2015

jtimon closed this Apr 8, 2015

jtimon reopened this Apr 10, 2015

jtimon force-pushed the proof2 branch from c8b1908 to bd8df51 Compare April 10, 2015 09:29

jtimon mentioned this pull request Apr 10, 2015

Several mining-related improvements #5957

Merged

jtimon reviewed Apr 10, 2015
View reviewed changes

src/miner.cpp Outdated

Copy link

Contributor Author

jtimon Apr 10, 2015

Choose a reason for hiding this comment

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

This log could be removed too for additional regtest performance.

jtimon force-pushed the proof2 branch 2 times, most recently from a663c23 to 260f19c Compare April 10, 2015 09:45

jonasschnelli mentioned this pull request Apr 10, 2015

detach wallet from miner #5994

Merged

jtimon mentioned this pull request Apr 20, 2015

Consensus: Consensus version of pow functions #6037

Closed

jtimon force-pushed the proof2 branch 3 times, most recently from 329d930 to 4ff179d Compare April 20, 2015 20:14

laanwj reviewed May 6, 2015
View reviewed changes

src/miner.cpp

Copy link

Member

laanwj May 6, 2015

Choose a reason for hiding this comment

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

What is this "1000" magic value based on?

Miner: Refactor: Create GenerateProof(CBlockHeader*) from ScanHash(co…

7e4758a

…nst CBlockHeader*, uint32_t&, uint256*) and use it in regtest Also, stop using multiple threads for mining in with regtest

jtimon force-pushed the proof2 branch from 4ff179d to 7e4758a Compare July 4, 2015 10:06

jgarzik closed this Jul 23, 2015

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Pow: Refactor: Encapsulate miner-related pow in GenerateProof (was ScanHash) and use it for regtest mining #4793

Pow: Refactor: Encapsulate miner-related pow in GenerateProof (was ScanHash) and use it for regtest mining #4793

Uh oh!

Conversation

jtimon commented Aug 30, 2014

Uh oh!

jtimon commented Sep 1, 2014

Uh oh!

jtimon commented Sep 2, 2014

Uh oh!

BitcoinPullTester commented Sep 2, 2014

Uh oh!

sipa Oct 8, 2014

Choose a reason for hiding this comment

Uh oh!

jtimon Oct 9, 2014

Choose a reason for hiding this comment

Uh oh!

gavinandresen Oct 9, 2014

Choose a reason for hiding this comment

Uh oh!

jtimon commented Oct 10, 2014

Uh oh!

jtimon commented Oct 10, 2014

Uh oh!

sipa commented Oct 10, 2014

Uh oh!

TheBlueMatt commented Oct 11, 2014

Uh oh!

jtimon commented Oct 11, 2014

Uh oh!

jtimon commented Oct 14, 2014

Uh oh!

gmaxwell commented Oct 14, 2014

Uh oh!

Diapolo commented Oct 14, 2014

Uh oh!

laanwj commented Oct 14, 2014

Uh oh!

sipa commented Oct 14, 2014

Uh oh!

jtimon commented Oct 22, 2014

Uh oh!

maaku commented Oct 28, 2014

Uh oh!

jtimon commented Mar 26, 2015

Uh oh!

sipa commented Apr 7, 2015

Uh oh!

jtimon commented Apr 8, 2015

Uh oh!

jtimon Apr 10, 2015

Choose a reason for hiding this comment

Uh oh!

jtimon commented Apr 10, 2015

Uh oh!

jtimon commented Apr 20, 2015

Uh oh!

laanwj May 6, 2015

Choose a reason for hiding this comment

Uh oh!

laanwj commented May 6, 2015

Uh oh!

jtimon commented May 6, 2015

Uh oh!

jtimon commented Jun 25, 2015

Uh oh!

jtimon commented Jul 4, 2015

Uh oh!

jgarzik commented Jul 23, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants