Consensus: Separate most consensus functions to consensus.cpp #6672
Conversation
07427df to
f99a0fd
Compare
|
Fixed @luke-jr 's nits. |
|
Tested/Reviewed ACK. |
|
Some people are wondering why I made a particular change. See my answer in #6009 (comment) |
|
Greg was here. |
|
lightly reviewed ACK. passes pruning.py. |
| int64_t* pend = &pmedian[MEDIAN_TIME_SPAN]; | ||
|
|
||
| for (unsigned int i = 0; i < MEDIAN_TIME_SPAN && pindex; i++, pindex = pindex->pprev) | ||
| *(--pbegin) = (int64_t)pindex->nTime; |
There was a problem hiding this comment.
As stated prior, this has changed, but ACK.
There was a problem hiding this comment.
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.
…dent function ...and CBlockIndex::nMedianTimeSpan into Consensus::Params::nMedianTimeSpan
f99a0fd to
8d60d87
Compare
|
utACK |
|
Looking good now. |
| * Context-independent CTransaction validity checks | ||
| */ | ||
| bool CheckTx(const CTransaction& tx, CValidationState& state, const Params& consensusParams); | ||
| /** |
There was a problem hiding this comment.
Small nit, the docblocks need a blank link after each declaration (throughout this file).
There was a problem hiding this comment.
Why? The documentation seems enough of a separation to me.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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
|
ACK, not yet verified completely move-only |
|
Confirmed move-only 35f8ebd80614dd1617b40ee221070871241ff551, apart from a set -> std::set. |
|
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. |
|
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. |
|
In general there is the goal to move consensus state and code to a specific, separate lib. I am hoping that
Added - I read every code change in every pull request that comes into github/bitcoin/bitcoin with three exceptions:
|
|
@btcdrak I could have included @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. |
8d60d87 to
321becc
Compare
|
I updated the initial description and the last commit (I forgot to remove |
|
@jtimon bah, you rebased. You could always squash later if your just fixing small things? |
|
utACK 321becc |
|
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. |
321becc to
23e8e95
Compare
|
Needed rebase. |
|
Needs comprehensive plan discussed on bitcoin-dev. |
|
@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. |
…tion 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.
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.
…sus.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
…tions: -CheckBlockHeader -ContextualCheckBlockHeader -CheckBlock -ContextualCheckBlock Also add nTime parameter to CheckBlockHeader and CheckBlock. Also use the oportunity to rename the functions inside the Consensus namespace.
23e8e95 to
88a3554
Compare
|
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. |
|
Sorry for insisting, but I would really appreciate some re-utACKs (or re-"I have been here") on #6625 before squashing it. |
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:
Params(), but still a global). I hope I don't need to explain here why removing globals is a desirable thing...