Proof of work related refactor

jtimon commented at 10:42 am on June 21, 2014: contributor

Continues #3839

jtimon commented at 10:10 am on June 25, 2014: contributor

Maybe I should have just put #4393 into this one… Edit: or even inside the “Decouple UpdateTime from pow” commit.

laanwj commented at 10:21 am on June 25, 2014: member

@drak As it changes how verifying POW works, this looks kind of risky and needs extensive testing and review. It’s not an easy merge like the code move.

jtimon commented at 10:36 am on June 25, 2014: contributor

I think both commits are straight-forward to merge, specially the first one. For the second one you may need to grep UpdateTime and see that nBits are always set after it’s called to believe it. On the other hand #4393 seems more risky, so any advice for modifying or writing new tests you think are useful for this, I guess in miner_test.cpp.

jtimon commented at 3:26 pm on June 26, 2014: contributor

I’ve closed #4393 with @jgarzik ’s input and turned it into a simpler commit 24410f9 here. I’m having second thoughts about UpdateTime again so I won’t include that commit for now.

jtimon commented at 8:26 am on July 7, 2014: contributor

I’m including more commits. All commits are refactors that don’t change functionality but if there’s some of them that are more controversial I can separate them in different PRs.

jtimon commented at 8:42 am on July 7, 2014: contributor

Also, to understand the motivation of this PR you may want to take a look to this unfinished branch that completely encapsulates the Proof of Work, making it easier to experiment with other Proofs:

https://github.com/jtimon/bitcoin/tree/proof

Probably Params().AllowMinDifficultyBlocks() can be removed with an specialized Proof. Well, not probably, for sure, what remains to be seen if it that will simplify the code or not.

laanwj commented at 8:48 am on July 7, 2014: member

Agree on overall changes, haven’t checked the code moves or tested yet.

jaromil commented at 12:40 pm on July 11, 2014: contributor

Ping @sipa - This looks good to me, was wondering around this part of code and now happy to find this PR. It is not invasive and makes the whole POW more readable and maintainable.

But I advise against going forward into #4506 mainly because class polymorphism will affect runtime performance signicantly and can introduce problems using templates.

#4508 looks like a very good move to follow up with this.

jtimon commented at 7:19 pm on July 16, 2014: contributor

I simplified the last two commits and unified them into one after completing #4506

in src/pow.h: in 75ee124c99 outdated

26+bool CheckProofRequired(const CBlockHeader* pblock, const CBlockIndex* pindexPrev);
27+bool GenerateProof(CBlockHeader* pblock, const CBlockIndex* pindexPrev);
28+void UpdateTime(CBlockHeader* pblock, const CBlockIndex* pindexPrev);
29+void ResetProof(CBlockHeader* pblock, const CBlockIndex* pindexPrev);
30+
31+uint256 GetLengthIncrement(unsigned int nBits);

sipa commented at 4:05 pm on July 17, 2014:

That’s a confusing name. How about GetProofIncrement()?

jtimon commented at 7:56 pm on July 17, 2014: contributor

Changed GetLengthIncrement to GetProofIncrement as suggested by @sipa

laanwj commented at 5:52 am on July 20, 2014: member

Tested ACK

laanwj commented at 10:29 am on July 28, 2014: member

I’m not entirely happy that part of the internal miner is moved to pow.cpp (the ugly loop GenerateProof , as well as global variable dHashesPerSec and nHPSTimerStart). Most of this clearly belongs in miner.cpp.

I understand why, but I would prefer if pow.cpp only contained the consensus-critical code that verifies proof of work.

I’d suggest to keep the outer loop in miner.cpp but move an inner pow-dependent part (ScanHash) to pow.cpp.

jtimon commented at 1:08 pm on July 28, 2014: contributor

I’m not entirely happy with the GenerateProof function either. My initial approach was to simply remove the optimizations, but #4423 was rejected. Maybe we could just remove the hashrate counter to simplify things… I’ll explore your proposal in more detail. Perhaps I could leave the “Encapsulate proof of work generation in pow” commit (and the previous one) for #4506 so that this can be merged faster, given that it seems the most controversial change.

jtimon commented at 2:01 pm on July 28, 2014: contributor

Thank you for the feedback. I thought it was good to rebase on top of the latest head often, but yes, that kind of invalidates earlier testing. I’ll update this PR with the most basic commits that have been here for longer and leave the rest for later PRs. And I won’t rebase it until it is required.

jtimon commented at 2:38 pm on July 28, 2014: contributor

Added the additional conversion to CheckMinWork as suggested by @laanwj Just kept the more tested and/or less controversial commits.

laanwj added the label Improvement on Jul 31, 2014

jtimon commented at 2:28 am on August 11, 2014: contributor

Ping. As far as I know I solved all the issues raised on the 3 commits that remain in the PR, right? If there’s anything I missed, please, tell me.

laanwj commented at 7:08 am on August 11, 2014: member

Tested ACK

in src/pow.cpp: in 9c00990b3c outdated

129     }
130     if (bnResult > bnLimit)
131         bnResult = bnLimit;
132-    return bnResult.GetCompact();
133+
134+    // TODO make sure this additional conversion is necessary before deleting it.

sipa commented at 11:02 am on August 23, 2014:

Deleting this conversion won’t hurt. GetCompact + SetCompact will only round the (absolute value of a) uint256 down, so the check becomes stricter. As it’s only a heuristic, loosening it is never a problem. Given that all it does is divide by a power of 4, I expect it to be no-op apart from the last 2 bits anyway.

In short: the test is probably immeasurably stronger by having the conversion.

sipa commented at 11:07 am on August 23, 2014: member

I don’t like keeping TODO’s like the one you mentioned in master code. A comment about why it’s there (if you don’t delete it) or where it used to be (if you do) is fine of course.

I’d keep the conversion.

sipa commented at 11:09 am on August 23, 2014: member

Mildly-tested ACK.

Move UpdateTime to pow c2c02f3fa9

Move CBlockIndex::GetBlockWork() to pow::GetProofIncrement(nBits) b343c1a1e3

replace ComputeMinWork with CheckMinWork 654871d436

jtimon force-pushed on Aug 23, 2014

jtimon commented at 11:24 am on August 23, 2014: contributor

Additional conversion and TODO removed.

BitcoinPullTester commented at 11:38 am on August 23, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4377_654871d43677947d124673c9e0dd2984f0d3ca61/ 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.

jtimon commented at 11:46 am on August 24, 2014: contributor

Sorry, in the comment to the commit you said it would be safe to remove the redundant conversion but later you say you would keep the conversion. I missed that and I removed it again. So I’m confused, should I remove the conversion or only the TODO?

sipa commented at 8:02 pm on August 27, 2014: member

@jtimon It’s fine.

Both are safe; the extra conversion makes the test up to 0.000036% stronger (3 / 2^23). Then again, it’s easy to do too.

jgarzik commented at 8:16 pm on August 27, 2014: contributor

ACK

sipa merged this on Aug 27, 2014

sipa closed this on Aug 27, 2014

sipa referenced this in commit d1062e32fa on Aug 27, 2014

jtimon deleted the branch on Aug 28, 2014

MarcoFalke locked this on Sep 8, 2021

Proof of work related refactor #4377