BIP-113: Mempool-only median time-past as endpoint for lock-time calculations #6566

pull maaku wants to merge 2 commits into bitcoin:master from maaku:medianpasttimelock changing 6 files +58 −10
  1. maaku commented at 1:20 am on August 18, 2015: contributor

    The lock-time code currently uses CBlock::nTime as the cutoff point for time based locked transactions. This has the unfortunate outcome of creating a perverse incentive for miners to lie about the time of a block in order to collect more fees by including transactions that by wall clock determination have not yet matured. By using CBlockIndex::GetMedianTimePast from the prior block instead, the self-interested miner no longer gains from generating blocks with fraudulent timestamps. Users can compensate for this change by simply adding an hour (3600 seconds) to their time-based lock times.

    Transactions are not allowed in the memory pool or selected for inclusion in a block until their lock times exceed chainActive.Tip()->GetMedianTimePast(). However blocks including transactions which are only mature under the old rules are still accepted; this is not the soft-fork required to actually rely on the new constraint in production.

    Note: This pull request is NO LONGER dependent on #6312 or #6564. If this pull request is merged first, those PR’s will have to be updated to include median time-past codes for relative lock-time as well.

  2. maaku renamed this:
    Medianpasttimelock
    Mempool-only Median time-past as endpoint for lock-time calculations
    on Aug 18, 2015
  3. laanwj added the label Mempool on Aug 20, 2015
  4. laanwj added the label Consensus on Aug 20, 2015
  5. btcdrak commented at 10:52 pm on September 23, 2015: contributor
    needs rebase
  6. maaku renamed this:
    Mempool-only Median time-past as endpoint for lock-time calculations
    BIP-113: Mempool-only median time-past as endpoint for lock-time calculations
    on Sep 24, 2015
  7. maaku force-pushed on Sep 27, 2015
  8. maaku force-pushed on Sep 29, 2015
  9. maaku force-pushed on Oct 1, 2015
  10. maaku force-pushed on Oct 5, 2015
  11. maaku force-pushed on Oct 16, 2015
  12. maaku commented at 0:27 am on October 16, 2015: contributor
    This pull request has been updated to no longer be dependent on #6312 or #6564.
  13. dcousens commented at 0:33 am on October 16, 2015: contributor
    @maaku awesome.
  14. in src/main.cpp: in 6bd1f3c4d4 outdated 
    828+
829     // Only accept nLockTime-using transactions that can be mined in the next
830     // block; we don't want our mempool filled up with transactions that can't
831     // be mined yet.
832-    if (!CheckFinalTx(tx))
833+    if (!IsFinalTx(tx, chainActive.Height() + 1, nLockTimeCutoff))
    CodeShark commented at 2:21 am on October 19, 2015:
    Minor nit: Other than removing the assertion on cs_main, is there any advantage to calling IsFinalTx directly rather than CheckFinalTx? If that difference could be important I would suggest having two versions of CheckFinalTx, one that does not have the assertion and one that has the assertion and calls the other one, since the rest of the logic seems to just be getting duplicated.
    jtimon commented at 10:07 am on October 19, 2015:
    IsFinalTx is compatible with libconsensus, CheckFinalTx (by using globals) is not. Consensus functions cannot access globals like chainActive or GetAdjustedTime() directly.
    maaku commented at 6:02 pm on October 19, 2015:
    This isn’t consensus code though. @CodeShark is right, we can save some lines by calling CheckFinalTx with an explicit flags parameter. I’ve made the change.
  15. CodeShark commented at 2:23 am on October 19, 2015: contributor
    utACK
  16. in src/main.cpp: in 6bd1f3c4d4 outdated 
    2729+        int64_t nLockTimeCutoff = (nLockTimeFlags & LOCKTIME_MEDIAN_TIME_PAST)
2730+                                ? pindexPrev->GetMedianTimePast()
2731+                                : block.GetBlockTime();
2732+        if (!IsFinalTx(tx, nHeight, nLockTimeCutoff)) {
2733             return state.DoS(10, error("%s: contains a non-final transaction", __func__), REJECT_INVALID, "bad-txns-nonfinal");
2734         }
    btcdrak commented at 2:33 am on October 19, 2015:
    style nit: braces not required for this if
    maaku commented at 5:59 pm on October 19, 2015:
    Braces already existed. Principle of smallest diff :)
    laanwj commented at 9:45 am on October 22, 2015:

    There’s nothing about brace usage for single-statement blocks in the developer-guide, so let’s not nit about this. Also personally I like using braces everywhere where possible to avoid famous bugs like

    0if (something)
