Continue proof of work encapsulation #5171

1. 
   jtimon commented at 8:21 pm on October 29, 2014: contributor

   It is built on top of #5170, replaces #4506 and continues #4377.
2. in src/pow.h: 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?
3. in src/pow.h: 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
4. 
   sipa commented at 6:56 pm on November 3, 2014: member

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

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

   ping
7. jtimon force-pushed on Nov 21, 2014
8. 
   jtimon commented at 2:56 pm on November 21, 2014: contributor

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

    Needed rebase.
17. laanwj added the label Improvement on Dec 5, 2014
18. 
    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:

    0bool CheckChallenge(const CBlockHeader& block, const CBlockIndex& indexLast);
    1void ResetChallenge(CBlockHeader& block, const CBlockIndex& indexLast);
    2
    3/** Avoid using these functions when possible */
    4double GetChallengeDifficulty(const CBlockIndex* blockindex);
    5std::string GetChallengeStr(const CBlockIndex& block);
    6std::string GetChallengeStrHex(const CBlockIndex& block);
    7uint32_t GetNonce(const CBlockHeader& block);
    8void 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).

19. 
    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.

20. 
    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.
21. 
    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
22. jtimon closed this on Jan 7, 2015

    ---

23. MarcoFalke locked this on Sep 8, 2021

Contributors
jtimon sipa jaromil petertodd

Labels
Refactoring