IsSuperMajority() softfork for BIPs 68,112 and 113 #7561

pull btcdrak wants to merge 8 commits into bitcoin:master from btcdrak:softfork changing 11 files +683 −9
  1. btcdrak commented at 3:16 pm on February 19, 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 Media Time Past.

    Dependencies

    Why 3 BIPs at once?

    • 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 42% 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.

    Why ISM()?

    It seems unlikely that BIP9 will be both developed and well tested enough to meet the roadmap schedule of deployment even with reasonable slippage taken into account. ISM is well oiled and tested.

    Todos

    • Change relay policy to allow tx.version=2
    • fix bip68-sequence-p2p.py tests
    • fix bip112-csv-p2p.py tests

    Misc

    This softfork could also include SegWit BIP141.

    Obviously, needs back-port to 0.12 at least.

  2. jonasschnelli added the label Consensus on Feb 19, 2016
  3. btcdrak force-pushed on Feb 19, 2016
  4. NicolasDorier commented at 4:07 pm on February 19, 2016: contributor
    Tested ACK 1fc8385
  5. maaku commented at 5:15 pm on February 19, 2016: contributor
    Sticklers for process are going to tell you that you should remove the BIP 113 soft-fork as such transactions are only just now getting marked as non-standard in the 0.12 release. By following this logic the earliest a 113 soft-fork could be applied is after the 0.13 release, at which point in time 0.12 will be the oldest supported version.
  6. btcdrak commented at 5:19 pm on February 19, 2016: contributor
    @maaku BIP113 mempool policy was deployed since 0.11.2 with the CLTV softfork, so the earliest available softfork is in fact 0.12.1. Also note that BIP68 requires BIP113 so all these BIPs must be enforced at once.
  7. btcdrak force-pushed on Feb 19, 2016
  8. btcdrak force-pushed on Feb 19, 2016
  9. morcos commented at 8:02 pm on February 19, 2016: member

    a couple comments/questions about the rpc tests:

    • blockversion5-ism.py seems extraneous to me. We already have prior versions of ism tests and in particular the p2p tests added here are testing the ISM logic for version 5 blocks already.
    • For BIP 68 we have the existing bip68-sequence.py which tests the logic of the code in a mempool only setting. I think we need to decide whether that is sufficient when combined with a simple test that the activation logic is working in the bip68 test added here. But for BIP 112 and 113 it seems we are still missing any tests that exercise the logic, other than the one simple case used to test activation in the rpc tests added here.
    • the RPC tests could have clearer names, for instance the existing bip68 rpc test is also a p2p test, so the distinction in the names is confusing, perhaps bip68-sequence-activation.py
  10. Policy: allow transaction version 2 relay policy. 
    This commit introduces a way to gracefully bump the default
transaction version in a two step process.
    957d59043b
  11. in qa/rpc-tests/mempool_reorg.py: in 1fc83850c1 outdated 
    58@@ -59,7 +59,8 @@ def run_test(self):
59         # Create a block-height-locked transaction which will be invalid after reorg
60         timelock_tx = self.nodes[0].createrawtransaction([{"txid": coinbase_txids[0], "vout": 0}], {node0_address: 49.99})
61         # Set the time lock
62-        timelock_tx = timelock_tx.replace("ffffffff", "11111111", 1)
63+        # Disable SequenceLock, Enable nLockTime
64+        timelock_tx = timelock_tx.replace("ffffffff", "11111191", 1)
    morcos commented at 8:43 pm on February 19, 2016:
    in case it saves anyone else time maybe add: # set most signficant bit (little-endian) to activate LOCKTIME_DISABLE_FLAG
  12. morcos commented at 8:57 pm on February 19, 2016: member

    quick once over utACK on the code.

    although there is a comment in CheckFinalTx about a convention of negative flags meaning consensus enforced rules. I think that means you need to change the 0 in https://github.com/btcdrak/bitcoin/blob/1fc83850c1308d6f72f357980f66dc9b30f3a4ed/src/main.cpp#L692 to match the result of ISM flags. But I’m not sure I understand the reasoning for that or if it’s necessary.

    There does seem to be some missing logic around whether code here or in miner.cpp can actually somehow use STANDARD flags that accidentally don’t incorporate consensus flags. But that’s not a new problem to this PR.

  13. Soft fork logic for BIP112 7b1b9626b0
  14. Soft fork logic for BIP113 81d808a6bd
  15. RPC tests for BIP113 soft fork f4663dd259
  16. Soft fork logic for BIP68 fbc0eba361
  17. RPC tests for BIP68 soft fork 57a9ae2190
  18. RPC test for IsSuperMajority() upgrade blockversion >= 5 83e4522430
  19. btcdrak force-pushed on Feb 19, 2016
  20. btcdrak commented at 6:36 am on February 20, 2016: contributor
    I have updated the OP to explain the PR rationale in more detail to address common questions.
  21. morcos commented at 2:38 pm on February 20, 2016: member

    A couple of comments about your PR message:

    • BIP 68 does not depend on BIP 113. It independently enforces the same semantics, but BIP 113 only applies for lock time locks, not sequence locks.
    • Are you sure about bit masking version numbers for ISM? It seems to me like that is fraught with problems.
  22. btcdrak commented at 3:36 pm on February 20, 2016: contributor
    @morcos updated OP.
  23. RPC tests for BIP112 soft fork 3bd5c4439e
  24. btcdrak force-pushed on Feb 20, 2016
  25. NicolasDorier commented at 6:19 pm on February 20, 2016: contributor

    @morcos @btcdrak CheckFinalTx and removeForReorg always enforce MTP since 0.11.2. To reflect that, I just got rid of the flags https://github.com/NicolasDorier/bitcoin/commit/2197781dbe4b64bd7a29902f30eb7eb9c259119f

    The flags came from initial old design of CSV BIP112 which had different cutoff depending of MTP activation. I don’t know if it is better to bundle my commit on this PR or do a separate one, please advise.

  26. btcdrak force-pushed on Feb 20, 2016
  27. jmcorgan commented at 6:47 pm on February 20, 2016: contributor
    Initial read through utACK 3bd5c44. However the additional changes by @NicolasDorier seem unrelated enough to enabling the soft fork logic that I think it would be clearer to have it in a separate PR.
  28. maaku commented at 8:38 pm on February 20, 2016: contributor

    @btcdrak BIP 68 doesn’t require or presume BIP 113 – at least not in the original implementation. It must necessarily use MTP as the start point, but the end point is either nTime or MTP based on activation of BIP 113.

    In any case this is a call for @laanwj and @gmaxwell .

    EDIT: To be clear I’ve advocated in the past for more aggressive rollout of BIP 113 and the response I got was that it must be declared non-standard first and then wait for a period of time…

  29. morcos commented at 9:39 pm on February 20, 2016: member
    @NicolasDorier I’m sympathetic to trying to remove these flags, but I don’t know if should be trying to confuse things by doing it now. The question is do we want to continue semi-supporting a mempool policy that doesn’t enforce MedianTimePast. @maaku BIP 68 was modified to use MTP semantics for both end points already. There was discussion about it a couple of months ago and universal agreement.
  30. btcdrak force-pushed on Feb 21, 2016
  31. NicolasDorier commented at 10:20 pm on February 21, 2016: contributor
    @morcos, I have opened a separate PR for removing the flags in #7574
  32. in src/main.cpp: in 3bd5c4439e 
    2175@@ -2176,6 +2176,14 @@ bool ConnectBlock(const CBlock& block, CValidationState& state, CBlockIndex* pin
2176         flags |= SCRIPT_VERIFY_CHECKLOCKTIMEVERIFY;
2177     }
2178 
2179+    // Start enforcing BIP68 (sequence locks) and CHECKSEQUENCEVERIFY (BIP112) for block.nVersion=5
2180+    // blocks, when 75% of the network has upgraded:
2181+    int nLockTimeFlags = 0;
2182+    if (block.nVersion >= 5 && IsSuperMajority(5, pindex->pprev, chainparams.GetConsensus().nMajorityEnforceBlockUpgrade, chainparams.GetConsensus())) {
    jtoomim commented at 3:13 am on March 5, 2016:

    This should be something like

    0if ((block.nVersion & 15) >= 5 && IsSuperMajority... )

    In order to avoid misinterpreting Version Bits blocks as votes for BIP112. IsSuperMajority would need to be modified slightly as well, plus line 3180.

    Without this change, blocks voting for BIP109 would be incorrectly interpreted as voting for BIP112, which could cause a premature triggering of the BIP112 fork.

    With this change, Bitcoin Classic, XT, and Unlimited can use block versions like 0x300000005 to be able to vote for BIP109 and BIP112 simultaneously, or plain 0x30000000 or 0x300000004 to vote for BIP109 without BIP112.

    petertodd commented at 4:28 am on March 5, 2016:
    This has been discussed a number of times on the mailing list; what you propose is not a good idea. You can read more from the similar discussion on the mailing list back in Sept in the context of CLTV.
    chaosgrid commented at 5:49 pm on March 5, 2016:

    So what is your proposed fix? Or do you want to risk forking prematurely? You do realize this could put Core on a minority chain in the event that BIP 109 has 50+ % hashing power?

    Oh, and could you elaborate again why this is not a good idea? I did not find the relevant bit in the mailing list discussion.

    petertodd commented at 4:13 pm on March 7, 2016:
    We have a proposed fix - versionbits - as well as a backup fix - pseudo-versionbits.
    petertodd commented at 4:14 pm on March 7, 2016:
    We have a proposed fix - versionbits - as well as a backup fix - pseudo-versionbits.
  33. btcdrak commented at 6:53 pm on March 15, 2016: contributor
    Closing in favour of 7648
  34. btcdrak closed this on Mar 15, 2016
  35. btcdrak deleted the branch on Dec 3, 2016
  36. DrahtBot locked this on Sep 8, 2021


btcdrak NicolasDorier maaku morcos jmcorgan jtoomim petertodd chaosgrid

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-21 18:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me