Consensus: Remove ISM #8391

NicolasDorier · 2016-07-21T23:40:15Z

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.

NicolasDorier · 2016-07-21T23:45:43Z

src/main.cpp

        flags |= SCRIPT_VERIFY_DERSIG;
    }

    // Start enforcing CHECKLOCKTIMEVERIFY, (BIP65) for block.nVersion=4
    // blocks, when 75% of the network has upgraded:
-    if (block.nVersion >= 4 && IsSuperMajority(4, pindex->pprev, chainparams.GetConsensus().nMajorityEnforceBlockUpgrade, chainparams.GetConsensus())) {
+    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.

laanwj · 2016-07-22T12:36:07Z

src/rpc/blockchain.cpp

-              nFound;
-        pstart = pstart->pprev;
+        case 2:
+            activated = consensusParams.BIP34Height >= pindex->nHeight;


Shouldn't this comparison be the other way around?

NicolasDorier · 2016-07-22T12:51:29Z

Fixed the inverted condition of activation of ISM soft fork in RPC
Fix outdated rpc documentation for getblockchaininfo

instagibbs · 2016-07-22T13:40:27Z

src/rpc/blockchain.cpp

@@ -817,22  817,23 @@ UniValue verifychain(const UniValue& params, bool fHelp)
 }

 /** Implementation of IsSuperMajority with better feedback */
-static UniValue SoftForkMajorityDesc(int minVersion, CBlockIndex* pindex, int nRequired, const Consensus::Params& consensusParams)
+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

jtimon · 2016-07-22T14:50:42Z

src/consensus/params.h

    /** Block height and hash at which BIP34 becomes active */
    int BIP34Height;
    uint256 BIP34Hash;
