Implement BIP91 Reduced threshold Segwit MASF #10444

jameshilliard commented at 10:09 PM on May 23, 2017: contributor

This is an implementation of BIP91. BIP91 details: https://github.com/bitcoin/bips/blob/master/bip-0091.mediawiki Previous discussion: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2017-May/014380.html

Reduced threshold MASF to require segwit signalling

This is a BIP9 deployment at 65% that requires signalling
of segwit upon activation if segwit is not already
locked-in or activated. Signalling requirement stop at
segwit expiry, or activation.

e4b207fc36

Use version bit 4 with an 80% threshold for activation d5779e474f

fanquake added the label Consensus on May 24, 2017

fanquake added the label Validation on May 24, 2017

jonasschnelli commented at 6:32 AM on May 24, 2017: contributor

bip68-112-113-p2p.py-tests are falling...

Add segsignal to getblockchaininfo ddf0fce682

Use BIP91 activation threshold only on networks with a 95% BIP9 activation threshold. 6a3cfb4d2c

in src/validation.cpp:2934 in 6a3cfb4d2c outdated

2929 | @@ -2918,6 +2930,13 @@ bool IsWitnessEnabled(const CBlockIndex* pindexPrev, const Consensus::Params& pa
2930 |      return (VersionBitsState(pindexPrev, params, Consensus::DEPLOYMENT_SEGWIT, versionbitscache) == THRESHOLD_ACTIVE);
2931 |  }
2932 |  
2933 | +// Check if Segregated Witness is Locked In
2934 | +bool IsWitnessLockedIn(const CBlockIndex* pindexPrev, const Consensus::Params& params)

jtimon commented at 12:33 PM on May 27, 2017:

This function could be static

jameshilliard commented at 11:11 PM on June 14, 2017:

Any reason IsWitnessEnabled isn't also static?

in src/versionbits.cpp:156 in 6a3cfb4d2c outdated

 151 | @@ -148,7 +152,14 @@ class VersionBitsConditionChecker : public AbstractThresholdConditionChecker {
 152 |      int64_t BeginTime(const Consensus::Params& params) const { return params.vDeployments[id].nStartTime; }
 153 |      int64_t EndTime(const Consensus::Params& params) const { return params.vDeployments[id].nTimeout; }
 154 |      int Period(const Consensus::Params& params) const { return params.nMinerConfirmationWindow; }
 155 | -    int Threshold(const Consensus::Params& params) const { return params.nRuleChangeActivationThreshold; }
 156 | +    int Threshold(const Consensus::Params& params) const {
 157 | +        if (params.nRuleChangeActivationThreshold == 1916 && params.nMinerConfirmationWindow == 2016 &&

earonesty commented at 5:54 PM on May 30, 2017:

can these magic numbers - when not being used as actual thresholds - be constants?

jtimon commented at 4:35 PM on May 31, 2017:

This commit seems unnecessary, just don't use Consensus::DEPLOYMENT_SEGSIGNAL on chains that shouldn't use it (like regtest and testnet3).

jameshilliard commented at 10:20 PM on June 14, 2017:

Fixed

in src/versionbits.cpp:158 in e4b207fc36 outdated

 151 | @@ -148,7 +152,13 @@ class VersionBitsConditionChecker : public AbstractThresholdConditionChecker {
 152 |      int64_t BeginTime(const Consensus::Params& params) const { return params.vDeployments[id].nStartTime; }
 153 |      int64_t EndTime(const Consensus::Params& params) const { return params.vDeployments[id].nTimeout; }
 154 |      int Period(const Consensus::Params& params) const { return params.nMinerConfirmationWindow; }
 155 | -    int Threshold(const Consensus::Params& params) const { return params.nRuleChangeActivationThreshold; }
 156 | +    int Threshold(const Consensus::Params& params) const {
 157 | +        if (params.vDeployments[id].bit == params.vDeployments[Consensus::DEPLOYMENT_SEGSIGNAL].bit &&
 158 | +            params.vDeployments[id].nStartTime == params.vDeployments[Consensus::DEPLOYMENT_SEGSIGNAL].nStartTime) {
 159 | +            return 1311; // 65% threshold for SEGSIGNAL only

jtimon commented at 4:24 PM on May 31, 2017:

Perhaps this can be a field on Consensus::Params? or at least a properly defined constant

jameshilliard commented at 10:19 PM on June 14, 2017:

This is now defined in chainparams

in src/versionbits.cpp:156 in e4b207fc36 outdated

 151 | @@ -148,7 +152,13 @@ class VersionBitsConditionChecker : public AbstractThresholdConditionChecker {
 152 |      int64_t BeginTime(const Consensus::Params& params) const { return params.vDeployments[id].nStartTime; }
 153 |      int64_t EndTime(const Consensus::Params& params) const { return params.vDeployments[id].nTimeout; }
 154 |      int Period(const Consensus::Params& params) const { return params.nMinerConfirmationWindow; }
 155 | -    int Threshold(const Consensus::Params& params) const { return params.nRuleChangeActivationThreshold; }
 156 | +    int Threshold(const Consensus::Params& params) const {
 157 | +        if (params.vDeployments[id].bit == params.vDeployments[Consensus::DEPLOYMENT_SEGSIGNAL].bit &&

jtimon commented at 4:25 PM on May 31, 2017:

why not just if (id == Consensus::DEPLOYMENT_SEGSIGNAL) ?

jameshilliard commented at 12:07 AM on June 15, 2017:

Is the behavior for that identical?

in src/chainparams.cpp:100 in d5779e474f outdated

  96 | @@ -97,7 +97,7 @@ class CMainParams : public CChainParams {
  97 |          consensus.vDeployments[Consensus::DEPLOYMENT_SEGWIT].nTimeout = 1510704000; // November 15th, 2017.
  98 |  
  99 |          // Deployment of SEGSIGNAL
 100 | -        consensus.vDeployments[Consensus::DEPLOYMENT_SEGSIGNAL].bit = 2;
 101 | +        consensus.vDeployments[Consensus::DEPLOYMENT_SEGSIGNAL].bit = 4;

jtimon commented at 4:27 PM on May 31, 2017:

why bit 4 ? what's wrong with bit 2 ?

jameshilliard commented at 10:18 PM on June 14, 2017:

This was chosen because it was what was listed in the NYC agreement, I made a separate BIP that uses bit 2 however.

Override default confirmation window and threshold for SEGSIGNAL d5d9460f68

Enforce BIP91 when locked in e0a9003fee

in src/rpc/blockchain.cpp:1138 in ddf0fce682 outdated

1134 | @@ -1135,6 +1135,7 @@ UniValue getblockchaininfo(const JSONRPCRequest& request)
1135 |      softforks.push_back(SoftForkDesc("bip65", 4, tip, consensusParams));
1136 |      BIP9SoftForkDescPushBack(bip9_softforks, "csv", consensusParams, Consensus::DEPLOYMENT_CSV);
1137 |      BIP9SoftForkDescPushBack(bip9_softforks, "segwit", consensusParams, Consensus::DEPLOYMENT_SEGWIT);
1138 | +    BIP9SoftForkDescPushBack(bip9_softforks, "segsignal", consensusParams, Consensus::DEPLOYMENT_SEGSIGNAL);

jtimon commented at 4:29 PM on May 31, 2017:

I would squash this commit and the previous one into the first commit (perhaps after review would just squash it all in one commit, it's all 1 logical change IMO).

jameshilliard commented at 11:39 PM on June 14, 2017: contributor

I've modified the BIP91 spec slightly with a different threshold and immediate enforcement upon lock-in.

Add regtest parameters for SEGSIGNAL 78e37a40dd

Enforce SEGSIGNAL only when SEGWIT is in STARTED state

bit 1 signaling is not required (and has no meaning) unless the state is STARTED

19a97987ed

[qa] Add RPC test for SEGSIGNAL 4f23724fa6

[qa] Fix tests under SEGSIGNAL de82047244

Reduce the SEGSIGNAL signalling window to 336 blocks and enforce only when ACTIVE 55caca1ca8

[qa] fix tests for SEGSIGNAL LOCKED_IN 200aedd26f

jameshilliard commented at 5:51 AM on June 16, 2017: contributor

I've updated BIP91 to use an even smaller confirmation window and changed it so enforcement isn't until activation in order to give a ~2 day grace period between lock-in and enforcement.

luke-jr commented at 4:01 AM on June 19, 2017: member

NACK, this does nothing for users, and is unnecessary with BIP148 around the corner.

earonesty commented at 4:45 PM on June 19, 2017: none

ACK: avoiding a chain split in a coordinated manner is certainly not "doing nothing for users". Since the -bip148 option is not available, a BIP91 feature is the best alternative. I would be fine with either.

dooglus commented at 7:04 AM on July 21, 2017: contributor

Apparently BIP91 was locked in today.

How can I make sure my node follows the correct chain? Is this pull request on a branch somewhere or do I need to merge it in my own local repository?

And what about the thousands of other nodes out there? Shouldn't this PR be merged to allow them to track the correct chain?

jameshilliard commented at 7:25 AM on July 21, 2017: contributor

@dooglus BIP91 is mostly a miner enforced soft fork, you can use this tree.

dooglus commented at 7:31 AM on July 21, 2017: contributor

As I understand Bitcoins miners don't enforce anything. They mine blocks which are either valid or invalid. It is the users who decide whether to accept their blocks or not, and thereby enforce the rules.

I don't see any argument here against merging this PR other than Luke's inaccurate complaint that BIP91 does nothing for users. What am I missing?

jameshilliard commented at 7:40 AM on July 21, 2017: contributor

@dooglus Both miners and users have the ability to enforce rules, a majority of miners can add a new rule without creating a chain split(assuming other nodes don't change their rules). Since a majority of miners are adding a rule with BIP91 the blocks will be valid to existing nodes. There's mostly 2 arguments I'm seeing, one is that more users should enforce the BIP91 rules to strengthen the rule enforcement, the other is that users should not so that there is more consistency among rule enforcement. I somewhat agree with the consistency argument, however any businesses accepting transactions without many confirmations should be running both BIP91 and non-BIP91 nodes and ensuring that the transactions are confirmed on both.

dooglus commented at 5:08 PM on July 21, 2017: contributor

however any businesses accepting transactions without many confirmations should be running both BIP91 and non-BIP91 nodes and ensuring that the transactions are confirmed on both

Exactly. That seems like an argument for merging this PR so that I can run a BIP91 node without having to trust or code review a non-core repository.

achow101 commented at 7:12 PM on July 21, 2017: member

Can this be squashed and possibly updated with the latest from the segsignal repo?

jameshilliard commented at 2:58 PM on August 9, 2017: contributor

Closing this as enforcement of BIP91 is no longer required.

jameshilliard closed this on Aug 9, 2017

DrahtBot locked this on Sep 8, 2021