1    do_something_else();
2    goto handle_error;
    btcdrak commented at 9:54 am on October 22, 2015:
    OK, agreed.
  17. btcdrak commented at 9:04 am on October 19, 2015: contributor
    utACK
  18. maaku force-pushed on Oct 19, 2015
  19. maaku force-pushed on Oct 19, 2015
  20. maaku commented at 9:09 pm on October 19, 2015: contributor
    Nits addressed and rebased.
  21. btcdrak commented at 10:33 pm on October 19, 2015: contributor
    Looks like Travis randomly borked. Needs another force push.
  22. maaku force-pushed on Oct 19, 2015
  23. maaku force-pushed on Oct 19, 2015
  24. maaku force-pushed on Oct 19, 2015
  25. maaku commented at 0:32 am on October 20, 2015: contributor
    Travis is happy.
  26. dcousens commented at 10:16 am on October 20, 2015: contributor

    How does this mempool-only change stop the miners from just using their own mempool policies?

    edit: nvm

    This is not the soft-fork required to actually rely on the new constraint in production.

    concept ACK, though I’d prefer to see a plan for merging the soft-fork before accepting this.

  27. btcdrak commented at 11:20 am on October 20, 2015: contributor
    @dcousens These rules will be eventually enforced through a soft-fork.
  28. btcdrak commented at 1:03 pm on October 20, 2015: contributor
    @dcousens As discussed at the RC meeting on the 15th, we aim to roll this out with CLTV using standard ISM.
  29. rustyrussell commented at 0:03 am on October 21, 2015: contributor
    Ack. Please apply soon :)
  30. instagibbs commented at 0:49 am on October 21, 2015: member
    ACK.
  31. jmcorgan commented at 7:38 pm on October 21, 2015: contributor
    utACK
  32. afk11 commented at 10:53 pm on October 21, 2015: contributor
    ACK
  33. in src/consensus/consensus.h: in 3bd176969c outdated 
    12@@ -13,4 +13,10 @@ static const unsigned int MAX_BLOCK_SIGOPS = MAX_BLOCK_SIZE/50;
13 /** Coinbase transaction outputs can only be spent after this number of new blocks (network rule) */
14 static const int COINBASE_MATURITY = 100;
15 
16+/** Flags for LockTime() */
17+enum {
18+    /* Use GetMedianTimePast() instead of nTime for end point timestamp. */
19+    LOCKTIME_MEDIAN_TIME_PAST = (1 << 1),
    laanwj commented at 9:29 am on October 22, 2015:
    Why is this (1<<1) and not (1<<0) ? I don’t think it interacts with any other flags. Hard to see from the context.
    laanwj commented at 9:31 am on October 22, 2015:
    If the value comes from the BIP, please add a reference
    maaku commented at 7:50 pm on October 22, 2015:
    Because #6312, which this was originally built on top of, uses (1<<0).
  34. in src/main.h: in 3bd176969c outdated 
    304@@ -305,7 +305,7 @@ bool IsFinalTx(const CTransaction &tx, int nBlockHeight, int64_t nBlockTime);
305  *
306  * Calls IsFinalTx() with current block height and appropriate block time.
307  */
    laanwj commented at 9:34 am on October 22, 2015:
    Add a comment about the flags field, e.g. where they come from
  35. rubensayshi commented at 9:43 am on October 22, 2015: contributor
    utACK
  36. gmaxwell commented at 6:16 pm on October 22, 2015: contributor

    I think this code would be easier to be confident if it was (and would remain) consensus-correct if it introduced a second IsFinal() (or the like) and then used that explicitly from the mempool codepath. The max on the flags is a fairly weird operation.

    Concept ACK.

  37. petertodd commented at 7:40 pm on October 22, 2015: contributor
    utACK @gmaxwell’s nit doesn’t bother me, as the CheckFinalTx() function wasn’t supposed to be used in consensus code anyway.
  38. in src/consensus/consensus.h: in 6bd1f3c4d4 outdated 
    12@@ -13,4 +13,10 @@ static const unsigned int MAX_BLOCK_SIGOPS = MAX_BLOCK_SIZE/50;
13 /** Coinbase transaction outputs can only be spent after this number of new blocks (network rule) */
14 static const int COINBASE_MATURITY = 100;
15 
16+/** Flags for LockTime() */
17+enum {
18+    /* Use GetMedianTimePast() instead of nTime for end point timestamp. */
19+    LOCKTIME_MEDIAN_TIME_PAST = (1 << 1),
20+};
    jtimon commented at 7:41 pm on October 22, 2015:
    almost-bike-shedding: Are we going to have CheckFinalTx flags or something more generic like Transaction validation flags?
    maaku commented at 3:52 pm on October 23, 2015:
    These flags are passed to LockTime(). Adding a bunch of other flag definitions which are not used by LockTime() would only cause confusion, I think.
  39. jtimon commented at 7:44 pm on October 22, 2015: contributor
    utACK
  40. dcousens commented at 0:28 am on October 23, 2015: contributor
    Agreed with @gmaxwell, lets remove the flag and just have two functions. It would be trivially short anyway.
  41. in src/test/miner_tests.cpp: in 3bd176969c outdated 
    261@@ -261,7 +262,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
262     //BOOST_CHECK(CheckFinalTx(tx2));
263 
264     BOOST_CHECK(pblocktemplate = CreateNewBlock(scriptPubKey));
265-    BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 3);
266+    BOOST_CHECK_EQUAL(pblocktemplate->block.vtx.size(), 2);
    luke-jr commented at 2:30 am on October 23, 2015:

    I suggest this should check that the correct transaction was included.

    Preferably, the test should go on to produce a block that translates into the second transaction being included as well.

  42. in src/miner.cpp: in 3bd176969c outdated 
    162@@ -162,7 +163,12 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)
163              mi != mempool.mapTx.end(); ++mi)
164         {
165             const CTransaction& tx = mi->GetTx();
166-            if (tx.IsCoinBase() || !IsFinalTx(tx, nHeight, pblock->nTime))
167+
168+            int64_t nLockTimeCutoff = (STANDARD_LOCKTIME_VERIFY_FLAGS & LOCKTIME_MEDIAN_TIME_PAST)
    luke-jr commented at 2:37 am on October 23, 2015:
    I don’t see why this can’t be moved to a const like nMedianTimePast, and skip calculating it for every mempool tx?
    maaku commented at 3:57 pm on October 23, 2015:
    Any compiler more recent than the 1960’s will evaluate an expression consisting entirely of literals at compile time. In this case I prefer the clarity of specifying what is being tested in-line.
  43. luke-jr commented at 2:41 am on October 23, 2015: member
    @gmaxwell The max is only in CheckFinalTx (not IsFinal), which is never used for consensus-critical code…
  44. luke-jr commented at 2:42 am on October 23, 2015: member
    utACK, although prefer if at least this nit is done before merge.
  45. Add rules--presently disabled--for using GetMedianTimePast as endpoint for lock-time calculations 
    The lock-time code currently uses CBlock::nTime as the cutoff point for time based locked transactions. This has the unfortunate outcome of creating a perverse incentive for miners to lie about the time of a block in order to collect more fees by including transactions that by wall clock determination have not yet matured. By using CBlockIndex::GetMedianTimePast from the prior block instead, the self-interested miner no longer gains from generating blocks with fraudulent timestamps. Users can compensate for this change by simply adding an hour (3600 seconds) to their time-based lock times.

If enforced, this would be a soft-fork change. This commit only adds the functionality on an unexecuted code path, without changing the behaviour of Bitcoin Core.
    9d55050773
  46. Enable policy enforcing GetMedianTimePast as the end point of lock-time constraints 
    Transactions are not allowed in the memory pool or selected for inclusion in a block until their lock times exceed chainActive.Tip()->GetMedianTimePast(). However blocks including transactions which are only mature under the old rules are still accepted; this is *not* the soft-fork required to actually rely on the new constraint in production.
    dea8d21fc6
  47. maaku force-pushed on Oct 23, 2015
  48. maaku commented at 4:03 pm on October 23, 2015: contributor
    I believe that nits have been addressed and this is ready for merge. @laanwj ?
  49. laanwj merged this on Oct 26, 2015
  50. laanwj closed this on Oct 26, 2015
  51. laanwj referenced this in commit ff057f41aa on Oct 26, 2015
  52. in src/main.cpp: in dea8d21fc6 
    2752-        if (!IsFinalTx(tx, nHeight, block.GetBlockTime())) {
2753+    BOOST_FOREACH(const CTransaction& tx, block.vtx) {
2754+        int nLockTimeFlags = 0;
2755+        int64_t nLockTimeCutoff = (nLockTimeFlags & LOCKTIME_MEDIAN_TIME_PAST)
2756+                                ? pindexPrev->GetMedianTimePast()
2757+                                : block.GetBlockTime();
    NicolasDorier commented at 2:00 am on October 27, 2015:
    nLockTimeCutoff assignment could be move outside the loop. Is it because nLockTimeFlags will eventually be decided by the version of the transaction ?
  53. luke-jr commented at 6:57 pm on November 1, 2015: member
    It seems to me BIP 113 is actually a hardfork being treated as a softfork, at least how it currently stands, and this PR will potentially produce invalid blocks. Suggest reverting it and thinking through revising the BIP to be an actual softfork.
  54. sipa commented at 7:02 pm on November 1, 2015: member
    In my understanding - but this is just a rough check - it seems that Luke is right. A transaction whose locktime is between a block’s nTime and its GetMedianTimePast() will be accepted post-BIP113 and not before. That looks like going from invalid to valid, which is a hard fork.
  55. maaku commented at 7:03 pm on November 1, 2015: contributor

    Could you explain why it is a hard fork? On Nov 1, 2015 10:58 AM, “Luke-Jr” notifications@github.com wrote:

    It seems to me BIP 113 is actually a hardfork being treated as a softfork, at least how it currently stands, and this PR will potentially produce invalid blocks. Suggest reverting it and thinking through revising the BIP to be an actual softfork.

    — Reply to this email directly or view it on GitHub #6566 (comment).

  56. maaku commented at 7:05 pm on November 1, 2015: contributor

    A valid block cannot have a nTime less than GetMedianTimePast + 1. On Nov 1, 2015 11:03 AM, “Pieter Wuille” notifications@github.com wrote:

    In my understanding - but this is just a rough check - it seems that Luke is right. A transaction whose locktime is between a block’s nTime and its GetMedianTimePast() will be accepted post-BIP113 and not before. That looks like going from invalid to valid, which is a hard fork.

    — Reply to this email directly or view it on GitHub #6566 (comment).

  57. sipa commented at 7:09 pm on November 1, 2015: member

    Assume you have a perfect sequence of blocks, where block at height N has timestamp N*600.

    Block 12 has timestamp 7200. Its mediantimepast() is 3600. It is valid as 7200 is >= 3601.

    A transaction has nLockTime 5000. It is invalid currently in block 12, as 5000 is below 7200. Post BIP113, it is valid, as it is above 3600.

  58. gmaxwell commented at 4:49 am on November 2, 2015: contributor

    @sipa No, we’re being silly. The requirement is that the txlocktime be above $thing. Under the old rule $thing was blocktime, under this rule it is MTP. Since blocktime is already required to be greater than MTP, it’s fine.

    5000 < 7200 so that transaction is valid now. It is > 3600 so it wouldn’t be accepted by MTP.

  59. NicolasDorier commented at 5:50 am on November 2, 2015: contributor

    The requirement is that the txlocktime be above $thing. Under the old rule $thing was blocktime.

    So the requirement is : blocktime < txlocktime [blocktime]7200 < [txlocktime]5000 evaluate to false so the transaction is invalid now.

  60. sipa commented at 5:58 am on November 2, 2015: member

    Indeed, I was wrong. I assumed that the tx timestamp was supposed to be above the block timestamp - not sure why I made that assumption - and even looking at the code didn’t convince me otherwise.

    I’m now convinced this is not a problem.

  61. NicolasDorier commented at 7:33 am on November 2, 2015: contributor

    The requirement is that the txlocktime be above $thing

    The requirement is that the txlocktime be under $thing. Not above ! @sipa your example seemed right and this is indeed a hardfork.

  62. sipa commented at 7:47 am on November 2, 2015: member
    The requirement is the the blocktime is above the txlocktime, not the other way around.
  63. NicolasDorier commented at 7:59 am on November 2, 2015: contributor

    Got it. This is what I said (txlocktime be under blocktime == blocktime be above txlocktime), but got confused by gmaxwell remark who refuted your point by saying wrongly “txlocktime be above blocktime”.

    I hate comparisons. :(

  64. unknown commented at 4:51 am on June 12, 2018: none
    ^^
  65. MarcoFalke referenced this in commit 28bdaa3f76 on Mar 14, 2022
  66. sidhujag referenced this in commit 0daf624c9e on Mar 14, 2022
  67. DrahtBot locked this on Aug 16, 2022


maaku btcdrak dcousens CodeShark jtimon laanwj rustyrussell instagibbs jmcorgan afk11 rubensayshi gmaxwell petertodd luke-jr NicolasDorier sipa

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