Skip to content

Conversation

jtimon
Copy link
Contributor

@jtimon jtimon commented Mar 26, 2015

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

@jtimon jtimon changed the title WIP: Put all the consensus code together WIP: Encapsulate consensus code Mar 26, 2015
@gavinandresen
Copy link
Contributor

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.

@jtimon
Copy link
Contributor Author

jtimon commented Mar 28, 2015

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.
I will do that after I reorganize the code in 2 or 3 clear stages.

@jtimon jtimon force-pushed the consensus_tip branch 4 times, most recently from 19da7dd to e6f5a35 Compare April 2, 2015 00:24
@jtimon jtimon force-pushed the consensus_tip branch 3 times, most recently from 365b717 to fa32857 Compare April 10, 2015 00:44
@jtimon jtimon changed the title WIP: Encapsulate consensus code API: Expose bitcoinconsensus_verify_block() in libconsensus Apr 10, 2015
@jtimon jtimon changed the title API: Expose bitcoinconsensus_verify_block() in libconsensus DEPENDENT: API: Expose bitcoinconsensus_verify_block() in libconsensus Apr 10, 2015
@jtimon
Copy link
Contributor Author

jtimon commented Apr 10, 2015

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.

@gavinandresen
Copy link
Contributor

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?
If there is, then I'd expect to just store uint256 hashes, and when a pointer to a CBlockIndexBase is needed have some GetBlockHeader(hash) or GetBlockIndex(hash) function(s).

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)

@jtimon
Copy link
Contributor Author

jtimon commented Apr 10, 2015

Why factor out CBlockIndexBase and add a PrevIndexGetter everywhere?

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.
But we can start to prepare that removing all other dependencies that are not CBlockIndex first.

Why the move away from blockindex->pprev ? Is there a problem with memory management or storing pointers?

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.
It's just an idea. What is clear is that we cannot expose CBlockIndex as a C++ class to the C API.

If there is, then I'd expect to just store uint256 hashes, and when a pointer to a CBlockIndexBase is needed have some GetBlockHeader(hash) or GetBlockIndex(hash) function(s).

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.

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?

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 .

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.

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.

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)

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.
I think everything else will be mostly uncontroversial.

@jtimon jtimon changed the title DEPENDENT: API: Expose bitcoinconsensus_verify_block() in libconsensus WIP: DEPENDENT: API: Expose bitcoinconsensus_verify_block() in libconsensus Apr 19, 2015
jtimon added 24 commits April 20, 2015 01:07
from main.cpp:
CheckBlockHeader, IsSuperMajority, ContextualCheckBlockHeader

from miner.cpp
GetNextWorkRequired, CalculateNextWorkRequired, CheckProofOfWork
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
@jtimon
Copy link
Contributor Author

jtimon commented Apr 23, 2015

I realized this is mostly unreadable until something similar to #6051 is merged.
I will close this for now but I will still maintain the list of dependencies.

@jtimon jtimon closed this Apr 23, 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