-
Notifications
You must be signed in to change notification settings - Fork 37.8k
MOVEONLY: Consensus: Move most of consensus functions (pre-block) #6051
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
Conversation
Closing until #5696 is resolved |
da84665
to
e97caf9
Compare
e97caf9
to
cdfa130
Compare
acd8f4c
to
88257eb
Compare
Are you sure |
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... |
But |
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. |
Ok fair enough. But I do think this destroys the purpose of |
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... |
49a107a
to
3fb125a
Compare
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 |
3fb125a
to
008cbbc
Compare
Rebased |
fc97b99
to
a2f35e2
Compare
a2f35e2
to
eea7358
Compare
Rebased with one less dependency (2 commits less). |
eea7358
to
ea4d54f
Compare
ea4d54f
to
bb5773d
Compare
Needed rebase |
bb5773d
to
9f24872
Compare
9f24872
to
2cf7657
Compare
Rebased. All PR dependencies merged or replaced with equivalent commits. |
2cf7657
to
a809028
Compare
Squashed the 2 moveonly commits. |
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. |
a809028
to
e1e4025
Compare
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
e1e4025
to
a45e825
Compare
I'm having second thoughts about txverify.cpp + blockverify.cpp vs policy.cpp. Do we care about being able to expose VerifyTx without being prepared to also expose VerifyHeader and VerifyBlock? Thoughts? |
Closing for now. |
Dependencies:
MOVEONLY: Move constants to consensus.h MOVEONLY: Move constants to consensus.h #5696Separate CValidationState from main Separate CValidationState from main #5669Consensus: Refactor: Separate CheckFinalTx from main::IsFinalTx Consensus: Refactor: Separate CheckFinalTx from main::IsFinalTx #6063Consensus: Decouple ContextualCheckBlockHeader from checkpoints Consensus: Decouple ContextualCheckBlockHeader from checkpoints #5975Separate Consensus::CheckTxInputs and GetSpendHeight in CheckInputs Separate Consensus::CheckTxInputs and GetSpendHeight in CheckInputs #6061I'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).