BIP9 versionbits softfork for BIP68, BIP112 and BIP113 #7648

pull btcdrak wants to merge 6 commits into bitcoin:master from btcdrak:vb_68_112_113_1 changing 10 files +816 −7
  1. btcdrak commented at 4:07 pm on March 6, 2016: contributor

    General

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

    1. BIP68 sequence locks for relative locktime;
    2. BIP112 CHECKSEQUENCEVERIFY;
    3. 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.

  2. jonasschnelli added the label Consensus on Mar 6, 2016
  3. btcdrak force-pushed on Mar 15, 2016
  4. btcdrak force-pushed on Mar 15, 2016
  5. btcdrak force-pushed on Mar 15, 2016
  6. btcdrak force-pushed on Mar 15, 2016
  7. btcdrak force-pushed on Mar 16, 2016
  8. btcdrak force-pushed on Mar 16, 2016
  9. btcdrak force-pushed on Mar 16, 2016
  10. btcdrak force-pushed on Mar 17, 2016
  11. Add CHECKSEQUENCEVERIFY softfork through BIP9 65751a3cf2
  12. Soft fork logic for BIP113 478fba6d52
  13. Soft fork logic for BIP68 02c2435802
  14. Policy: allow transaction version 2 relay policy. 
    This commit introduces a way to gracefully bump the default
transaction version in a two step process.
    12c89c9185
  15. Add RPC test for BIP 68/112/113 soft fork. 
    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.
    19d73d540c
  16. btcdrak force-pushed on Mar 18, 2016
  17. btcdrak commented at 8:49 am on March 18, 2016: contributor
    Rebase now that #7575 has been merged.
  18. afk11 commented at 6:38 pm on March 18, 2016: contributor

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

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

  19. morcos commented at 7:01 pm on March 18, 2016: member
    ACK 19d73d5
  20. NicolasDorier commented at 3:59 pm on March 19, 2016: contributor

    Can you add my commit, https://github.com/btcdrak/bitcoin/pull/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.

  21. Test of BIP9 fork activation of mtp, csv, sequence_lock 71527a0f31
  22. NicolasDorier commented at 10:17 am on March 21, 2016: contributor
    tACK 71527a0
  24. jl2012 commented at 2:23 pm on March 23, 2016: contributor
    tACK 71527a0
  25. sipa commented at 2:36 pm on March 23, 2016: member
    Concept ACK, utACK code changes; I only glanced over the test code
  26. laanwj commented at 4:52 pm on March 23, 2016: member

    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:

    0Test 1: PASS [143]
1Test 2: PASS [287]
2...
3Test 124: PASS [582]
4Test 125: PASS [582]
  27. morcos commented at 5:19 pm on March 23, 2016: member

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

  28. btcdrak commented at 5:27 pm on March 23, 2016: contributor
    @laanwj I also agree, but it’s orthogonal to this PR imo. If you don’t mind, I’d like to push back on quieting tests for this PR. Ideally it would be much better to print a dot per test on the same line and only emit error messages if necessary. But that would require a change to the ComparisonTestFramework.
  29. laanwj commented at 5:34 pm on March 23, 2016: member
    @btcdrak @morcos Sure, I’m fine with keeping it this way in this pull.
  30. afk11 commented at 8:38 pm on March 23, 2016: contributor
    ACK
  31. CodeShark commented at 5:52 am on March 25, 2016: contributor
    ACK code changes, ran tests but did not review test code.
  32. in qa/pull-tester/rpc-tests.py: in 71527a0f31 
    82@@ -83,6 +83,7 @@
