Consensuslib: Block Verify / Transaction Verify [Do not merge, work in progress] #8339
Conversation
NicolasDorier
changed the title
NicolasDorier
force-pushed
the
blkconsensus
branch
7 times, most recently
from
bd83e46 to
49c22c5
Compare
|
Actually I'm copying/re-writing things from that branch to https://github.com/jtimon/bitcoin/tree/0.12.99-consensus (which contains #8328 #8329 and #8337 ). |
src/main.cpp
Outdated
There was a problem hiding this comment.
Do you plan to move GetContextInfo() to libconsensus ? If not, please don't move consensus critical code out of consensus critical functions.
There was a problem hiding this comment.
I noticed that the consensus code only depends on the current majorityVersion. So I'm calculating the majorityVersion outside consensus, and I make the check inside. I've not found a better solution for now.
|
Thanks @jtimon, it is actually moving very fast now, I'll try to consolidate with the same base a you when the dust in my code will settle. For now I'll fix the pow you just told. How did you do for the logging ? with CValidationState I see you just remove the "error" methods ? (I really don't like the "Logger" abstraction, but I don't want to remove features either :() Also I broke the consensus somewhere :p |
Look again to the PR I link you, the messages aren't lost.
Maybe in #8339 (comment) ? |
|
Awesome I'll cherry pick your commit and remove my logger. EDIT: ho I need to do it for the new methods :( |
|
the consensus break in the time-too-old rule. (block too early) EDIT: Found the culprit, in TestValidityBlock I was not using prevIndex for calculating the context. |
NicolasDorier
force-pushed
the
blkconsensus
branch
from
49c22c5 to
3656767
Compare
|
@jtimon if I understand well https://github.com/bitcoin/bitcoin/pull/7287/files actually I only need to remove error and pass "false" to DoS/Invalid ValidationState methods right? Since you are already printing out stuff in the caller ? |
|
You need to make sure you print from all callers, yes. |
NicolasDorier
force-pushed
the
blkconsensus
branch
6 times, most recently
from
2d70454 to
1059bc8
Compare
|
@jtimon NicolasDorier@53f677b might be useful (independant commit which remove error from contextualcheckblockheader) |
|
Yes, I just created #8341 with the reference of the two callers. (they already have error calls) |
NicolasDorier
force-pushed
the
blkconsensus
branch
2 times, most recently
from
d86e85d to
28a6030
Compare
NicolasDorier
force-pushed
the
blkconsensus
branch
6 times, most recently
from
7cc5844 to
786e831
Compare
NicolasDorier
force-pushed
the
blkconsensus
branch
from
786e831 to
6e6ecd8
Compare
|
Closing this for now due to inactivity. @NicolasDorier feel free to reopen if your continuing this work. |
4 participants
It is a work in progress, I manage to remove the CBlockIndex dependency from consensus block by adding two new types: CConsensusFlags and CConsensusContextualInfo (last block, last block mtp, now, last block height). Both of them can be trivial calculated in main() with pIndex, then passed to consensus method.
The code change is minimal and easy to review.
BlockVerify/TransactionVerify will require the user to pass the CConsensusFlags as well CConsensusContextualInfo.
A GetFlags method without too much code is difficult as it relies heavily on CBlockIndex. It is also fairly easy enough to port into other languages.
As such it is considered out of scope of this PR.
Alternatively, @jtimon is also working on https://github.com/jtimon/bitcoin/commits/jt .