Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consensus: Adapt declarations of most obviously consensus functions #6591

Closed
wants to merge 6 commits into from

Conversation

jtimon
Copy link
Contributor

@jtimon jtimon commented Aug 26, 2015

Before the first step #5696 (after creating an initial libconsensus and without counting checkpoints and Consensus::Params changes) gets negated (see #6526), here's a little second step towards completing libconsensus. This is also part of #5970 process of explicitly passing chainparams around, and in that sense it replaces #6024.

The function declarations are adapted and moved to consensus/consensus.h
Since we're touching all declarations, it is diff-wise free to put them in the Consensus namespace at the same time. It also cherry picks one commit from #6526 (modified to rename CheckTransaction to Consensus::CheckTx [again, that rename is diff-wise free]).

The function definitions are left in main.cpp for now, but some of them will have to be moved to consensus/consensus.cpp before exposing anything else in libconsensus (my preference would be to expose VerifyHeader next, but I haven't got much feedback in that reward).

Blocked by:

@dcousens
Copy link
Contributor

concept ACK

@jonasschnelli
Copy link
Contributor

Concept ACK.
Not sure if GetAdjustedTime() as param is required. After this PR there would be 5 more calls to GetAdjustedTime() (which is maybe not very time critical but at least it locks cs_nTimeOffset).

@jtimon
Copy link
Contributor Author

jtimon commented Aug 29, 2015

@jonasschnelli GetAdjustedTime() is not welcomed in libconsensus. Maybe @theuni can give more details about why. A vague explanation would be that libconsensus has to be stateless.
In main.cpp (full of global variables anyway) it's just fine, but these functions will be eventually moved to consensus.cpp to be built independently for libconsensus (or at least that's my plan).

@jtimon
Copy link
Contributor Author

jtimon commented Sep 4, 2015

Rebased on top of modified #6625 and force-pushed.

theuni and others added 6 commits September 6, 2015 06:35
…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. For now, everything passes MAX_BLOCK_SIZE, so the
result matches what it would've before.

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.
@jtimon
Copy link
Contributor Author

jtimon commented Sep 14, 2015

Closing in favor of #6672.

@jtimon jtimon closed this Sep 14, 2015
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants