MOVEONLY: Move consensus functions out of main #7310

1. 
   jtimon commented at 4:45 pm on January 7, 2016: contributor

   Based on #7287 to avoid consensus/consensus.cpp having to depend on FormatStateMessage() (static function in main.cpp) temporarily. This moves most of the remaining consensus-critical code in main.cpp to the consensus directory.

   Even if I haven’t published the promised document explaining the libconsensus longer term plan, all that is needed to review this is being able to tell whether all the moved functions are consensus critical or not; and then make sure that the code is only being moved without changes. As other times, I have introduced trivial non-functional changes that those who check “moveonlyness” should notice.

2. 
   dcousens commented at 1:16 am on January 8, 2016: contributor

   concept ACK, will review later
3. jonasschnelli added the label Refactoring on Jan 8, 2016
4. 
   dcousens commented at 2:24 pm on January 28, 2016: contributor

   utACK df57ab7
5. jtimon force-pushed on Feb 1, 2016
6. 
   jtimon commented at 7:11 pm on February 1, 2016: contributor

   Rebased after #7287 has been merged.
7. 
   jtimon commented at 8:59 pm on February 1, 2016: contributor

   Also, in case it is useful to anyone, I have backported this with a few other merged consensus changes on top of 3cd836c (last-0.11.99) in https://github.com/bitcoin/bitcoin/commit/9877eb520bee42a3d6241b9268f29dce314052ea (on top of https://github.com/jtimon/bitcoin/commits/backports-0.12 ).
8. 
   dcousens commented at 11:58 pm on February 1, 2016: contributor

   re-ACK 1c31e65 MOVE-ONLY (bar 1 comment)
9. 
   laanwj commented at 3:13 pm on February 2, 2016: member

   So for TL; DR this moves the following functions to consensus:

   • IsSuperMajority
   • GetBlockSubsidy
   • IsFinalTx
   • GetLegacySigOpCount
   • GetP2SHSigOpCount
   • CheckTransaction
   • Consensus::CheckTxInputs
   • CheckBlockHeader
   • CheckBlock
   • ContextualCheckBlockHeader
   • ContextualCheckBlock

   needed to review this is being able to tell whether all the moved functions are consensus critical or not

   AFAIK, all these functions are consensus critical. I have not checked move-onlyness.

