Skip to content

Proof of work related refactor #4377

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.

Sign up for GitHub

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

Jump to bottom
Merged
sipa merged 3 commits into from
Aug 27, 2014
Merged

Proof of work related refactor #4377

sipa merged 3 commits into from
Aug 27, 2014

Conversation

@jtimon
Copy link
Contributor

@jtimon jtimon commented Jun 21, 2014

Continues #3839

@jtimon
Copy link
Contributor Author

jtimon commented Jun 25, 2014

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

@laanwj
Copy link
Member

laanwj commented Jun 25, 2014

@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
Copy link
Contributor Author

jtimon commented Jun 25, 2014

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
Copy link
Contributor Author

jtimon commented Jun 26, 2014

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
Copy link
Contributor Author

jtimon commented Jul 7, 2014

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
Copy link
Contributor Author

jtimon commented Jul 7, 2014

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
Copy link
Member

laanwj commented Jul 7, 2014

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

@jtimon jtimon mentioned this pull request Jul 10, 2014
Closed
@jaromil
Copy link
Contributor

jaromil commented Jul 11, 2014

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
Copy link
Contributor Author

jtimon commented Jul 16, 2014

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

sipa
sipa reviewed Jul 17, 2014
View reviewed changes
src/pow.h Outdated
Copy link
Member

@sipa sipa Jul 17, 2014

Choose a reason for hiding this comment

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

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

@jtimon
Copy link
Contributor Author

jtimon commented Jul 17, 2014

Changed GetLengthIncrement to GetProofIncrement as suggested by @sipa

@laanwj
Copy link
Member

laanwj commented Jul 20, 2014

Tested ACK

@laanwj
Copy link
Member

laanwj commented Jul 28, 2014

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
Copy link
Contributor Author

jtimon commented Jul 28, 2014

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 jtimon mentioned this pull request Jul 28, 2014
Closed
@jtimon
Copy link
Contributor Author

jtimon commented Jul 28, 2014

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
Copy link
Contributor Author

jtimon commented Jul 28, 2014

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

@jtimon
Copy link
Contributor Author

jtimon commented Aug 11, 2014

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
Copy link
Member

laanwj commented Aug 11, 2014

Tested ACK

sipa
sipa reviewed Aug 23, 2014
View reviewed changes
src/pow.cpp Outdated
Copy link
Member

@sipa sipa Aug 23, 2014

Choose a reason for hiding this comment

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

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
Copy link
Member

sipa commented Aug 23, 2014

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
Copy link
Member

sipa commented Aug 23, 2014

Mildly-tested ACK.

@jtimon
Copy link
Contributor Author

jtimon commented Aug 23, 2014

Additional conversion and TODO removed.

@BitcoinPullTester
Copy link

BitcoinPullTester commented Aug 23, 2014

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
Copy link
Contributor Author

jtimon commented Aug 24, 2014

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
Copy link
Member

sipa commented Aug 27, 2014

@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
Copy link
Contributor

jgarzik commented Aug 27, 2014

ACK

@sipa sipa merged commit 654871d into bitcoin:master Aug 27, 2014
sipa added a commit that referenced this pull request Aug 27, 2014
@sipa
Merge pull request #4377
d1062e3 
654871d replace ComputeMinWork with CheckMinWork (jtimon)
b343c1a Move CBlockIndex::GetBlockWork() to pow::GetProofIncrement(nBits) (jtimon)
c2c02f3 Move UpdateTime to pow (jtimon)
@jtimon jtimon deleted the pow2 branch August 28, 2014 08:36
@jtimon jtimon mentioned this pull request Aug 30, 2014
Closed
@jtimon jtimon mentioned this pull request Oct 29, 2014
Closed
@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.

Reviewers

No reviews

Assignees

No one assigned

Labels

Refactoring

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@jtimon @laanwj @jaromil @sipa @BitcoinPullTester @jgarzik