BIP9 versionbits softfork for BIP68, BIP112 and BIP113 #7648

btcdrak · 2016-03-06T16:07:04Z

General

This PR softforks 3 lock-time related BIPs which are all implemented in master as mempool-only logic at the moment.

BIP68 sequence locks for relative locktime;
BIP112 CHECKSEQUENCEVERIFY;
BIP113 Median Time Past.

Dependencies

BIP68 (sequence locks) independently enforces the same semantics BIP113 (MTP)
BIP112 (CSV) relies on BIP68

Relay policy for BIP113

BIP113 mempool-only policy was deployed with Bitcoin Core 0.11.2 at the same time as the BIP65 CLTV softfork so the policy is in wide use (at least 70% of nodes) as well as all the miners who upgraded to 0.11.2.

Relay policy for v2 transactions

BIP68/112 rely on v2 transactions. Currently only v1 transactions are relayed, so it is necessary to change the relay policy to allow v2. This will be done at the same time as softfork deployment and will have the net effect that once the softfork enforces, we can be pretty sure miners will mine v2 transactions.

At a later date once enough nodes upgrade and we're sure v2 transaction will be relayed efficiently, we can bump the default transaction version in core see #7562.

This commit introduces a way to gracefully bump the default transaction version in a two step process.

This RPC test will test both the activation mechanism of the first versionbits soft fork as well as testing many code branches of the consensus logic for BIP's 68, 112, and 113.

btcdrak · 2016-03-18T08:49:02Z

Rebase now that #7575 has been merged.

afk11 · 2016-03-18T18:38:26Z

To aid review, here are the pull requests for each BIP:

#7184 - Sequence locks
#7524 - CSV
#6566 - Median time past

morcos · 2016-03-18T19:01:10Z

ACK 19d73d5

NicolasDorier · 2016-03-19T15:59:12Z

Can you add my commit, btcdrak#8 the current test does not cover exactly the activation part, I'm not comfortable with it.

My test can be used for testing segwit activation later also in few lines.

NicolasDorier · 2016-03-21T10:17:22Z

tACK 71527a0

instagibbs · 2016-03-22T14:57:54Z

utACK 71527a0

jl2012 · 2016-03-23T14:23:05Z

tACK 71527a0

sipa · 2016-03-23T14:36:24Z

Concept ACK, utACK code changes; I only glanced over the test code

laanwj · 2016-03-23T16:52:25Z

nit: The new RPC test is very noisy. Printing lines for passing tests is not necessary, at least by default, could be behind some verbose flag:

Test 1: PASS [143]
Test 2: PASS [287]
...
Test 124: PASS [582]
Test 125: PASS [582]

morcos · 2016-03-23T17:19:47Z

@laanwj yeah, I noticed that too. Unfortunately, that's the underlying behavior of the ComparisonTestFramework that the RPC test is built off of. I agree it would be nice to make optional.

EDIT: I suppose it would be easy to just always surpress the PASS output, and still output on failures.

afk11 · 2016-03-23T20:38:21Z

ACK

CodeShark · 2016-03-25T05:52:50Z

ACK code changes, ran tests but did not review test code.

jonasschnelli · 2016-03-29T07:50:02Z

