Continue proof of work encapsulation #5171

pull jtimon wants to merge 6 commits into bitcoin:master from jtimon:pow4 changing 7 files +72 −32

jtimon commented at 8:21 PM on October 29, 2014: contributor

It is built on top of #5170, replaces #4506 and continues #4377.
jtimon cross-referenced this on Oct 29, 2014 from issue Create Proof(OfWork) class by jtimon

in src/pow.h:None in a91c09e045 outdated

  23 | -uint256 GetProofIncrement(unsigned int nBits);
  24 | +uint256 GetBlockProof(const CBlockIndex& block);
  25 | +bool CheckChallenge(const CBlockHeader& block, const CBlockIndex& indexLast);
  26 | +void ResetChallenge(CBlockHeader& block, const CBlockIndex& indexLast);
  27 | +
  28 | +/** Avoid using this functions when possible */

sipa commented at 6:41 PM on November 3, 2014:

Nit: these

jtimon commented at 7:05 PM on November 3, 2014:

Can you explain the nit a little bit more?

in src/pow.h:None in a91c09e045 outdated

  24 | +uint256 GetBlockProof(const CBlockIndex& block);
  25 | +bool CheckChallenge(const CBlockHeader& block, const CBlockIndex& indexLast);
  26 | +void ResetChallenge(CBlockHeader& block, const CBlockIndex& indexLast);
  27 | +
  28 | +/** Avoid using this functions when possible */
  29 | +double GetChallengeDouble(const CBlockIndex* blockindex);

sipa commented at 6:42 PM on November 3, 2014:

Nit: call it GetChallengeDifficulty? It's a well-defined number.

jtimon commented at 7:05 PM on November 3, 2014:

Ok, GetChallengeDifficulty it is.

jtimon commented at 7:07 PM on November 3, 2014:

ops, sorry, s/this/these, got it

sipa commented at 6:56 PM on November 3, 2014: member

utACK, apart from the above nits.
jtimon commented at 7:14 PM on November 3, 2014: contributor

Added commit to correct nits (plus fix a missing include).
jtimon commented at 2:47 PM on November 14, 2014: contributor

ping
jtimon force-pushed on Nov 21, 2014
jtimon commented at 2:56 PM on November 21, 2014: contributor

Rebased for clarity after #5170 being merged. Nit fixes squashed too.
Hide GetNextWorkRequired() c3ee304478
Implement GetChallengeDouble(CBlockIndex) (Move 2 direct accesses to proof.nBits to pow) 7067ed1c52
GetChallengeStr(CBlockIndex) 8732d1c213
GetChallengeStrHex(CBlockIndex) 678ef3046d
GetNonce(CBlockHeader) 2b378502dc
SetNonce(CBlockHeader) 6f72b8e366
jtimon force-pushed on Nov 26, 2014
jtimon commented at 11:37 AM on November 26, 2014: contributor

Needed rebase.
laanwj added the label Improvement on Dec 5, 2014

jaromil commented at 1:02 PM on December 15, 2014: contributor

This definitely makes the code more readable, it rebases on 0.10 without problems and its the sort of change that helps to isolate and track pow related operations. It adds the following functions to pow.h:

bool CheckChallenge(const CBlockHeader& block, const CBlockIndex& indexLast);
void ResetChallenge(CBlockHeader& block, const CBlockIndex& indexLast);

/** Avoid using these functions when possible */
double GetChallengeDifficulty(const CBlockIndex* blockindex);
std::string GetChallengeStr(const CBlockIndex& block);
std::string GetChallengeStrHex(const CBlockIndex& block);
uint32_t GetNonce(const CBlockHeader& block);
void SetNonce(CBlockHeader& block, uint32_t nNonce);

It may be desirable to add to the comment: brief reason why to avoid those functions and a mention where they are currently called (rpcblockchain.cpp and rpcmining.cpp).

petertodd commented at 1:28 PM on December 15, 2014: contributor

NACK

"more readable" is not a good justification to be messing around with this code. If anything, I'd argue it makes the code less readable as what it's actually doing has been obscured by more layers of abstraction.
jaromil commented at 2:51 PM on December 15, 2014: contributor

Well, it introduces the Challenge term on check/reset block-index operations which seems more readable to me compared to the if checks. It also does so keeping block as first argument for all new functions, consistent with other existing calls like CheckBlockHeader. At last it introduces get/set function wrappers to operations accessing the nonce. I think this is an improvement and should be merged. I'm not sure what do you mean by abstraction: that has a particular meaning in C++ and I see no real abstraction introduced and even on the linguistic level the function names are good explanations for what they do.
jtimon commented at 8:23 PM on January 7, 2015: contributor

Closing for now, at least until #5599 has been merged (if it gets merged) and I can propose something more complete. For those interested, this is the patch I intend to maintain permanently (to make sure nNonce and nBits are not used directly out of pow.o): https://github.com/jtimon/bitcoin/tree/noproof
jtimon closed this on Jan 7, 2015
jtimon cross-referenced this on Mar 11, 2015 from issue Consensus: Refactor: Decouple pow from chainparams by jtimon
bitcoin locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#4377 Proof of work related refactor #4506 Create Proof(OfWork) class #5170 Correct mistakes in #4377 and #4796 #5599 RPC: Get rid of the internal miner's hashmeter #5812 Consensus: Refactor: Decouple pow from chainparams