Implement BIP91 Reduced threshold Segwit MASF #10444

jameshilliard · 2017-05-23T22:09:30Z

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

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.

jonasschnelli · 2017-05-24T06:32:08Z

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

…ation threshold.

jtimon · 2017-05-27T12:33:27Z

src/validation.cpp

@@ -2918,6 +2930,13 @@ bool IsWitnessEnabled(const CBlockIndex* pindexPrev, const Consensus::Params& pa
    return (VersionBitsState(pindexPrev, params, Consensus::DEPLOYMENT_SEGWIT, versionbitscache) == THRESHOLD_ACTIVE);
 }

+// Check if Segregated Witness is Locked In
+bool IsWitnessLockedIn(const CBlockIndex* pindexPrev, const Consensus::Params& params)


This function could be static

Any reason IsWitnessEnabled isn't also static?

earonesty · 2017-05-30T17:54:10Z

src/versionbits.cpp

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


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

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

jtimon · 2017-05-31T16:24:26Z

src/versionbits.cpp

+    int Threshold(const Consensus::Params& params) const {
+        if (params.vDeployments[id].bit == params.vDeployments[Consensus::DEPLOYMENT_SEGSIGNAL].bit &&
+            params.vDeployments[id].nStartTime == params.vDeployments[Consensus::DEPLOYMENT_SEGSIGNAL].nStartTime) {
+            return 1311; // 65% threshold for SEGSIGNAL only


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

This is now defined in chainparams

jtimon · 2017-05-31T16:25:31Z

src/versionbits.cpp

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


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

Is the behavior for that identical?

jtimon · 2017-05-31T16:27:38Z

src/chainparams.cpp

@@ -97,7 +97,7 @@ class CMainParams : public CChainParams {
        consensus.vDeployments[Consensus::DEPLOYMENT_SEGWIT].nTimeout = 1510704000; // November 15th, 2017.

        // Deployment of SEGSIGNAL
-        consensus.vDeployments[Consensus::DEPLOYMENT_SEGSIGNAL].bit = 2;
+        consensus.vDeployments[Consensus::DEPLOYMENT_SEGSIGNAL].bit = 4;


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

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

jtimon · 2017-05-31T16:29:29Z

src/rpc/blockchain.cpp

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


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 · 2017-06-14T23:39:36Z

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

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

… when ACTIVE

jameshilliard · 2017-06-16T05:51:45Z

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 · 2017-06-19T04:01:05Z

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

earonesty · 2017-06-19T16:45:30Z

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 · 2017-07-21T07:04:21Z

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 · 2017-07-21T07:25:15Z

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

dooglus · 2017-07-21T07:31:08Z

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 · 2017-07-21T07:40:11Z

@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 · 2017-07-21T17:08:54Z

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 · 2017-07-21T19:12:19Z

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

jameshilliard · 2017-08-09T14:58:50Z

Closing this as enforcement of BIP91 is no longer required.

shaolinfry and others added 2 commits May 21, 2017 23:30

Reduced threshold MASF to require segwit signalling

e4b207f

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.

Use version bit 4 with an 80% threshold for activation

d5779e4

fanquake added Consensus Validation labels May 24, 2017

jameshilliard added 2 commits May 24, 2017 12:08

Add segsignal to getblockchaininfo

ddf0fce

Use BIP91 activation threshold only on networks with a 95% BIP9 activ…

6a3cfb4

…ation threshold.

jtimon reviewed May 27, 2017

View reviewed changes

earonesty reviewed May 30, 2017

View reviewed changes

jtimon reviewed May 31, 2017

View reviewed changes

jtimon mentioned this pull request Jun 2, 2017

[consensus] SegWit2x signalling on bit 4 btc1/bitcoin#6

Closed

jl2012 and others added 2 commits June 15, 2017 02:33

Override default confirmation window and threshold for SEGSIGNAL

d5d9460

Enforce BIP91 when locked in

e0a9003

jl2012 added 6 commits June 16, 2017 00:39

Add regtest parameters for SEGSIGNAL

78e37a4

Enforce SEGSIGNAL only when SEGWIT is in STARTED state

19a9798

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

[qa] Add RPC test for SEGSIGNAL

4f23724

[qa] Fix tests under SEGSIGNAL

de82047

Reduce the SEGSIGNAL signalling window to 336 blocks and enforce only…

55caca1

… when ACTIVE

[qa] fix tests for SEGSIGNAL LOCKED_IN

200aedd

jameshilliard closed this Aug 9, 2017

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

Implement BIP91 Reduced threshold Segwit MASF #10444

Implement BIP91 Reduced threshold Segwit MASF #10444

jameshilliard commented May 23, 2017

jonasschnelli commented May 24, 2017 •

edited

Loading

jtimon May 27, 2017

jameshilliard Jun 14, 2017

earonesty May 30, 2017

jtimon May 31, 2017

jameshilliard Jun 14, 2017

jtimon May 31, 2017

jameshilliard Jun 14, 2017

jtimon May 31, 2017

jameshilliard Jun 15, 2017

jtimon May 31, 2017

jameshilliard Jun 14, 2017

jtimon May 31, 2017 •

edited

Loading

jameshilliard commented Jun 14, 2017

jameshilliard commented Jun 16, 2017

luke-jr commented Jun 19, 2017

earonesty commented Jun 19, 2017 •

edited

Loading

dooglus commented Jul 21, 2017

jameshilliard commented Jul 21, 2017

dooglus commented Jul 21, 2017

jameshilliard commented Jul 21, 2017

dooglus commented Jul 21, 2017

achow101 commented Jul 21, 2017

jameshilliard commented Aug 9, 2017

Implement BIP91 Reduced threshold Segwit MASF #10444

Implement BIP91 Reduced threshold Segwit MASF #10444

Conversation

jameshilliard commented May 23, 2017

jonasschnelli commented May 24, 2017 • 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

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 May 31, 2017 • edited Loading

Choose a reason for hiding this comment

jameshilliard commented Jun 14, 2017

jameshilliard commented Jun 16, 2017

luke-jr commented Jun 19, 2017

earonesty commented Jun 19, 2017 • edited Loading

dooglus commented Jul 21, 2017

jameshilliard commented Jul 21, 2017

dooglus commented Jul 21, 2017

jameshilliard commented Jul 21, 2017

dooglus commented Jul 21, 2017

achow101 commented Jul 21, 2017

jameshilliard commented Aug 9, 2017

jonasschnelli commented May 24, 2017 •

edited

Loading

jtimon May 31, 2017 •

edited

Loading

earonesty commented Jun 19, 2017 •

edited

Loading