Skip to content

Conversation

jtimon
Copy link
Contributor

@jtimon jtimon commented Apr 22, 2015

Dependencies:

I'm sorry for killing commit ids and creating new PRs so easily around this, but I really needed to explore different ways serializing/parallelizing this. And my conlusion is that everything gets more readable if we do the moveonly first, it is specially important for libconsensus API proposals and how could they potentially change bitcoin core's storage more readable.
Since Script is already exposed, I think the remaining types to be verified by libconsensus are: Transaction, BlockHeader and Block.

For each one of them we will want to expose a full verification function. Additionally, we may want to expose checks in phases, for example, in the way that is already separated in bitcoin core, mostly to prevent DoS attacks and to put policy checks in between (ie: context-less checks before storage-dependent checks, expensive script validation the last thing, etc). I don't think the intermediary checks will generate a lot of discussion, since if we can reach the full validation version for each of the types with proper C APIs, exposing any subset of the checks will probably be easy. So I will focus on the full validation functions for now and calling them VerifyTx, VerifyHeader and VerifyBlock respectively.

But this is not moving everything necessary for libconsensus. Before it is possible for libconsensus to expose full block verification, it has to be able to verify block headers and transactions independently.
So we know VerifyBlock comes after VerifyTx and VerifyHeader, but we don't know whether VerifyTx or VerifyHeader will be ready first. Because they depend on discussions about CCoinsViewCache and
CBlockIndex respectively about how to expose its storage, which may require or fit well with structural changes in how bitcoin core stores them, for example, if UTXO commitments are going to be softforked in some way, the API for VerifyTx should be forward compatible with that, which may delay its exposure.

And because we don't want to unnecessarily make libbitcoin bigger unless something more it's going to be exposed, I thought that having two cpp files could be useful, I named them txverify.cpp and blockverify.cpp but it's open to bikesheding. The point is that when one is ready you can add it to libconsensus while leaving the other in the server building module.

Once they're both ready, a last moveonly commit after this one will be required to expose a C API for full block rule validation, that new code can go in blockverify.cpp

