Consensus: Separate most consensus functions to consensus.cpp

jtimon commented at 7:06 pm on September 14, 2015: contributor

This replaces/includes #6625 (previously #6526), #6009, #6051 and #6591 (previously #6024).

EDIT (further explanation demanded):

Although the PRs linked from here can provide a better explanation of the motivation, it is not so convenient for reviewers since they have to go through them individually.

The first 4 commits are taken from #6625, which is a modification of #6526 that doesn’t break some other things. The modifications are explained in #6625. Although the #6526 has an extensive description, I will summarize its general purpose here. The goal is to prepare the consensus code for an eventual block size hardfork and also for previous testing. It should simplify other PRs like #6382 (that introduces std::numeric_limits<uint64_t>::max() testnet chains to test different block sizes), #6451 (BIP102) or any other block-size-related proposal.

The next commit is taken from #6009, which separates GetMedianTimePast() from CBlockIndex and moves it to consensus (it’s done in the same commit instead of the next one because it’s “cheaper diff-wise”). GetMedianTimePast() is needed for verifying block headers, but chain.o will not.

The next commit is the code movement. All consensus critical functions that are not separated already (like 4 of the 5 functions in pow.o [the other one can be separated out]) in build-separable files are moved. If you think any of the moved functions are not consensus critical or that there’s some missing functions, please, say so. One exception is main::ConnectBlock() which is consensus critical but contains other things that are not welcomed in libconsensus. A VerifyBlock library-safe function can be extracted from it later. #6051 was a similar movement.

The last commit is taken from #6591 (previously #6024). This one has 2 purposes:

