Create Proof(OfWork) class

jtimon commented at 4:15 pm on July 10, 2014: contributor

Continues #4377 The goal of this PR is to make easier to experiment on alternative proofs and at the same time improve bitcoind’s modularity. IT is very possible that this will never make it into master, and that’s fine, but ideally it would be very easy to maintain the branch by continuously rebasing on top of bitcoin/master. I open this as a PR to get some feedback on what’s acceptable for master and what is not, and maybe also to improve the code (specially the ugly hack for the serialization, which apparently doesn’t work anyway). It should also help understand #4377 better. @jaromil is going to work on an alternative using function pointers.

jtimon commented at 5:07 pm on July 11, 2014: contributor

I got some feedback on #4377 and other places, I’m closing since this is not acceptable for bitcoind and people have already seen it. I will reopen it with new version still uses a class but without polymorphism. Feedback is still welcomed.

jtimon closed this on Jul 11, 2014

jtimon renamed this:
~~Encapsulate Proof of work behind an abstract class~~
Create ProofOfWork class
on Jul 11, 2014

jtimon commented at 9:47 pm on July 11, 2014: contributor

It is not completely encapsulated because the attributes are public. No polymorphism now. Less interesting for experimentation because there’s no factory, but it should still be useful and in my opinion makes the POW code more clear. For those who may want to see it, I left the polymorphic version here: https://github.com/jtimon/bitcoin/tree/old_proof

jtimon reopened this on Jul 11, 2014

jgarzik commented at 9:55 pm on July 11, 2014: contributor

In general cleaning things up is pretty nice, and POW class seems to do that. Specific comments:

some places seem to get less clean, not more:

0-            if (GenerateProof(pblock, pindexPrev))
1+            if (pblock->proof.GenerateProof(pblock, pindexPrev))

The Hash(block header) code appears to be slower and more complex. Not the right direction for such a speed-sensitive area. That may be a problem for a cleanup we like but don’t need…

jtimon commented at 10:07 pm on July 11, 2014: contributor

Updated removing a bunch of unnecessary #include “pow.h”

in src/core.cpp: in 225434804b outdated

247@@ -248,7 +248,12 @@ uint64_t CTxOutCompressor::DecompressAmount(uint64_t x)
248 
249 uint256 CBlockHeader::GetHash() const
250 {
251-    return Hash(BEGIN(nVersion), END(nNonce));
252+    CHashWriter ss(SER_GETHASH, PROTOCOL_VERSION);

sipa commented at 10:09 pm on July 11, 2014:

Just use “return SerializeHash(*this);” here, and have its IMPLEMENT_SERIALIZE contain a “READWRITE(proof);”.

jtimon commented at 10:51 pm on July 11, 2014: contributor

Updated CBlockHeader::GetHash() with @sipa ’s suggestion, which removes the extra complexity concern. I haven’t measured the performance hit, but I expect it to be small. @jgarzik I agree the call to GenerateProof is quite ugly, but nothing comes to mind to make it prettier…

in src/pow.cpp: in 540ca78279 outdated

314+    UpdateTime(pindexPrev);
315+    nBits = GetNextWorkRequired(pindexPrev, GetBlockTime());
316+    nNonce = 0;
317+}
318+
319+uint256 ProofOfWork::GetLengthIncrement() const

sipa commented at 11:24 pm on July 11, 2014:

Maybe GetWork() is a better name?

sipa commented at 11:27 pm on July 11, 2014: member

CBlockIndex could gain a CheckWork() method that just calls proof.CheckWork(GetBlockHash()).

jtimon commented at 8:32 pm on July 14, 2014: contributor

I created the CheckProof() method for CBlockHeader and CBlockIndex. I could have also added GenerateProof to CBlockHeader or CBlock, but the ugly call is only on miner.cpp (which could disappear in the future, who knows) so I don’t think it’s worth it. It doesn’t feel like it belongs to a core library since it is only for the miner (which is only used for testing).

jtimon commented at 11:01 pm on July 29, 2014: contributor

Commits reordered so that the PR is easier to read (by creating the Proof class early on and then slowly encapsulating things). I left the proof generation part (mining) for later, so GetChallengeUint() method is needed for now SetSolutionUint() is also used in miner.cpp, but that was already used miner_tests.cpp.

laanwj added the label Improvement on Jul 31, 2014

jtimon force-pushed on Aug 27, 2014

jtimon renamed this:
~~Create ProofOfWork class~~
Create Proof(OfWork) class
on Aug 27, 2014

jtimon force-pushed on Aug 28, 2014

jtimon commented at 2:13 pm on August 28, 2014: contributor

I have left the last two commits with multiple small changes for later, after I know what’s the most acceptable way to encapsulate the pow around miner. I will continue that discussion in #4423. But the “final touches” will depend on that, so there’s no reason to include them before or stop this PR until that is resolved. Also the last commit was quite noisy.

jtimon commented at 6:29 pm on August 28, 2014: contributor

mhmm, I wonder if nBits and nNonce should be uint32_t instead of unsigned int…

sipa commented at 8:42 pm on August 28, 2014: member

Yes, I believe all integers being serialized anywhere should eventually be turned into (u)intN_t’s, but let’s do that separately. There may already have been some PR for (part of) that.

jtimon force-pushed on Aug 30, 2014

jtimon commented at 8:54 am on August 30, 2014: contributor

It needed rebase because #4180 was merged so I just changed unsigned int to unit32_t too.

jtimon force-pushed on Sep 1, 2014

jtimon force-pushed on Sep 2, 2014

jtimon force-pushed on Sep 13, 2014

jtimon commented at 7:23 pm on September 13, 2014: contributor

Rebased. Since timedata was moved from core to server, I needed to decouple pow from timedata to be able to move pow from server to core.

sipa commented at 2:39 am on September 16, 2014: member

Needs rebase.

Also, can you replace the error messages with the function name in them with __func__?

jtimon force-pushed on Sep 16, 2014

jtimon commented at 6:23 pm on September 16, 2014: contributor

Done, should I squash the __func__ commit somewhere?

laanwj commented at 9:36 am on September 25, 2014: member

Done, should I squash the func commit somewhere?

It’s fine as a separate one.

jtimon force-pushed on Sep 29, 2014

jtimon commented at 10:22 pm on September 29, 2014: contributor

Should I divide this further to make it easier to review?

BitcoinPullTester commented at 10:35 pm on September 29, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4506_e67e9f7d7651405f45e11df8416eaf9fe2101cff/ 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 force-pushed on Oct 6, 2014

jtimon commented at 4:02 pm on October 6, 2014: contributor

Rebased

gmaxwell commented at 0:20 am on October 7, 2014: contributor

I like the changes, I did have a thought that maybe CProof is at slight risk of ambiguity (E.g. SPV proof vs block proof).

sipa commented at 2:08 am on October 9, 2014: member

I think CProof should be split in its core data structure (which can go in core, or be included by it), and validation functions that take a CProof and perform the various checks on them (which can be in server). Not doing that will for example result in libscript contain block PoW validation…

jtimon commented at 3:29 am on October 15, 2014: contributor

Having the class in core with only the serialization and using it with the functions is incompatible with making the fields private/protected, which is the final goal of this series of PRs. After thinking about this a lot there’s doesn’t seem to be any solution compatible with both goals that isn’t ugly one way or another. I guess I should just keep moving functions here first and leave the class for later when we have a solution that doesn’t make anyone unhappy.

jtimon commented at 3:35 am on October 15, 2014: contributor

“Not doing that will for example result in libscript contain block PoW validation” Not necessarily, only if you want to put Transaction and BlockHeader in the same module, script doesn’t need BlockHeader’s at all. There could be a xxx.o with only COutPoint, CTxIn, CTxOut and CTransaction, which is everything script/interpreter needs.

sipa commented at 3:38 am on October 15, 2014: member

I’m fine with splitting core into a transaction and a block part, and that would indeed suffice for now, but it would still mean that anything that uses block headers will depend on its validation code to. I don’t like it, but I can’t immediately come up with an example of something that would use headers parsing, but not need the ability to verify proofs.

jtimon force-pushed on Oct 21, 2014

jtimon commented at 7:37 pm on October 21, 2014: contributor

I reduced the scope of the PR again. Now it only creates a minimal CProof class independent from pow.o. It’s built on top of #5100 and #4793.

jtimon force-pushed on Oct 21, 2014

jtimon force-pushed on Oct 22, 2014

jtimon force-pushed on Oct 23, 2014

jtimon force-pushed on Oct 27, 2014

jtimon commented at 3:43 pm on October 27, 2014: contributor

Rebased on top of the latest #5100 and without depending on #4793 (if that get merged first I can update this with the older version).

Minimal CProof class independent from pow.o 5b90a9aabd

CheckProofOfWork(nBits) -> CheckProof(CProof) dd1e043652

Use __func__ in error messages e544670810

pow.o depends on chain.o, but not main.o 89afbe0d0c

Implement CProof::ToString() aae3a70da7

Implement GetChallengeDouble(CProof) (Move 2 direct accesses to

proof.nBits to pow)

fd0799b484

Hide GetNextWorkRequired() 2aa7670ec6

GetProofIncrement(nBits) -> GetProofIncrement(CProof) e3d65ccee9

MOVEONLY: Move void UpdateTime() from pow.o to miner.o 552a17f69b

jtimon force-pushed on Oct 28, 2014

jtimon commented at 1:34 pm on October 28, 2014: contributor

Rebased to make it more readable.

jtimon commented at 8:26 pm on October 29, 2014: contributor

Closing in favor of #5171, without a class.

jtimon closed this on Oct 29, 2014

MarcoFalke locked this on Sep 8, 2021

Create Proof(OfWork) class #4506