MOVEONLY: Move consensus functions out of main #7310

jtimon · 2016-01-07T16:45:58Z

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.

dcousens · 2016-01-08T01:16:36Z

concept ACK, will review later

dcousens · 2016-01-28T14:24:43Z

utACK df57ab7

jtimon · 2016-02-01T19:11:58Z

Rebased after #7287 has been merged.

jtimon · 2016-02-01T20:59:28Z

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 9877eb5 (on top of https://github.com/jtimon/bitcoin/commits/backports-0.12 ).

dcousens · 2016-02-01T23:58:20Z

re-ACK 1c31e65 MOVE-ONLY (bar 1 comment)

laanwj · 2016-02-02T15:13:48Z

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.

jtimon · 2016-02-14T02:07:48Z

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

jtimon · 2016-02-14T02:49:29Z

Rebased(8) [after #7184]

btcdrak · 2016-02-14T10:41:09Z

src/test/transaction_tests.cpp

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

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.

btcdrak · 2016-02-14T10:44:56Z

Concept ACK.

NicolasDorier · 2016-02-14T10:58:10Z

I always see the may15 tests

laanwj · 2016-02-15T12:37:08Z

I always see the may15 tests

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

jtimon · 2016-02-15T18:27:15Z

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

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

jtimon · 2016-02-19T15:10:15Z

Ping

morcos · 2016-02-19T21:41:43Z

utACK 3a3cd39

maflcko · 2016-02-25T16:18:36Z

src/consensus/consensus.h

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

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.

@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.)

jtimon · 2016-03-03T10:51:17Z

@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).

laanwj · 2016-03-03T11:01:46Z

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

jtimon · 2016-03-03T11:05:51Z

Agreed, but apparently most potential reviewers disagree.

jonasschnelli added the Refactoring label Jan 8, 2016

jtimon mentioned this pull request Jan 29, 2016

Consensus: Remove calls to error() and FormatStateMessage() #7287

Merged

jtimon force-pushed the consensus-moveonly-0.13.99 branch from df57ab7 to 1c31e65 Compare February 1, 2016 19:11

jtimon force-pushed the consensus-moveonly-0.13.99 branch from 1c31e65 to 7c7cca2 Compare February 14, 2016 01:37

jtimon force-pushed the consensus-moveonly-0.13.99 branch from 7c7cca2 to d0832f0 Compare February 14, 2016 02:46

btcdrak reviewed Feb 14, 2016
View reviewed changes

jtimon force-pushed the consensus-moveonly-0.13.99 branch from d0832f0 to 8f67d21 Compare February 15, 2016 18:24

jtimon force-pushed the consensus-moveonly-0.13.99 branch 2 times, most recently from 72f84d3 to 3a3cd39 Compare February 15, 2016 18:37

jtimon mentioned this pull request Feb 20, 2016

libconsensus-p2a: Decouple pow.o from chain.o and move it to the consensus package #7563

Closed

maflcko reviewed Feb 25, 2016
View reviewed changes

MOVEONLY: Libconsensus: Move consensus functions away from main.cpp

9001c60

jtimon force-pushed the consensus-moveonly-0.13.99 branch from 3a3cd39 to 9001c60 Compare February 25, 2016 19:19

jtimon closed this Mar 3, 2016

jtimon mentioned this pull request Jul 11, 2016

Consensus: MOVEONLY: Move functions for tx verification #8329

Merged

jtimon mentioned this pull request Aug 31, 2016

Consensus: MOVEONLY: Move functions for header verification #8337

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

MOVEONLY: Move consensus functions out of main #7310

MOVEONLY: Move consensus functions out of main #7310

Uh oh!

Conversation

jtimon commented Jan 7, 2016

Uh oh!

dcousens commented Jan 8, 2016

Uh oh!

dcousens commented Jan 28, 2016

Uh oh!

jtimon commented Feb 1, 2016

Uh oh!

jtimon commented Feb 1, 2016

Uh oh!

dcousens commented Feb 1, 2016

Uh oh!

laanwj commented Feb 2, 2016

Uh oh!

jtimon commented Feb 14, 2016

Uh oh!

jtimon commented Feb 14, 2016

Uh oh!

btcdrak Feb 14, 2016

Choose a reason for hiding this comment

Uh oh!

jtimon Feb 14, 2016

Choose a reason for hiding this comment

Uh oh!

btcdrak commented Feb 14, 2016

Uh oh!

NicolasDorier commented Feb 14, 2016

Uh oh!

laanwj commented Feb 15, 2016

Uh oh!

jtimon commented Feb 15, 2016

Uh oh!

jtimon commented Feb 19, 2016

Uh oh!

morcos commented Feb 19, 2016

Uh oh!

maflcko Feb 25, 2016

Choose a reason for hiding this comment

Uh oh!

jtimon Feb 25, 2016

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 3, 2016

Choose a reason for hiding this comment

Uh oh!

jtimon commented Mar 3, 2016

Uh oh!

laanwj commented Mar 3, 2016

Uh oh!

jtimon commented Mar 3, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants