Consensuslib: Block Verify / Transaction Verify [Do not merge, work in progress]

NicolasDorier commented at 6:05 am on July 14, 2016: contributor

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 .

NicolasDorier renamed this:
~~Consensuslib: Block Verify / Transaction Verify [Work in progress]~~
Consensuslib: Block Verify / Transaction Verify [Do not merge, work in progress]
on Jul 14, 2016

NicolasDorier force-pushed on Jul 14, 2016

jtimon commented at 11:26 am on July 14, 2016: contributor

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 ). I really think we should try to work on a common base…

in src/main.cpp: in 49c22c5dde outdated

2503     assert(pindex);
2504     CConsensusContextInfo contextInfo;
2505     contextInfo.height = pindex->nHeight;
2506     contextInfo.medianTimePast = pindex->GetMedianTimePast();
2507     contextInfo.now = GetAdjustedTime();
2508     contextInfo.bestBlock = pindex->GetBlockHeader();

jtimon commented at 11:27 am on July 14, 2016:

Do you plan to move GetContextInfo() to libconsensus ? If not, please don’t move consensus critical code out of consensus critical functions.

NicolasDorier commented at 11:45 am on July 14, 2016:

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.

in src/main.cpp: in 36eb1a0e77 outdated

3598@@ -3583,11 +3599,11 @@ bool ContextualCheckBlock(const CBlock& block, CValidationState& state, const CC
3599             // already does not permit it, it is impossible to trigger in the
3600             // witness tree.
3601             if (block.vtx[0].wit.vtxinwit.size() != 1 || block.vtx[0].wit.vtxinwit[0].scriptWitness.stack.size() != 1 || block.vtx[0].wit.vtxinwit[0].scriptWitness.stack[0].size() != 32) {
3602-                return state.DoS(100, error("%s : invalid witness nonce size", __func__), REJECT_INVALID, "bad-witness-nonce-size", true);
3603+                return state.DoS(100, logger.error("%s : invalid witness nonce size", __func__), REJECT_INVALID, "bad-witness-nonce-size", true);

jtimon commented at 11:32 am on July 14, 2016:

There’s no logging needed in libconsensus, CValidationState should be enough, see https://github.com/bitcoin/bitcoin/pull/7287/files

in src/script/blockvalidator.cpp: in 71b29a4de8 outdated

3@@ -4,6 +4,106 @@
4 
5 #include "blockvalidator.h"
6

jtimon commented at 11:33 am on July 14, 2016:

If we’re not going to try to move the code to the same files, can you please at least use the consensus directory instead of the script one?

in src/pow.cpp: in 5eef3d92be outdated

72@@ -73,22 +73,3 @@ unsigned int CalculateNextWorkRequired(const CBlockIndex* pindexLast, int64_t nF
73 
74     return bnNew.GetCompact();
75 }

jtimon commented at 11:37 am on July 14, 2016:

pow is already a file containing only consensus code and ready for becoming part of the consensus module/package (see makefile) except for its CBlockIndex dependency. Just call CheckProofOfWork from the other consensus code directly, it’s ok to include pow.h

NicolasDorier commented at 2:29 pm on July 14, 2016:

@jtimon actually, it is not ok, pow is not part of consensus (some methods are linked to CBlockIndex)

jtimon commented at 11:16 pm on July 14, 2016:

then we should remove the undesired dependencies.

On Thu, Jul 14, 2016 at 4:30 PM, Nicolas Dorier notifications@github.com wrote:

In src/pow.cpp #8339 (review):
@@ -73,22 +73,3 @@ unsigned int CalculateNextWorkRequired(const CBlockIndex* pindexLast, int64_t nF
0 return bnNew.GetCompact();
}
@jtimon https://github.com/jtimon actually, it is not ok, pow is not part of consensus (some methods are linked to CBlockIndex)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/8339/files/95b710aabc271222b669da505fc50abe67413df2..5eef3d92beecb72cef6dd6dd4af55ea15dc45b94#r70815431, or mute the thread https://github.com/notifications/unsubscribe/AA9jSpRVFQNcuxXn4wPG4yGXQoihYfWbks5qVkfvgaJpZM4JMHBr .

NicolasDorier commented at 10:01 am on July 15, 2016:

Ultimately I agree, however it means we need either to put CBlockIndex as part of consensuslib or making a stripped type representing it in consensuslib.

But it involves lots of refactoring that I don’t need it for my VerifyBlock / VerifyTx so I won’t do it for now. I’ll maybe include nextWorkRequired as field in CConsensusContextInfo and calculated in GetContextualInfo() though.

in src/main.cpp: in c5b19d92be outdated

2396     }
2397 
2398     // Start enforcing WITNESS rules using versionbits logic.
2399     if (IsWitnessEnabled(pindex->pprev, chainparams.GetConsensus())) {
2400-        flags |= SCRIPT_VERIFY_WITNESS;
2401+        consensusFlags.scriptFlags |= SCRIPT_VERIFY_WITNESS;

jtimon commented at 11:39 am on July 14, 2016:

As discussed the consensusFlags struct seems to just cause unnecessary disruption with no apparent benefit.

NicolasDorier commented at 11:49 am on July 14, 2016:

Agree, I’ll fix that at the end.

in src/main.cpp: in 95b710aabc outdated

2527@@ -2528,13 +2528,13 @@ CConsensusFlags GetConsensusFlags(const CBlock& block, const CBlockIndex *pindex
2528 
2529     // Start enforcing the DERSIG (BIP66) rules, for block.nVersion=3 blocks,
2530     // when 75% of the network has upgraded:
2531-    if (block.nVersion >= 3 && IsSuperMajority(3, pindex->pprev, chainparams.GetConsensus().nMajorityEnforceBlockUpgrade, chainparams.GetConsensus())) {
2532+    if (pindex->nVersion >= 3 && IsSuperMajority(3, pindex->pprev, chainparams.GetConsensus().nMajorityEnforceBlockUpgrade, chainparams.GetConsensus())) {

jtimon commented at 11:40 am on July 14, 2016:

aren’t you changing functionality here?

NicolasDorier commented at 11:48 am on July 14, 2016:

no, where it is used, pindex points to the block. (I ran the tests after this change, worked fine)

NicolasDorier commented at 11:44 am on July 14, 2016: contributor

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

jtimon commented at 11:50 am on July 14, 2016: contributor

How did you do for the logging ?

#8339 (review)

with CValidationState I see you just remove the “error” methods ?

Look again to the PR I link you, the messages aren’t lost.

Also I broke the consensus somewhere :p

Maybe in #8339 (review) ?

jonasschnelli added the label Refactoring on Jul 14, 2016

NicolasDorier commented at 11:56 am on July 14, 2016: contributor

Awesome I’ll cherry pick your commit and remove my logger.

EDIT: ho I need to do it for the new methods :(

NicolasDorier commented at 11:57 am on July 14, 2016: contributor

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 on Jul 14, 2016

NicolasDorier commented at 12:14 pm on July 14, 2016: contributor

@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 ?

jtimon commented at 12:59 pm on July 14, 2016: contributor

You need to make sure you print from all callers, yes. You replace the error(“my message”) with false, yes, but also, at the end, you add ‘’’, false, “my message”"" so that you don’t lose the longer message.

NicolasDorier force-pushed on Jul 14, 2016

NicolasDorier commented at 4:31 pm on July 14, 2016: contributor

@jtimon https://github.com/NicolasDorier/bitcoin/commit/53f677b8240f4eb40803885473ddbc5a51991dd2 might be useful (independant commit which remove error from contextualcheckblockheader)

NicolasDorier commented at 3:08 am on July 15, 2016: contributor

Yes, I just created #8341 with the reference of the two callers. (they already have error calls)

NicolasDorier force-pushed on Jul 15, 2016

NicolasDorier force-pushed on Jul 16, 2016

Consensus: Trivial transform BOOST_FOREACH into for loop df47ff2da7

Consensus: Remove calls to error() from ContextualCheckBlock d748cfd953

Encapsulate consensus flags into CConsensusFlags 508716c725

Encapsulate CConsensusFlags calculation a59f79dfb7

GetConsensusFlags does not depend on CBlock 068a6915ac

ContextualCheckBlock uses CConsensusFlags 9bc6c02630

ContextualCheckBlock does not depends on CBlockIndex 57c3c46a39

ContextualCheckBlockHeader does not depends on CBlockIndex 6e6ecd8e31

NicolasDorier force-pushed on Jul 16, 2016

fanquake commented at 1:18 pm on January 12, 2017: member

Closing this for now due to inactivity. @NicolasDorier feel free to reopen if your continuing this work.

fanquake closed this on Jan 12, 2017

fanquake moved this from the "In progress" to the "Plans and discussion" column in a project

MarcoFalke locked this on Sep 8, 2021

Consensuslib: Block Verify / Transaction Verify [Do not merge, work in progress] #8339