-
Notifications
You must be signed in to change notification settings - Fork 37.7k
WIP: DEPENDENT: API: Expose bitcoinconsensus_verify_block() in libconsensus #5946
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
RE: people who want to see the big picture: A design document for libconsensus would be a much more efficient way for people like me to get the big picture. And seeing the API you're aiming for would be really nifty, too. |
Yes, I'm trying to separate what could get into the next libconsensus relatively easily from what will require some API discussion first. I was planning on just writing to the mailing list for the later, but having an editable document in the wiki or somewhere with the whole proposed API (even if it has question marks in some functions) will probably be better. |
19da7dd
to
e6f5a35
Compare
365b717
to
fa32857
Compare
I updated the PR text with something more descriptive, including a PR where there's actually an API to judge. I'm not very happy with it as it is myself, because it ends with a little bit of a hack to get rid of the chain.o dependency. But it should serve to get an idea of the kind of API I would like to propose, at least for VerifyBlockHeader. |
Ok, some big-picture design questions I have after spending a few minutes looking through the blizzard of commits: Why factor out CBlockIndexBase and add a PrevIndexGetter everywhere? That seems like a weird hybrid way of doing things. Why the move away from blockindex->pprev ? Is there a problem with memory management or storing pointers? Or if you're trying to save memory not storing a prevHash-- why not just have GetPrevBlock(parent_hash) as part of the API? Are you worried it would be too slow? On a higher level: what is libconsensus supposed to be do versus not do? I'm gathering it is to encapsulate all of the validation logic, but leave storing/managing all the data up to higher layers. How are the layers supposed to be connected-- pass in lookup functions to the libconsensus methods, as needed? Register lookup methods at startup? (that tends to create a friendlier API, in my opinion, but maybe one that is harder to test) |
Well, that's the particular interface I chose to replace CBlockIndex. I don't like much myself but I wanted to give an example of a C API at the end of #5995. As explained there, that's the last thing is done (there) to help people replace those last 2 commits with their own proposal.
Well, maybe we want to expose a struct with pprev directly in the C API. That's what needs discussion, how are other codebases using libconsensus expected to feed VerifyBlockHeader with chain index data. I think the function pointer I chose is quite flexible. Maybe another codebase maintains a dictionary nHeight -> blockHeader and other codebases give a pointer to a function that takes the height index, substracts one and looks in that dictionary.
That's another solution that in fact I like more, and then other implementations could replace that getter with whatever they want. But this will also require more changes than my little hack.
Yeah, if it wasn't for the extra memory of storing prevHash we should already have a common base for CBlockIndex and CBlockHeader. The other suggestions are possible too, that's the kind of discussion I would like to have in #5995 .
That's what I envision as well. Although, @luke-jr for example thinks that we should also expose a validation + basic storage library. In any case that would be something to do later.
Yes, I think having lookup function pointers as parameters of the validation functions is what makes more sense. It is very flexible for other implementations and it allows bitcoin core to adapt to that without necessarily having to radically change its internal structures. The question is which function pointers should replace CBlockIndex and CCoinsViewCache. |
160f8d7
to
b909512
Compare
from main.cpp: CheckBlockHeader, IsSuperMajority, ContextualCheckBlockHeader from miner.cpp GetNextWorkRequired, CalculateNextWorkRequired, CheckProofOfWork
…functions in main.cpp and miner.cpp
Decouple it from util.h [bool error(char*)] and BOOST_FOREACH
…arately from main::CheckInputs when possible
from main.cpp: -CheckFinalTx -Consensus::CheckTx -Consensus::CheckTxInputs -GetLegacySigOpCount -GetP2SHSigOpCount
…renamed GetBlockSubsidy] Remove redundant getter CChainParams::SubsidyHalvingInterval()
and use it instead of MANDATORY_SCRIPT_VERIFY_FLAGS in miner::CreateNewBlock()
…ify.cpp: from main: -CheckBlock -ContextualCheckBlock -GetBlockSubsidy
I realized this is mostly unreadable until something similar to #6051 is merged. |
To fully verify a block one needs to be able to verify block headers and transactions first.
Therefor VerifyBlock() can only be exposed after VerifyTransaction() and VerifyBlockHeader() are exposed or ready to be exposed as well.
We can work on exposing VerifyTransaction() and VerifyBlockHeader() in parallel. This PR contains all the work I'm doing on those rebased together plus some extra commits towards exposing a VerifyBlock later.
Dependencies:
-API: Expose bitcoinconsensus_verify_header() in libconsensus #5995