+    /** 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...).

jtimon · 2016-07-22T15:22:50Z

utACK besides nits.

NicolasDorier · 2016-07-22T16:12:49Z

BIP34 does not activate on regnet, rationale explained on Consensus: Remove ISM #8391 (comment)

NicolasDorier · 2016-07-23T01:49:30Z

Sync of Testnet succeed, the Travis error is unrelated to this PR is it possible to run it again ? (still going on for mainnet)

NicolasDorier · 2016-07-26T04:34:41Z

Intial sync of testnet and mainnet succeed.

jtimon · 2016-07-26T16:19:55Z

Here are my proposed changes in code: jtimon/bitcoin@remove-ism...consensus-post-remove-ism

In any case, utACK 122786d

sipa · 2016-07-28T19:42:28Z

Note that #8418 adds a means to override the activation times for BIP9 activations in regtest mode. Perhaps a compatible approach can be followed here? Or is there no need for testing with different softforks activate in regtest?

sipa · 2016-07-31T23:21:16Z

utACK 122786d

NicolasDorier · 2016-07-31T23:57:45Z

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 NicolasDorier@e4844f7

laanwj · 2016-08-04T10:21:06Z

ACK 122786d

122786d Consensus: Remove ISM (NicolasDorier)

sdaftuar · 2016-08-04T15:15:10Z

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

jtimon · 2016-08-04T16:13:45Z

I agree a BIP makes sense.

NicolasDorier · 2016-08-04T20:42:51Z

will write BIP

122786d Consensus: Remove ISM (NicolasDorier)

8375bd8 [Tests] Use blocks v7 on regtest (random-zebra) 596902f [Core] Remove IsSuperMajority (random-zebra) 9cd2f81 [Consensus] Reject outdated blocks by enforcement height (random-zebra) Pull request description: `IsSuperMajority` checks are slow, and not needed after enforcement. Also ISM is not a good mechanism for deployment, so will not likely be used again after BIP65. (based on bitcoin#8391) We should implement better fork signal/activation solutions such as [BIP 9](https://github.com/bitcoin/bips/blob/master/bip-0009.mediawiki) in the future. **Note:** testnet resync completed. currently checking resync on mainnet. ACKs for top commit: furszy: Tested ACK 8375bd8 Fuzzbawls: ACK 8375bd8 Tree-SHA512: 587b08ab25b92661a362494023d914af606acce5af8ddb9bd4f4f36dc9dd621c6d5dfbe2a530c73aab3e9c47af90a98ff319cd4116e72eff2fdb424f4df517bd

NicolasDorier reviewed Jul 21, 2016
View reviewed changes

NicolasDorier force-pushed the remove-ism branch from 8d4ca9a to 86f5e2e Compare July 22, 2016 05:40

maflcko added the Consensus label Jul 22, 2016

laanwj reviewed Jul 22, 2016
View reviewed changes

NicolasDorier force-pushed the remove-ism branch from 86f5e2e to 9cf33d5 Compare July 22, 2016 12:48

NicolasDorier force-pushed the remove-ism branch 2 times, most recently from 4610c9f to 705d731 Compare July 22, 2016 12:55

instagibbs reviewed Jul 22, 2016
View reviewed changes

NicolasDorier force-pushed the remove-ism branch 2 times, most recently from eca5b6f to 316ecbd Compare July 22, 2016 14:32

jtimon reviewed Jul 22, 2016
View reviewed changes

Consensus: Remove ISM

122786d

NicolasDorier force-pushed the remove-ism branch from 316ecbd to 122786d Compare July 22, 2016 16:03

laanwj merged commit 122786d into bitcoin:master Aug 4, 2016

laanwj added a commit that referenced this pull request Aug 4, 2016

Merge #8391: Consensus: Remove ISM

37d83bb

122786d Consensus: Remove ISM (NicolasDorier)

NicolasDorier mentioned this pull request Aug 4, 2016

Parametrize buried soft fork in regtest and refactor #8460

Closed

mruddy mentioned this pull request Oct 18, 2016

More recent checkpoints? #7591

Closed

codablock mentioned this pull request Jan 19, 2018

Backport missing PRs from Bitcoin 0.14 - Part 9 dashpay/dash#1859

Merged

codablock pushed a commit to codablock/dash that referenced this pull request Jan 20, 2018

Merge bitcoin#8391: Consensus: Remove ISM

7321bfd

122786d Consensus: Remove ISM (NicolasDorier)

dagurval mentioned this pull request Jun 27, 2018

Consensus: Remove ISM bitcoinxt/bitcoinxt#467

Merged

andvgal pushed a commit to energicryptocurrency/gen2-energi that referenced this pull request Jan 6, 2019

Merge bitcoin#8391: Consensus: Remove ISM

aacc413

122786d Consensus: Remove ISM (NicolasDorier)

random-zebra mentioned this pull request Mar 16, 2020

[Consensus] Remove IsSuperMajority PIVX-Project/PIVX#1418

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consensus: Remove ISM #8391

Consensus: Remove ISM #8391

NicolasDorier commented Jul 21, 2016

NicolasDorier Jul 21, 2016

laanwj Jul 22, 2016

NicolasDorier commented Jul 22, 2016 •

edited

Loading

instagibbs Jul 22, 2016

NicolasDorier Jul 22, 2016

instagibbs Jul 22, 2016

jtimon Jul 22, 2016

NicolasDorier Jul 22, 2016

jtimon Jul 22, 2016

jtimon commented Jul 22, 2016

NicolasDorier commented Jul 22, 2016 •

edited

Loading

NicolasDorier commented Jul 23, 2016 •

edited

Loading

NicolasDorier commented Jul 26, 2016

jtimon commented Jul 26, 2016 •

edited

Loading

sipa commented Jul 28, 2016

sipa commented Jul 31, 2016

NicolasDorier commented Jul 31, 2016 •

edited

Loading

laanwj commented Aug 4, 2016

sdaftuar commented Aug 4, 2016

jtimon commented Aug 4, 2016

NicolasDorier commented Aug 4, 2016

Consensus: Remove ISM #8391

Consensus: Remove ISM #8391

Conversation

NicolasDorier commented Jul 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NicolasDorier commented Jul 22, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtimon commented Jul 22, 2016

NicolasDorier commented Jul 22, 2016 • edited Loading

NicolasDorier commented Jul 23, 2016 • edited Loading

NicolasDorier commented Jul 26, 2016

jtimon commented Jul 26, 2016 • edited Loading

sipa commented Jul 28, 2016

sipa commented Jul 31, 2016

NicolasDorier commented Jul 31, 2016 • edited Loading

laanwj commented Aug 4, 2016

sdaftuar commented Aug 4, 2016

jtimon commented Aug 4, 2016

NicolasDorier commented Aug 4, 2016

NicolasDorier commented Jul 22, 2016 •

edited

Loading

NicolasDorier commented Jul 22, 2016 •

edited

Loading

NicolasDorier commented Jul 23, 2016 •

edited

Loading

jtimon commented Jul 26, 2016 •

edited

Loading

NicolasDorier commented Jul 31, 2016 •

edited

Loading