83 
84 #Tests
85 testScripts = [
86+    'bip68-112-113-p2p.py',
    jonasschnelli commented at 7:50 am on March 29, 2016:
    Shouldn’t this be an “extended test”?
    btcdrak commented at 8:20 am on March 29, 2016:
    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.
  33. in qa/pull-tester/rpc-tests.py: in 71527a0f31 
    118@@ -118,6 +119,7 @@
119     'p2p-versionbits-warning.py',
120 ]
121 testScriptsExt = [
122+    'bip9-softforks.py',
    jonasschnelli commented at 7:54 am on March 29, 2016:
    IMO: bip9 itself is not a softfork and could therefore be in the “normal” testScripts array.
    btcdrak commented at 8:21 am on March 29, 2016:
    It’s testing the activation logic of bip9 softforks.
  35. in src/consensus/params.h: in 71527a0f31 
    14@@ -15,6 +15,7 @@ namespace Consensus {
15 enum DeploymentPos
16 {
17     DEPLOYMENT_TESTDUMMY,
    jonasschnelli commented at 8:18 am on March 29, 2016:
    nit: probably to late now and kinda-unrelated to this PR, but instead of TESTDUMMY we could have used something like DEFAULT or DEFAULTVOID.
    btcdrak commented at 8:22 am on March 29, 2016:
    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.
    jonasschnelli commented at 8:24 am on March 29, 2016:
    Fair enough.
  36. jonasschnelli commented at 8:51 am on March 29, 2016: contributor
    Tested ACK 71527a0f31ae67edad0a7fcda59c75a6ce5666ca
  37. jtimon commented at 12:30 pm on March 29, 2016: contributor
    utACK (although I didn’t look at the rpc tests in depth ).
  38. sdaftuar commented at 4:48 pm on March 29, 2016: member

    I saw there was some discussion of this already (https://github.com/btcdrak/bitcoin/pull/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 (https://github.com/bitcoin/bitcoin/blob/5131005e5b26d12b5b3f79c1c3f8ee08172fc386/qa/rpc-tests/bip68-sequence.py#L337)? 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.

  39. in qa/rpc-tests/bip68-112-113-p2p.py: in 71527a0f31 
    154+        for row in info['bip9_softforks']:
155+            if row['id'] == key:
156+                return row
157+        raise IndexError ('key:"%s" not found' % key)
158+
159+    def create_bip68txs(self, bip68inputs, txversion, locktime_delta = 0):
    sdaftuar commented at 4:51 pm on March 29, 2016:
    nit: locktime_delta appears to be unused in this function
  40. in qa/rpc-tests/bip68-112-113-p2p.py: in 71527a0f31 
    383+        for bip113tx in [bip113signed1, bip113signed2]:
384+            yield TestInstance([[self.create_test_block([bip113tx]), False]]) # 9,10
385+        # BIP 113 tests should now pass if the locktime is < MTP
386+        bip113tx_v1.nLockTime = self.last_block_time - 600 * 5 - 1 # = MTP of prior block (not <) but < time put on current block
387+        bip113signed1 = self.sign_transaction(self.nodes[0], bip113tx_v1)
388+        bip113tx_v2.nLockTime = self.last_block_time - 600 * 5 - 1 # = MTP of prior block (not <) but < time put on current block
    sdaftuar commented at 4:53 pm on March 29, 2016:
    nit: The comments at lines 386 and 388 appear to be incorrect. Perhaps # < MTP of prior block?
  41. in qa/rpc-tests/bip68-112-113-p2p.py: in 71527a0f31 
    488+
489+        yield TestInstance([[self.create_test_block(success_txs), True]]) # 83
490+        self.nodes[0].invalidateblock(self.nodes[0].getbestblockhash())
491+
492+        ## SEQUENCE_LOCKTIME_DISABLE_FLAG is unset in argument to OP_CSV for all remaining txs ##
493+        # All txs with nSequence 11 should fail either due to earlier mismatch or failing the CSV check
    sdaftuar commented at 4:56 pm on March 29, 2016:
    nit: I think this comment should be ... nSequence 9 ..., rather than nSequence 11?
  42. in qa/rpc-tests/bip68-112-113-p2p.py: in 71527a0f31 
    494+        fail_txs = []
495+        fail_txs.extend(all_rlt_txs(bip112txs_vary_nSequence_9_v2)) # 16/16 of vary_nSequence_9
496+        for b25 in xrange(2):
497+            for b22 in xrange(2):
498+                for b18 in xrange(2):
499+                    fail_txs.append(bip112txs_vary_OP_CSV_9_v2[0][b25][b22][b18]) # 16/16 of vary_OP_CSV_9
    sdaftuar commented at 4:56 pm on March 29, 2016:
    nit: I think this comment is off, should be # 8/16 of vary_OP_CSV_9?
  43. in qa/rpc-tests/bip68-112-113-p2p.py: in 71527a0f31 
    513+        # If sequencelock types mismatch, tx should fail
514+        fail_txs = []
515+        for b25 in xrange(2):
516+            for b18 in xrange(2):
517+                fail_txs.append(bip112txs_vary_nSequence_v2[0][b25][1][b18]) # 12/16 of vary_nSequence
518+                fail_txs.append(bip112txs_vary_OP_CSV_v2[0][b25][1][b18]) # 12/16 of vary_OP_CSV
    sdaftuar commented at 4:58 pm on March 29, 2016:
    nit: I think these two comments should say # 4/16 ...
  44. in qa/rpc-tests/bip68-112-113-p2p.py: in 71527a0f31 
    522+        # Remaining txs should pass, just test masking works properly
523+        success_txs = []
524+        for b25 in xrange(2):
525+            for b18 in xrange(2):
526+                success_txs.append(bip112txs_vary_nSequence_v2[0][b25][0][b18]) # 16/16 of vary_nSequence
527+                success_txs.append(bip112txs_vary_OP_CSV_v2[0][b25][0][b18]) # 16/16 of vary_OP_CSV
    sdaftuar commented at 4:59 pm on March 29, 2016:
    nit: I think these two should also say # 4/16 ...
  45. NicolasDorier commented at 5:12 pm on March 29, 2016: contributor

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

  46. btcdrak commented at 5:20 pm on March 29, 2016: contributor
    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.
  47. in qa/rpc-tests/bip9-softforks.py: in 71527a0f31 
    130+        yield TestInstance(test_blocks, sync_every_block=False)
131+
132+        assert_equal(self.get_bip9_status(bipName)['status'], 'locked_in')
133+
134+        # Test 5
135+        # Check that the new rule is enforced
    sdaftuar commented at 5:20 pm on March 29, 2016:
    nit: I think this comment should say # Check that the new rule is not enforced ?
  48. sdaftuar commented at 5:28 pm on March 29, 2016: member

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

  50. laanwj commented at 8:47 am on March 30, 2016: member

    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.

  51. laanwj commented at 11:36 am on March 30, 2016: member
    @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?
  52. petertodd commented at 12:08 pm on March 30, 2016: contributor

    @laanwj Those nits got fixed.

    utACK https://github.com/bitcoin/bitcoin/pull/7648/commits/71527a0f31ae67edad0a7fcda59c75a6ce5666ca

  53. laanwj commented at 5:00 pm on March 30, 2016: member
    utACK 71527a0
  54. laanwj merged this on Mar 30, 2016
  55. laanwj closed this on Mar 30, 2016
  56. laanwj referenced this in commit e8a8f3d4b2 on Mar 30, 2016
  57. vlamer referenced this in commit 3cd266a181 on Mar 30, 2016
  58. MarcoFalke locked this on Sep 8, 2021


btcdrak afk11 morcos NicolasDorier instagibbs jl2012 sipa laanwj CodeShark jonasschnelli paveljanik jtimon sdaftuar Roasbeef petertodd

Labels
Consensus

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 21:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me