I hard coded ISM SF in regtest and adapted the tests. Will try a sync on mainnet and testnet soon.
Note that even if the activation threshold (75% which enforce a rule for block version above a number) and enforcement threshold (95% reject old versions) are different, only the enforcement one really matter post fork.
2375 | } 2376 | 2377 | // Start enforcing CHECKLOCKTIMEVERIFY, (BIP65) for block.nVersion=4 2378 | // blocks, when 75% of the network has upgraded: 2379 | - if (block.nVersion >= 4 && IsSuperMajority(4, pindex->pprev, chainparams.GetConsensus().nMajorityEnforceBlockUpgrade, chainparams.GetConsensus())) { 2380 | + if (block.nVersion >= 4 && pindex->nHeight >= chainparams.GetConsensus().BIP65Height) {
actually, I don't need to compare the version. BIP65Height represent the 95% threshold height, so the first condition is always true.
829 | {
830 | - if (pstart->nVersion >= minVersion)
831 | - ++nFound;
832 | - pstart = pstart->pprev;
833 | + case 2:
834 | + activated = consensusParams.BIP34Height >= pindex->nHeight;
Shouldn't this comparison be the other way around?
816 | @@ -817,22 +817,23 @@ UniValue verifychain(const UniValue& params, bool fHelp) 817 | } 818 | 819 | /** Implementation of IsSuperMajority with better feedback */ 820 | -static UniValue SoftForkMajorityDesc(int minVersion, CBlockIndex* pindex, int nRequired, const Consensus::Params& consensusParams) 821 | +static UniValue SoftForkMajorityDesc(int minVersion, CBlockIndex* pindex, const Consensus::Params& consensusParams)
"minVersion" is a bit confusing after this.
What would be better ?
"version", like the caller SoftForkDesc
249 | - consensus.nMajorityRejectBlockOutdated = 950; 250 | - consensus.nMajorityWindow = 1000; 251 | - consensus.BIP34Height = -1; // BIP34 has not necessarily activated on regtest 252 | + consensus.BIP34Height = 100000000; // BIP34 has not necessarily activated on regtest 253 | consensus.BIP34Hash = uint256(); 254 | + consensus.BIP65Height = 1351; // BIP65 activated on regtest
seem like magic numbers without explanation
I'll update the comment, this magic number is chosen such that bip65 tests in python extended tests pass.
2366 | @@ -2372,15 +2367,13 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin 2367 | 2368 | unsigned int flags = fStrictPayToScriptHash ? SCRIPT_VERIFY_P2SH : SCRIPT_VERIFY_NONE; 2369 | 2370 | - // Start enforcing the DERSIG (BIP66) rules, for block.nVersion=3 blocks, 2371 | - // when 75% of the network has upgraded: 2372 | - if (block.nVersion >= 3 && IsSuperMajority(3, pindex->pprev, chainparams.GetConsensus().nMajorityEnforceBlockUpgrade, chainparams.GetConsensus())) { 2373 | + // Start enforcing the DERSIG (BIP66) rule 2374 | + if (pindex->nHeight >= chainparams.GetConsensus().BIP66Height) {
nit: Any reason not to leave version check in? The fewer differences the better, imo. (clearly this isn't a HF, just asking)
Yes, will later make refactoring for libconsensus easier because this does not depend on Block anymore.
Moreover, it is impossible that block.nVersion to be below 3 at this point. I'm using the 95% threshold for this test (compared to the 75% of before), so if the block.nVersion were below 3 it would have been rejected before by https://github.com/bitcoin/bitcoin/pull/8391/files#diff-7ec3c68a81efff79b6ca22ac1f1eabbaR3516
The IsSuperMajority call is slow.
@sipa? Hm? I was just asking about nVersion check.
That said, future refactoring purposes is good enough for me.
Yeah, as @NicolasDorier points out the block.nVersion check can be safely removed from here because blocks with invalid versions will be rejected in ContextualCheckBlockHeader.
170 | - consensus.nMajorityRejectBlockOutdated = 75; 171 | - consensus.nMajorityWindow = 100; 172 | consensus.BIP34Height = 21111; 173 | consensus.BIP34Hash = uint256S("0x0000000023b3a96d3484e5abb3755c413e7d41500f8e2a5c3f0dd01299cd8ef8"); 174 | + consensus.BIP65Height = 581885; // 00000000007f6655f22f98e72ed80d8b06dc761d5da09df0fa1dc4be4f861eb6 175 | + consensus.BIP66Height = 330776; // 000000002104c8c45e99a8853285a3b592602a3ccde2b832481da85e9e4ba182
Why is it not possible to check the hash for main and test?
Where would that be useful? The hash is only used as an exception to BIP30 rule at https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L2358
Well, in fact, what that's doing is that if we're past BIP34Height but at BIP34Height we don't have the hardcoded block, then keep checking bip30 as if bip34 hadn't been activated. But now in https://github.com/bitcoin/bitcoin/pull/8391/files#diff-7ec3c68a81efff79b6ca22ac1f1eabbaL3550 if we have a reorg below BIP34Height, BIP34 will be activated after BIP34Height regardless of the original activation being different, thus the optimization of not checking BIP30 if BIP34 is activated could be used as well. So BIP34Hash can go after this change, I believe. If we have a reorg to BIP34Height things are terribly broken anyway.
896 | @@ -897,13 +897,9 @@ UniValue getblockchaininfo(const UniValue& params, bool fHelp) 897 | " {\n" 898 | " \"id\": \"xxxx\", (string) name of softfork\n" 899 | " \"version\": xx, (numeric) block version\n" 900 | - " \"enforce\": { (object) progress toward enforcing the softfork rules for new-version blocks\n" 901 | + " \"reject\": { (object) progress toward rejecting pre-softfork blocks (same fields as \"enforce\")\n"
This comment doesn't really make sense to me since "enforce" is no longer anywhere.
244 | @@ -247,11 +245,10 @@ class CRegTestParams : public CChainParams { 245 | CRegTestParams() { 246 | strNetworkID = "regtest"; 247 | consensus.nSubsidyHalvingInterval = 150; 248 | - consensus.nMajorityEnforceBlockUpgrade = 750; 249 | - consensus.nMajorityRejectBlockOutdated = 950; 250 | - consensus.nMajorityWindow = 1000; 251 | - consensus.BIP34Height = -1; // BIP34 has not necessarily activated on regtest 252 | + consensus.BIP34Height = 100000000; // BIP34 has not necessarily activated on regtest
Why is this diff necessary?
@MarcoFalke I just re added it because without, tests with block v1 on regnet get rejected by the "obsolete version" (https://travis-ci.org/bitcoin/bitcoin/jobs/146661777).
It makes sense as if BIP34Height is -1 then https://github.com/bitcoin/bitcoin/pull/8391/files#diff-7ec3c68a81efff79b6ca22ac1f1eabbaR3515 will kick out.
44 | - int nMajorityRejectBlockOutdated; 45 | - int nMajorityWindow; 46 | /** Block height and hash at which BIP34 becomes active */ 47 | int BIP34Height; 48 | uint256 BIP34Hash; 49 | + /** Block height at which BIP65 becomes active */
We can make an array with an enum, like with vDeployments. Say, vPastDeployments, vOldDeployments, vBuriedDeployments, or something of the sort.
I prefer keeping change of logic and refactoring done in different PR. I want this PR to be easy to review, so I'm using the previous way of doing with BIP34Height.
Well, you can leave BIP34 as it is and put the new things in the array if you think that is less disruptive, no? Anyway, if you feel strongly against doing it now, we can do it later. It's just feel cleaner not to put code to destroy it "right afterwards" (I can foresee the same "we can do the refactor later" argument when moving BIP113 there...).
utACK besides nits.
Sync of Testnet succeed, the Travis error is unrelated to this PR is it possible to run it again ? (still going on for mainnet)
Intial sync of testnet and mainnet succeed.
Here are my proposed changes in code: https://github.com/jtimon/bitcoin/compare/remove-ism...consensus-post-remove-ism
In any case, utACK 122786d
utACK 122786d0e0170c73536360b705af711e1338adbf
I'll do a separate PR on top of this one for using the approach of #8418 alongs with @jtimon suggestion. I prefer keeping this PR as simple as I can.
EDIT: something like https://github.com/NicolasDorier/bitcoin/commit/e4844f776a5048cf7f10c0ec326e9a1ad73cb39f
ACK 122786d
@morcos and I were discussing this, and I think this change is subtle enough that it deserves some kind of BIP write-up.
Technically this change is a hardfork: old ISM-based code, when presented with an alternate chain history in which one of these softforks activated earlier, would then reject a subsequent block that violated the softfork before the hardcoded heights here, whereas this code would not reject such a block (since the softfork would not yet be enforced). This seems like a fundamental consequence of removing the ISM code (excluding doing something like checkpointing the whole headers chain to detect this kind of thing, which seems like obvious overkill).
In my judgement this is fine, as by the time we deploy this code in 0.14, it'll have been over a year since the most recent one of these softforks deployed. But to further everyone's understanding of how these kinds of consensus changes are made (especially for future developers, as well as authors of alternate implementations) I think it'd be beneficial to document this change along with the considerations and rationale that went into it.
I agree a BIP makes sense.
will write BIP