The intention of moving all the function declarations that we know we will need (for example, main::CheckBlock) without moving the actual code to a cpp in the same commit is to minimize the number of include restructuring commits, they can all be done right after this PR.
Alternatively the block declarations can be delayed, but I believe that it's worth it to move all transaction and header declarations in the same commmit since it doesn't take that much to move it all fast (see the non-moveonly commits in this PR, they're quite trivial).

@jtimon
Copy link
Contributor Author

jtimon commented Apr 23, 2015

Closing until #5696 is resolved

@laanwj
Copy link
Member

laanwj commented Apr 30, 2015

Are you sure GetBlockProof is not part of consensus? e.g. it's used in AddToBlockIndex to determine the total amount of work on a chain.
It's the only function left in pow.h which is kind of unfortunate.

@jtimon
Copy link
Contributor Author

jtimon commented Apr 30, 2015

The highest level function I'm contemplating would be VerifyBlock and that's not necessary for it. It's certainly not necessary for VerifyHeader. That's used to chose the longest chain, so it's seems more part of the index than validation rules. Maybe in the future we can expose that too somehow, but one step at a tine...

@laanwj
Copy link
Member

laanwj commented May 1, 2015

But nChainWork (computed using GetBlockProof) is used in e.g. ActivateBestChain(Step), which choose the best chain.
@theuni what's your opinion here?

@sipa
Copy link
Member

sipa commented May 1, 2015

Depends on whether we define consensus as (conceptually) a black box that just says whether a particular chain is valid or not, or whether we include the mechanism for determining which of possible valid chains as well.

I don't really have an opinion here, apart from that I think separating these two concerns in the code makes sense - whether we expose that in libconsensus or not.

@laanwj
Copy link
Member

laanwj commented May 1, 2015

Ok fair enough. But I do think this destroys the purpose of pow.cpp/pow.h. The goal was to have the proof-of-work functions in one place. Now one function is left in that file...

@jtimon
Copy link
Contributor Author

jtimon commented May 2, 2015

well yes, my plan was to destroy pow and move the remaining function to chain.o, but zince there's more stuff being created there...

@ghost
Copy link

ghost commented May 10, 2015

Built and ran tests ACK

edit: happy to do more, but I wasn't sure where to begin testing this, aside from writing my own tests

@jtimon jtimon force-pushed the consensus_moveonly branch from 3fb125a to 008cbbc Compare May 15, 2015 14:01
@jtimon
Copy link
Contributor Author

jtimon commented May 15, 2015

Rebased

@jtimon
Copy link
Contributor Author

jtimon commented May 27, 2015

Rebased with one less dependency (2 commits less).

@jtimon jtimon force-pushed the consensus_moveonly branch from eea7358 to ea4d54f Compare June 2, 2015 11:17
@jtimon
Copy link
Contributor Author

jtimon commented Jun 2, 2015

Rebased again.
Only 2 PR dependencies to go, ping #5975 and #6061.

@jtimon jtimon force-pushed the consensus_moveonly branch from ea4d54f to bb5773d Compare June 4, 2015 19:21
@jtimon
Copy link
Contributor Author

jtimon commented Jun 4, 2015

Needed rebase

@jtimon jtimon force-pushed the consensus_moveonly branch from bb5773d to 9f24872 Compare June 10, 2015 13:07
@jtimon jtimon changed the title DEPENDENT: MOVEONLY: Move most of consensus functions (pre-block) MOVEONLY: Move most of consensus functions (pre-block) Jun 10, 2015
@jtimon jtimon force-pushed the consensus_moveonly branch from 9f24872 to 2cf7657 Compare June 10, 2015 16:18
@jtimon
Copy link
Contributor Author

jtimon commented Jun 10, 2015

Rebased. All PR dependencies merged or replaced with equivalent commits.
Now this is purely move only.

@jtimon jtimon force-pushed the consensus_moveonly branch from 2cf7657 to a809028 Compare June 12, 2015 09:15
@jtimon
Copy link
Contributor Author

jtimon commented Jun 12, 2015

Squashed the 2 moveonly commits.

@theuni
Copy link
Member

theuni commented Jun 17, 2015

Confirmed move-only with the exception of

-    set<COutPoint> vInOutPoints;
+    std::set<COutPoint> vInOutPoints;

which is obviously fine.

I didn't bother checking includes and function prototypes though.

@jtimon jtimon changed the title MOVEONLY: Move most of consensus functions (pre-block) Consensus: MOVEONLY: Move most of consensus functions (pre-block) Jun 25, 2015
@jtimon jtimon changed the title Consensus: MOVEONLY: Move most of consensus functions (pre-block) MOVEONLY: Consensus: Move most of consensus functions (pre-block) Jun 26, 2015
@jtimon jtimon force-pushed the consensus_moveonly branch from a809028 to e1e4025 Compare July 6, 2015 22:51
@jtimon
Copy link
Contributor Author

jtimon commented Jul 6, 2015

Rebased

Functions definitions are moved to 2 independent files:

To consensus/txverify.cpp:

from main.cpp:
-CheckTransaction
-Consensus::CheckTxInputs
-GetLegacySigOpCount
-GetP2SHSigOpCount
-IsFinalTx

To consensus/blockverify.cpp

from main.cpp:
-CheckBlockHeader
-ContextualCheckBlockHeader
-IsSuperMajority

from pow.cpp:
-CalculateNextWorkRequired
-CheckProofOfWork
-GetNextWorkRequired
@jtimon jtimon force-pushed the consensus_moveonly branch from e1e4025 to a45e825 Compare July 7, 2015 02:47
@jtimon
Copy link
Contributor Author

jtimon commented Jul 12, 2015

I'm having second thoughts about txverify.cpp + blockverify.cpp vs policy.cpp.
If we don't care about exposing verifyTx or verifyHeader before verifyBlock is ready, then we can just have policy.cpp and we can move more things at the same time (CheckBlock, ContextualCheckBlock and GetBlockSubsidy, for example).

Do we care about being able to expose VerifyTx without being prepared to also expose VerifyHeader and VerifyBlock?
Do we care about being able to expose VerifyHeader without being prepared to also expose VerifyTx and VerifyBlock?

Thoughts?

@jtimon
Copy link
Contributor Author

jtimon commented Jul 15, 2015

Closing for now.

@jtimon jtimon closed this Jul 15, 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.

4 participants