10. jtimon force-pushed on Feb 14, 2016
11. 
    jtimon commented at 2:07 am on February 14, 2016: contributor

    EDIT: Rebased after 0.12 fork (see the updated enumeration in https://github.com/jtimon/bitcoin/tree/backports-0.12):

    Rebase (1): Report non-mandatory script failures correctly #7276 Rebase (2): Mark blocks with too many sigops as failed #7217 Rebase (3): Consensus: Remove calls to error() and FormatStateMessage() from some consensus code in main #7287 Rebase (4): MOVEONLY: non-consensus: from pow to chain: #7311 Rebase (5): Consensus build package #7091 6) Improve block validity/ConnectBlock() comments #7444 (don’t confuse with 2f19905 petertodd “Improve block validity/ConnectBlock() comments”) 7): tests: Remove May15 test #7490 8) Implement SequenceLocks functions for BIP 68 #7184

12. jtimon force-pushed on Feb 14, 2016
13. 
    jtimon commented at 2:49 am on February 14, 2016: contributor

    Rebased(8) [after #7184]
14. in src/test/transaction_tests.cpp: in d0832f0b77 outdated

    314@@ -314,7 +315,6 @@ BOOST_AUTO_TEST_CASE(test_Get)
    315 
    316 BOOST_AUTO_TEST_CASE(test_IsStandard)
    317 {
    318-    LOCK(cs_main);
    

    ---

    ---

    

    btcdrak commented at 10:41 am on February 14, 2016:

    Shouldn’t be part of a move only PR surely?

    ---

    

    jtimon commented at 1:28 pm on February 14, 2016:

    In an older version I believe that allowed me to remove the main include, but since that’s not the case anymore I guess I should leave this out.
15. 
    btcdrak commented at 10:44 am on February 14, 2016: contributor

    Concept ACK.
16. 
    NicolasDorier commented at 10:58 am on February 14, 2016: contributor

    I always see the may15 tests
17. 
    laanwj commented at 12:37 pm on February 15, 2016: member

    I always see the may15 tests

    Good catch. They were removed in #7490, and appear to be readded here.

18. jtimon force-pushed on Feb 15, 2016
19. 
    jtimon commented at 6:27 pm on February 15, 2016: contributor

    Oops, updated without nits. By the way, after #7184 I these two functions are moved as well:

    0std::pair<int, int64_t> CalculateSequenceLocks(const CTransaction &tx, int flags, std::vector<int>* prevHeights, const CBlockIndex& block);
    1bool EvaluateSequenceLocks(const CBlockIndex& block, std::pair<int, int64_t> lockPair);
    2bool SequenceLocks(const CTransaction &tx, int flags, std::vector<int>* prevHeights, const CBlockIndex& block)
    

20. jtimon force-pushed on Feb 15, 2016
21. jtimon force-pushed on Feb 15, 2016
22. 
    jtimon commented at 3:10 pm on February 19, 2016: contributor

    Ping
23. 
    morcos commented at 9:41 pm on February 19, 2016: member

    utACK 3a3cd39
24. MOVEONLY: Libconsensus: Move consensus functions away from main.cpp 9001c6050b
25. in src/consensus/consensus.h: in 3a3cd39dad outdated

    33@@ -22,4 +34,94 @@ enum {
    34     LOCKTIME_MEDIAN_TIME_PAST = (1 << 1),
    35 };
    36 
    37+/**
    38+ * Context-independent CTransaction validity checks.
    39+ * Nobody should spend an extra cycle on a transaction that doesn't pass this.
    

    ---

    ---

    

    MarcoFalke commented at 4:18 pm on February 25, 2016:

    I fail to see how this additional comment is move only. You may as well just keep it the way it was, as this comment is not adding much information.

    Also two lines below this you are changing the white space. CValidationState &state.

    Still, Concept ACK

    ---

    

    jtimon commented at 7:09 pm on February 25, 2016:

    There’s some other small documentation additions in consensus/consensus.h but they can all be postponed. I’m happy to remove this extra doc line or any other as nit.

    Regarding the withe space, the goal was to only change that kind of little style thing if it’s to comply more with our style rules, but here I’m actually doing the opposite (https://github.com/bitcoin/bitcoin/blob/master/src/.clang-format#L41), so thank you for noticing on time. Probably lost in rebase at some point, I will change this.

    ---

    

    MarcoFalke commented at 11:21 am on March 3, 2016:

    @jtimon I am happy to review again, if you stick to what is mentioned in the commit message… I mean, it is great that you are adding documentation and fixing white space but it is a lot harder to verify MOVEONLY if there are random changes in between.

    There is no rule that says you can only use one commit per pull request. Each goal which can be achieved in a separate commit, should indeed go into a separate atomic commit. (This is not meant specifically for you but also for others, including me.)

26. jtimon force-pushed on Feb 25, 2016
27. 
    jtimon commented at 10:51 am on March 3, 2016: contributor

    @laanwj as predicted, people are afraid of doing this now to avoid “complicating backports” I keep thinking that just after branching is maybe the worse possible time for refactors. Closing for lack of interest (can reopen when there’s more interest).
28. jtimon closed this on Mar 3, 2016

    ---

29. 
    laanwj commented at 11:01 am on March 3, 2016: member

    @jtimon Yes sorry for that. On the other hand there’s always some reason to delay stuff like this, at some point we have to just do it, “complicate backports” or not.
30. 
    jtimon commented at 11:05 am on March 3, 2016: contributor

    Agreed, but apparently most potential reviewers disagree.
31. DrahtBot locked this on Sep 8, 2021

Contributors
jtimon dcousens laanwj btcdrak NicolasDorier morcos MarcoFalke

Labels
Refactoring