Remove chainparams.h and timedata.h dependencies (which are not library-friendly) from consensus.o
It’s part of a bigger process (see #5970 ) to eventually be able to remove the pCurrentParams global (hidden behind Params(), but still a global). I hope I don’t need to explain here why removing globals is a desirable thing…

jtimon force-pushed on Sep 14, 2015

jtimon commented at 7:50 pm on September 14, 2015: contributor

Fixed @luke-jr ’s nits.

jonasschnelli commented at 8:31 pm on September 14, 2015: contributor

Tested/Reviewed ACK. A solution for FormatStateMessage() can be found later.

jtimon commented at 9:05 pm on September 14, 2015: contributor

Some people are wondering why I made a particular change. See my answer in #6009#commitcomment-13204480

gmaxwell commented at 9:08 pm on September 14, 2015: contributor

Greg was here.

morcos commented at 9:47 pm on September 14, 2015: member

lightly reviewed ACK. passes pruning.py.

in src/consensus/consensus.cpp: in f99a0fd79e outdated

156+    int64_t pmedian[MEDIAN_TIME_SPAN];
157+    int64_t* pbegin = &pmedian[MEDIAN_TIME_SPAN];
158+    int64_t* pend = &pmedian[MEDIAN_TIME_SPAN];
159+
160+    for (unsigned int i = 0; i < MEDIAN_TIME_SPAN && pindex; i++, pindex = pindex->pprev)
161+        *(--pbegin) = (int64_t)pindex->nTime;

dcousens commented at 9:51 pm on September 14, 2015:

As stated prior, this has changed, but ACK.

theuni commented at 10:01 pm on September 14, 2015:

As discussed, I agree and I’d prefer to see this left as a function. That’ll mean addressing it again when the time comes.

jtimon referenced this in commit c85ded0a67 on Sep 14, 2015

jtimon force-pushed on Sep 14, 2015

jtimon commented at 10:41 pm on September 14, 2015: contributor

Solved @dcousens and @sipa ’s nits.

dcousens commented at 10:42 pm on September 14, 2015: contributor

utACK

btcdrak commented at 10:44 pm on September 14, 2015: contributor

Looking good now.

in src/consensus/consensus.h: in 8d60d87182 outdated

29+
30+/**
31+ * Context-independent CTransaction validity checks
32+ */
33+bool CheckTx(const CTransaction& tx, CValidationState& state, const Params& consensusParams);
34+/**

btcdrak commented at 10:46 pm on September 14, 2015:

Small nit, the docblocks need a blank link after each declaration (throughout this file).

jtimon commented at 10:51 pm on September 14, 2015:

Why? The documentation seems enough of a separation to me.

dcousens commented at 11:00 pm on September 14, 2015:

@jtimon it helps to distinguish blocks apart from each other, and is the standard adopted by most of the code base as far as I can tell.

jtimon commented at 11:09 pm on September 14, 2015:

from IRC:

< dcousens> jtimon: the doc blocks padded line is a standard used throughout hte codebas < dcousens> the codebase* < dcousens> It makes it easier to see blocks < dcousens> Rather than just 1 contiguous piece of code < jtimon> dcousens: this “standard” is not documented in the style guide and it’s not followed in many places (for example, main.h) < dcousens> jtimon: I only opened 3-4 files before I said that, maybe my sample size was too small < dcousens> even in main.h, its pretty consistently there except for the 1 liners < dcousens> I’d say its 50/50 in that file, with a higher prevalance in other files < jtimon> we should document this if it’s part of the style (unfortunately I don’t think clang-format can enforce it) < dcousens> jtimon: maybe just be consistent with what is already in the file < dcousens> jtimon: i’ve lost the PR link, but, I’ll leave that up to you < dcousens> its a nit anyway < jtimon> not opposing to it, but some people are already verifying the move-only commit and I would prefer not to modify the hash of the commit unless I need to for some other reason < jtimon> well, there’s not much in consensus/consensus.h before this PR… < dcousens> :thumbs_up:, just mention that, the inconsistencies and that at some point we should unify it < dcousens> its not relevant to your PR if its inconsistent elsewhere

theuni commented at 11:00 pm on September 14, 2015: member

ACK, not yet verified completely move-only

sipa commented at 11:19 pm on September 14, 2015: member

Confirmed move-only 35f8ebd80614dd1617b40ee221070871241ff551, apart from a set -> std::set.

jgarzik commented at 2:00 am on September 15, 2015: contributor

General comment:

Is there a link to any sort of written or spoken plan for these sort of commits? Was that covered at a conference roundtable?

To someone not watching this specific subset of libconsensus refactoring PRs, the 10,000 foot view is that there is a rather random stream of refactors that proceed in fits and starts without apparent plan or end other than a one sentence “isolate consensus state and code” summary.

dcousens commented at 2:38 am on September 15, 2015: contributor

Agreed with @jgarzik, these PRs do appear out of nowhere and often have little to no description. But there does seem to be some immediate consensus as though some discussion has occurred?

Don’t get me wrong, having reviewed each of the above PR’s, I am in favour of the refactor, but, it appeared to be random each time as to why they keep getting closed/re-opened and even re-authored without much of a comment.

jgarzik commented at 3:19 am on September 15, 2015: contributor

In general there is the goal to move consensus state and code to a specific, separate lib.

I am hoping that

There is some plan
We will not see a five year stream of random consensus code movement patches causing lots of downstream developer headaches.

Added - I read every code change in every pull request that comes into github/bitcoin/bitcoin with three exceptions:

consensus code movement changes - too big, too chaotic, too frequent, too unfocused
some non-code changes (docs)
ignore 80% of the Qt changes

jtimon commented at 1:03 pm on September 15, 2015: contributor

@btcdrak I could have included using namespace std to avoid that little change, but I actually like to sneak at least one little change in move-only commits to make sure somebody actually verifies the move-only (now I know that sipa and you have done so). @jgarzik @dcousens I’m sorry for not providing a bigger description, but I thought that reading the PRs that this includes/replaces would be enough. I will update the description and answer to the off-topic complains in the mailing list.

jtimon force-pushed on Sep 15, 2015

jtimon commented at 5:20 pm on September 15, 2015: contributor

I updated the initial description and the last commit (I forgot to remove #include "timedata.h").

dcousens commented at 6:55 am on September 16, 2015: contributor

@jtimon bah, you rebased. You could always squash later if your just fixing small things? It would just make re-ACKing the PR easier.

dcousens commented at 6:56 am on September 16, 2015: contributor

utACK 321becc

jtimon commented at 8:43 pm on September 16, 2015: contributor

I made it separate, but it was a one line change to the last commit so I just squashed it directly. Previous commits (particularly the move-only one) are unaffected.

jtimon force-pushed on Sep 22, 2015

jtimon commented at 5:34 pm on September 22, 2015: contributor

Needed rebase.

jgarzik commented at 10:55 pm on September 22, 2015: contributor

Needs comprehensive plan discussed on bitcoin-dev.

jtimon commented at 5:43 pm on September 23, 2015: contributor

@jgarzik there have been multiple discussions in the mailing list about this but you have chosen to ignore all of them (probably because you have limited time and this hasn’t been a priority for you). In addition to that, I’ve been persistently asking for review on freenode’s #bitcoin-dev People like @laanwj , @theuni , @sipa , @jonasschnelli and @morcos know this very well. If you want to be added to my “to ask for libconsensus-related review” list I am more than happy to do so. You would never miss one of my libconsensus-related PRs again. If not, you can still any concrete questions you may have about them (unfortunately you don’t have any concrete questions for this one, despite how confusing and “random” it looks to you).

I won’t create yet another ml thread that I myself have a hard time searching for when periodically someone asks about “the big picture” or “a detailed plan” for libconsensus-related work. This time I have created an issue that I can edit on github instead: #6714

If you have any questions about this PR, please, ask them here. If you have more general questions about “a plan” for libconsensus, please ask them in the issue’s thread. I’ve been trying hard to effectively communicate about this work for more than a year and I believe I have miserably failed, not just communicating it to you. You can help me with your apparently many doubts if you turn them into concrete questions.

laanwj added the label Consensus on Sep 23, 2015

jtimon commented at 8:14 pm on September 24, 2015: contributor

@jgarzik Please see #6714 (as discussed in IRC and the mailing list).

EDIT: also, it needed rebase again.

consensus: don't define MAX_STANDARD_TX_SIGOPS in terms of block size a70c1e28e0

consensus: teach ExtractMatches to check for an arbitrary max transaction number

This is a no-op change. For now, everything passes MAX_BLOCK_SIZE / 60, so the
result matches what it would've before.

Tests use a number equal to the number of transactions where necessary,
to ensure that they're never rejected when blocksizesize isn't being tested.

8df32a9ca4

consensus: teach CheckTransaction to check for an arbitrary max tx size

This is a no-op change.

Tests use a value of std::numeric_limits<uint64_t>::max() where necessary, to ensure that they're never
rejected when size isn't being tested.

e61fd27115

consensus: Move consensus constants into Consensus::Params and consensus.h (as functions)

The following are now tied to a chain rather than being defined as global
constants. Their values have not changed.

nMinTxSize
nMaxBlockSize
nMaxTxSize
nMaxBlockSigops
nCoinbaseMaturity

Also, for free (diff-wise):

Blocksize: Turn MAX_BLOCK_SIZE (nMaxBlockSize) and MAX_BLOCK_SIGOPS (nMaxBlockSigops) into functions

...which take Consensus::Params as parameter
This will be convenient to reduce the diff of any proposal that changes the blocksize as a hardfork

7cca35a60d

Consensus: Refactor: Turn CBlockIndex::GetMedianTimePast into independent function 1f22ddcfff

Consensus: MOVEONLY: Move a lot of consensus functions from main.cpp to consensus.cpp 03e04b9bda

Consensus: Chainparams: Explicit Consensus::Params for consensus functions:

-CheckBlockHeader
-ContextualCheckBlockHeader
-CheckBlock
-ContextualCheckBlock

Also add nTime parameter to CheckBlockHeader and CheckBlock.
Also use the oportunity to rename the functions inside the Consensus namespace.

88a3554851

jtimon force-pushed on Sep 24, 2015

jtimon commented at 4:05 pm on October 20, 2015: contributor

Needs rebase, but since more documentation was demanded to continue with the libconsensus work, I will rebase and reopen #6625 instead, which is more related to testing and simplifying future potential blocksize-related changes than it is to the completion of libconsensus. It also contains 4 of the 7 commits contained in this PR, but doesn’t have any code movement refactor.

To all the people who reviewed this PR, please re-review the rebased #6625 so that the review of this PR hasn’t been completely in vain.

Closing this.

jtimon closed this on Oct 20, 2015

jtimon commented at 2:52 pm on November 11, 2015: contributor

Sorry for insisting, but I would really appreciate some re-utACKs (or re-“I have been here”) on #6625 before squashing it. What’s the motivation for #6625 if we’re leaving the libconsensus movements for after 0.12 ? Apart from preventing any maxblocksize changes from getting in the way of libconsensus in the future and making any maxblocksize prototype branch easier to read, both #5970 and #6382 depend on #6625 and would become much easier to review and maintain if #6625 was merged.

DrahtBot locked this on Sep 8, 2021

Consensus: Separate most consensus functions to consensus.cpp #6672