qa/pull-tester/rpc-tests.py


 #Tests
 testScripts = [
+    'bip68-112-113-p2p.py',


Shouldn't this be an "extended test"?

no, this is a softfork and certainly until there is activation, I want to test for regressions as a standard part of the CI process.

paveljanik · 2016-03-29T08:10:42Z

ACK 71527a0

jonasschnelli · 2016-03-29T08:18:55Z

src/consensus/params.h


 enum DeploymentPos
 {
    DEPLOYMENT_TESTDUMMY,


nit: probably to late now and kinda-unrelated to this PR, but instead of TESTDUMMY we could have used something like DEFAULT or DEFAULTVOID.

Not added in PR though. That was #7575. I think the name is sufficient though, to indicate it's not a real softfork, but for testing purposes.

Fair enough.

jonasschnelli · 2016-03-29T08:51:57Z

Tested ACK 71527a0

jtimon · 2016-03-29T12:30:28Z

utACK (although I didn't look at the rpc tests in depth ).

sdaftuar · 2016-03-29T16:48:36Z

I saw there was some discussion of this already (btcdrak#8), but I don't really see the point of the bip9-softforks.py test -- it seems like a subset of the testing done in bip68-112-113-p2p.py. I don't feel strongly if you want to include it here, in case it helps anyone's review, but I thought I'd mention that this seems like a candidate for removal from the repo in the future.

Also perhaps we should remove the code in bip68-sequence.py which tests that BIP68 is not consensus (

bitcoin/qa/rpc-tests/bip68-sequence.py

Line 337 in 5131005

def test_bip68_not_consensus(self):

)? That test will continue to pass for the time being, because not enough blocks are generated to trigger the soft fork, but it's somewhat confusing to keep that test in the repo, and at any rate the RPC test bip68-112-113-p2p.py is comprehensively testing that BIP68 activates in the right way.

sdaftuar · 2016-03-29T16:51:36Z

qa/rpc-tests/bip68-112-113-p2p.py

+                return row
+        raise IndexError ('key:"%s" not found' % key)
+
+    def create_bip68txs(self, bip68inputs, txversion, locktime_delta = 0):


nit: locktime_delta appears to be unused in this function

NicolasDorier · 2016-03-29T17:12:26Z

@sdaftuar as explained in btcdrak#8 the test has a different purpose than bip68-112-113-p2p.py.

All bip are generally tested in their own PR, bip68-112-113-p2p.py is doing test that are already done but also testing the sf activation logic. This expand way more than the subject of this PR.

softforks.py has a different purpose, it is here to test ONLY the sf activation logic. This basically mean that in future softfork, you can test the activation correctly by adding a single line at https://github.com/bitcoin/bitcoin/pull/7648/files#diff-98a8abf7f80dbe5eda93bbbbb4348e80R190 .

For example, testing any new segwit sf activation will only be a matter of adding a function which change the scriptPubKey to be push.

The test is meant to make testing sf soft fork logic activation of future sf a breeze.

btcdrak · 2016-03-29T17:20:00Z

bip68-sequence.py is a valid part of the mempool-only test suite and should remain in the same way we have bip65 mempool tests. In any case, it's out of scope for this PR.

sdaftuar · 2016-03-29T17:20:53Z

qa/rpc-tests/bip9-softforks.py

+        assert_equal(self.get_bip9_status(bipName)['status'], 'locked_in')
+
+        # Test 5
+        # Check that the new rule is enforced


nit: I think this comment should say # Check that the new rule is not enforced ?

sdaftuar · 2016-03-29T17:28:11Z

I left some comment nits in the RPC test, bip68-112-113-p2p.py, which I reviewed in depth in addition to the rest of the code. That test is pretty comprehensive, and I think that test coverage had been missing before, so thanks for including that in this pull.

I also manually tested the case that is mentioned as being missing from bip68-112-113-p2p.py, for an OP_CSV with an empty stack.

ACK 71527a0

@btcdrak To be clear, I'm not talking about removing the entire bip68-sequence.py test, just comment out the specific test within that one which is checking that BIP68 is not enforced as a consensus rule. That test made sense when we were deploying BIP68 as mempool-only, but doesn't make sense when we're proposing it as a soft fork. See the line of code I linked to above.

Roasbeef · 2016-03-29T18:53:53Z

tACK 71527a0

laanwj · 2016-03-30T08:47:56Z

it's out of scope for this PR.

Issues with the tests, unless they pinpoint issues in the code, or break Travis, should not hold up this pull. They can be fixed later.

laanwj · 2016-03-30T11:36:26Z

@petertodd You said in #7184 (comment) that you wanted some nits addressed before this (the BIP68 part) would be acceptable as a softfork to you. Could you take a look whether this is the case now?

petertodd · 2016-03-30T12:08:22Z

@laanwj Those nits got fixed.

utACK 71527a0

laanwj · 2016-03-30T17:00:42Z

utACK 71527a0

71527a0 Test of BIP9 fork activation of mtp, csv, sequence_lock (NicolasDorier) 19d73d5 Add RPC test for BIP 68/112/113 soft fork. (Alex Morcos) 12c89c9 Policy: allow transaction version 2 relay policy. (BtcDrak) 02c2435 Soft fork logic for BIP68 (BtcDrak) 478fba6 Soft fork logic for BIP113 (BtcDrak) 65751a3 Add CHECKSEQUENCEVERIFY softfork through BIP9 (Pieter Wuille)

…ftfork for BIP68, BIP112 and BIP113'

jonasschnelli added the Consensus label Mar 6, 2016

btcdrak force-pushed the vb_68_112_113_1 branch from dc0f10a to e99ce43 Compare March 15, 2016 08:47

btcdrak mentioned this pull request Mar 15, 2016

Minimal BIP9 implementation #7575

Merged

btcdrak force-pushed the vb_68_112_113_1 branch 7 times, most recently from c9b879c to 89acee3 Compare March 17, 2016 21:00

sipa and others added 5 commits March 18, 2016 08:09

Add CHECKSEQUENCEVERIFY softfork through BIP9

65751a3

Soft fork logic for BIP113

478fba6

Soft fork logic for BIP68

02c2435

Policy: allow transaction version 2 relay policy.

12c89c9

This commit introduces a way to gracefully bump the default transaction version in a two step process.

Add RPC test for BIP 68/112/113 soft fork.

19d73d5

This RPC test will test both the activation mechanism of the first versionbits soft fork as well as testing many code branches of the consensus logic for BIP's 68, 112, and 113.

btcdrak force-pushed the vb_68_112_113_1 branch from 89acee3 to 19d73d5 Compare March 18, 2016 08:09

btcdrak mentioned this pull request Mar 18, 2016

[0.12] Backport BIP9, BIP68 and BIP112 with softfork #7543

Merged

morcos mentioned this pull request Mar 18, 2016

[0.11] Backport BIP9 and softfork for BIP's 68,112,113 #7716

Closed

NicolasDorier mentioned this pull request Mar 20, 2016

Test of BIP9 fork activation of mtp, csv, sequence_lock btcdrak/bitcoin#8

Closed

Test of BIP9 fork activation of mtp, csv, sequence_lock

71527a0

jonasschnelli reviewed Mar 29, 2016
View reviewed changes

sdaftuar reviewed Mar 29, 2016
View reviewed changes

laanwj merged commit 71527a0 into bitcoin:master Mar 30, 2016

vlamer added a commit to vlamer/bitcoin that referenced this pull request Mar 30, 2016

Merge remote-tracking branch 'Merge bitcoin#7648: BIP9 versionbits so…

3cd266a

…ftfork for BIP68, BIP112 and BIP113'

btcdrak mentioned this pull request Mar 30, 2016

Fix comments in tests #7773

Merged

jnewbery mentioned this pull request Feb 11, 2017

Fix BIP68 activation test #9739

Merged

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

BIP9 versionbits softfork for BIP68, BIP112 and BIP113 #7648

BIP9 versionbits softfork for BIP68, BIP112 and BIP113 #7648

Uh oh!

Conversation

btcdrak commented Mar 6, 2016

General

Dependencies

Relay policy for BIP113

Relay policy for v2 transactions

Uh oh!

btcdrak commented Mar 18, 2016

Uh oh!

afk11 commented Mar 18, 2016

Uh oh!

morcos commented Mar 18, 2016

Uh oh!

NicolasDorier commented Mar 19, 2016

Uh oh!

NicolasDorier commented Mar 21, 2016

Uh oh!

instagibbs commented Mar 22, 2016

Uh oh!

jl2012 commented Mar 23, 2016

Uh oh!

sipa commented Mar 23, 2016

Uh oh!

laanwj commented Mar 23, 2016

Uh oh!

morcos commented Mar 23, 2016

Uh oh!

afk11 commented Mar 23, 2016

Uh oh!

CodeShark commented Mar 25, 2016

Uh oh!

jonasschnelli Mar 29, 2016

Choose a reason for hiding this comment

Uh oh!

btcdrak Mar 29, 2016

Choose a reason for hiding this comment

Uh oh!

paveljanik commented Mar 29, 2016

Uh oh!

jonasschnelli Mar 29, 2016

Choose a reason for hiding this comment

Uh oh!

btcdrak Mar 29, 2016

Choose a reason for hiding this comment

Uh oh!

jonasschnelli Mar 29, 2016

Choose a reason for hiding this comment

Uh oh!

jonasschnelli commented Mar 29, 2016

Uh oh!

jtimon commented Mar 29, 2016

Uh oh!

sdaftuar commented Mar 29, 2016

Uh oh!

sdaftuar Mar 29, 2016

Choose a reason for hiding this comment

Uh oh!

NicolasDorier commented Mar 29, 2016

Uh oh!

btcdrak commented Mar 29, 2016

Uh oh!

sdaftuar Mar 29, 2016

Choose a reason for hiding this comment

Uh oh!

sdaftuar commented Mar 29, 2016

Uh oh!

Roasbeef commented Mar 29, 2016

Uh oh!

laanwj commented Mar 30, 2016

Uh oh!

laanwj commented Mar 30, 2016

Uh oh!

petertodd commented Mar 30, 2016

Uh oh!

laanwj commented Mar 30, 2016

Uh oh!

Reviewers