[0.14] Enforce segsignal activation height and rules #10900

pull achow101 wants to merge 2 commits into bitcoin:0.14 from achow101:early-uasf-bip91 changing 5 files +85 −1

achow101 commented at 10:34 PM on July 21, 2017: member

This PR adds a segsignal compatible rule that all blocks after block 477120 (segsignal's activation height) must signal for segwit so long as segwit is still in the started state. This will make sure that miners are enforcing the rules that they have chosen to activate.

This PR is on the 0.14 branch so that a 0.14.3 release can be quickly made which includes this change. It can also be easily rebased onto master if needed.
fanquake added this to the milestone 0.14.3 on Jul 21, 2017
fanquake added the label Validation on Jul 21, 2017
morcos commented at 11:33 PM on July 21, 2017: member

Strong Concept ACK

However I don't think this should be built on the 0.14 branch but directly on 0.14.2 I don't think we want any of the other backports. If we merge this, there will be no time for significant testing and we want the decision on whether to run this or keep running 0.14.2 to be based solely on a user's opinion on whether BIP91 represents the new consensus rules.

I'd advocate for releasing this as perhaps 0.14.2-segsignal or something else outside of the usual version numbering scheme.

In future versions, I think we should be able to drop all this transient BIP91 nonsense. This code/patch is hopefully only necessary to protect users for the next 3 weeks until BIP 141 (segwit) activates.
Segsignal compatible UASF 8651c4ef94
Add test for segsignal UASF b6abab6533
achow101 force-pushed on Jul 21, 2017
achow101 commented at 11:59 PM on July 21, 2017: member

@morcos I have rebased it so that it is just a patch on top of 0.14.2. Of course that means that a separate branch based on 0.14.2 will need to be made for this to be merged and released as a 0.14.2 change.
in qa/rpc-tests/segsignal.py:48 in b6abab6533
```
  43 | +        self.generate_blocks(1, 0x20000002)
  44 | +        print(self.nodes[0].getblockchaininfo())
  45 | +        assert_equal(self.get_bip9_status('segwit')['status'], 'locked_in')
  46 | +
  47 | +        # Mine a non-segwit block, should be accepted
  48 | +        self.generate_blocks(1, 0x20000000)
```
jimmysong commented at 1:11 AM on July 22, 2017:

Not sure if this is a test case we necessarily care about, but maybe generate another 143 blocks and check that a non-segwit-signaling block is still accepted?

achow101 commented at 2:10 AM on July 22, 2017:

I don't think that test case matters as it is already covered by this one here.

jimmysong commented at 2:14 AM on July 22, 2017:

Wouldn't the bip9 status be "active"? Just suggesting you check behavior in that last state for completeness.
jimmysong commented at 1:37 AM on July 22, 2017: contributor

Tested ACK

Thank you for getting this change in so quickly. Just one minor point on the rpc test.
h0jeZvgoxFepBQ2C commented at 1:53 AM on July 22, 2017: none

utAck - yes please, merge BIP148 into core! 🎉 😄

in src/validation.cpp:1856 in b6abab6533

1850 | @@ -1851,6 +1851,17 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
1851 |          flags |= SCRIPT_VERIFY_NULLDUMMY;
1852 |      }
1853 |  
1854 | +    // SEGSIGNAL mandatory segwit signalling.
1855 | +    if (pindex->nHeight >= chainparams.GetConsensus().BIP91Height &&
1856 | +        VersionBitsState(pindex->pprev, chainparams.GetConsensus(), Consensus::DEPLOYMENT_SEGWIT,    versionbitscache) == THRESHOLD_STARTED)

jamesob commented at 2:13 AM on July 22, 2017:

nit: whitespace before last param

luke-jr commented at 3:22 AM on July 22, 2017: member

Concept ACK, agree with @morcos on a new branch for 0.14.2.1 or such (we've used a fourth number for this kind of purpose before). Code looks okay too, but I didn't test (utACK).
andyrowe commented at 4:28 AM on July 22, 2017: none

NACK. I think it's absurdly late in the game to be attempting to throw together a minimally tested segwit2x compatibility fix given the current state of the network.
petertodd commented at 5:08 AM on July 22, 2017: contributor

Concept ACK @morcos's suggestions.

Similar situation w/ the previous BIP148 pull-req: Core users have good reason to want this rule, but whether or not it represents Bitcoin is still undecided. Unlike the BIP148 situation, we're incredibly short on time, so a non-standard version like -segsignal or .1 (@luke-jr's suggestion) makes sense.
tmornini commented at 5:28 AM on July 22, 2017: none

I'd encourage naming the release 0.14.2-segsignal rather than 0.14.2.1.

Rational: There's nothing in 0.14.2.1 to indicate that it's NOT a bug fix of 0.14.2.

Consider if a decision is made to release a BIP148 patch against 0.14.2. What might it be called? 0.14.2-BIP148 is perfectly clear, whereas 0.14.2.2 could easily be a follow-on release to 0.14.2.1.

Avoid confusion at all costs, and always call a thing a thing. :-)
felipelalli commented at 6:04 AM on July 22, 2017: none

ACK right now. Agreed with @tmornini observation.
jameshilliard commented at 10:17 AM on July 22, 2017: contributor

utAck
luke-jr commented at 10:41 AM on July 22, 2017: member

@tmornini Softforks are always deployed as a minor release. In ordinary circumstances, BIP148 or this would be 0.14.3. The only reason this can't really be that, is because we don't have time to do a full RC cycle to test the other fixes (which are being skipped). Historically such time-sensitive releases have used a fourth number, hence 0.14.2.1.
gomedera commented at 12:22 PM on July 22, 2017: none

This change could be released as 14.3 and whatever 14.3 meant to be could be released as 14.4. Problem solved.
earonesty commented at 6:44 AM on July 24, 2017: none

It's too late now, but any consensus change where it may be possible that the change is unsafe today, but may be safer in the future should be released as a user configurable option early on, not patched in at the last minute.
kallewoof commented at 3:12 PM on July 24, 2017: member

utACK b6abab653300da5152d9d9cfa5f8582e0b4b85d4
Leviathn commented at 10:05 PM on July 31, 2017: none

Concept ACK in line with @morcos @luke-jr & @petertodd
achow101 commented at 5:53 PM on August 9, 2017: member

BIP 91 is no longer active now that segwit has locked in, so I am closing this.
achow101 closed this on Aug 9, 2017
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Milestone
0.14.3