Consensus: Use pindex for ISM check

NicolasDorier commented at 9:31 am on July 16, 2016: contributor

This is the first step to later make a single “ConsensusFlags” type or enum.

When VerifyBlock and VerifyTx will be in libconsensus, the lib will probably still not have access to CBlockIndex. The user of libconsensus would be responsible for giving the ConsensusFlags. (as done now with EvalScript)

The ConsensusFlag calculation will thus be a function in Main and would depends only on ConsensusParams and the top CBlockIndex. Thanks to that, lots of consensus function currently in Main will not have any dependencies to CBlockIndex left.

Even if you don’t really agree on this plan, this commit add a “const” constraint in the passed CBlockIndex, and make the codebase more coherent. @jtimon, it will be usefull for your current libconsensus work as well.

in src/main.cpp: in f59137427a outdated

2292@@ -2293,10 +2293,11 @@ static int64_t nTimeIndex = 0;
2293 static int64_t nTimeCallbacks = 0;
2294 static int64_t nTimeTotal = 0;
2295 
2296-bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pindex,
2297+bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* const pindex,
2298                   CCoinsViewCache& view, const CChainParams& chainparams, bool fJustCheck)
2299 {
2300     AssertLockHeld(cs_main);
2301+    assert(pindex->nVersion == block.nVersion);

paveljanik commented at 9:49 am on July 16, 2016:

Does this apply every time?

NicolasDorier commented at 10:08 am on July 16, 2016:

I added the assert, because if that’s not the case, then my change is not without side effect. I reviewed the caller, and it seems that pindex always point to block’s CBlockIndex.

in src/main.cpp: in f59137427a outdated

3522@@ -3522,9 +3523,12 @@ bool ContextualCheckBlockHeader(const CBlockHeader& block, CValidationState& sta
3523     return true;
3524 }
3525 
3526-bool ContextualCheckBlock(const CBlock& block, CValidationState& state, CBlockIndex * const pindexPrev)
3527+bool ContextualCheckBlock(const CBlock& block, CValidationState& state, CBlockIndex * const pindex)
3528 {
3529-    const int nHeight = pindexPrev == NULL ? 0 : pindexPrev->nHeight + 1;
3530+    CBlockIndex * const pindexPrev = pindex->pprev;

paveljanik commented at 9:50 am on July 16, 2016:

assert(pindex) first?

in src/main.cpp: in f59137427a outdated

3547@@ -3544,9 +3548,9 @@ bool ContextualCheckBlock(const CBlock& block, CValidationState& state, CBlockIn
3548         }
3549     }
3550 
3551-    // Enforce block.nVersion=2 rule that the coinbase starts with serialized block height
3552+    // Enforce pindex->nVersion =2 rule that the coinbase starts with serialized block height

paveljanik commented at 9:50 am on July 16, 2016:

super minor nit: added SPC before =2

in src/main.h: in f59137427a outdated

449@@ -450,7 +450,7 @@ bool ContextualCheckBlock(const CBlock& block, CValidationState& state, CBlockIn
450 /** Apply the effects of this block (with given index) on the UTXO set represented by coins.
451  *  Validity checks that depend on the UTXO set are also done; ConnectBlock()
452  *  can fail if those validity checks fail (among other reasons). */
453-bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pindex, CCoinsViewCache& coins,
454+bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* const pindex, CCoinsViewCache& coins,

paveljanik commented at 9:57 am on July 16, 2016:

Hmm, compare the prototype of ContextualCheckBlock and ConnectBlock: pindexPrev vs. pindex. I think this calls for oversights.

paveljanik commented at 9:58 am on July 16, 2016: contributor

Travis fails with:

0stderr:
1 bitcoind: ../../src/main.cpp:3529: bool ContextualCheckBlock(const CBlock&, CValidationState&, CBlockIndex*): Assertion `pindexPrev' failed.

NicolasDorier commented at 9:59 am on July 16, 2016: contributor

yes, it seems that prevIndex can be NULL, hopefully MTP can’t be activated at height 0.

NicolasDorier force-pushed on Jul 16, 2016

NicolasDorier commented at 10:23 am on July 16, 2016: contributor

@paveljanik fixed the nit and the travis now should pass. I added https://github.com/bitcoin/bitcoin/pull/8344/files#diff-7ec3c68a81efff79b6ca22ac1f1eabbaR3540 .

Even if (nLockTimeFlags & LOCKTIME_MEDIAN_TIME_PAST) && !pindexPrev is impossible right now, because MTP can’t possibly be activated at block height = 0. However, in consensus lib, the user will be able to verify a block of height 0 with the MTP rule activated, so better we don’t crash in this case.

Consensus: Use pindex for ISM check 01d903d564

NicolasDorier force-pushed on Jul 16, 2016

jtimon commented at 2:10 pm on July 16, 2016: contributor

Just created #8345 related to this one. I guess with this the interface for Consensus::GetFlags() becomes cleaner, but I don’t think this is strictly necessary or a priority for consensus encapsulation. Concept ACK.

Regarding not calculating the flags within libconsensus, I strongly disagree. I don’t think users of libconsensus should be expected to reimplement everything in https://github.com/bitcoin/bitcoin/pull/8345/files#diff-d6954440f0346f8e42dcd669dd4aeafaR173 correctly. I want to expose GetFlags in the C API so users can just call that before calling VerifyTx and/or VerifyScript. My thought was that VerifyBlock would call GetFlags internally, but I guess it can take a flags parameter instead. I also don’t see that as a solution to avoid having to create a C API-compatible for CBlockIndex, since you will still need to have it for the pow checks in VerifyHeader() and VerifyBlock().

NicolasDorier commented at 2:25 pm on July 16, 2016: contributor

Related to your PR, this commit only allows you to call GetFlags without passing a CBlock to it.

jtimon commented at 5:07 pm on July 16, 2016: contributor

Yes, another option would be to pass an nBlockVersion to GetFlags like in https://github.com/bitcoin/bitcoin/pull/8345/commits/8c893cfcb949ffd03d8951f8ae916e7a0a2293ed#diff-7ec3c68a81efff79b6ca22ac1f1eabbaR3450 I guess your version is cleaner because it has one parameter less but seems more disruptive. Certainly passing the whole CBlock is unnecessary.

jonasschnelli added the label Refactoring on Jul 17, 2016

NicolasDorier commented at 12:51 pm on July 21, 2016: contributor

@gmaxwell seems to be working on removing ISM completely, making this PR useless. https://botbot.me/freenode/bitcoin-core-dev/2016-07-17/?msg=69776851&page=1

NicolasDorier closed this on Jul 21, 2016

MarcoFalke locked this on Sep 8, 2021

Consensus: Use pindex for ISM check #8344