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

maaku · 2015-08-18T01:20:06Z

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.

btcdrak · 2015-09-23T22:52:48Z

needs rebase

maaku · 2015-10-16T00:27:20Z

This pull request has been updated to no longer be dependent on #6312 or #6564.

dcousens · 2015-10-16T00:33:24Z

@maaku awesome.

CodeShark · 2015-10-19T02:21:01Z

src/main.cpp

    // Only accept nLockTime-using transactions that can be mined in the next
    // block; we don't want our mempool filled up with transactions that can't
    // be mined yet.
-    if (!CheckFinalTx(tx))
+    if (!IsFinalTx(tx, chainActive.Height() + 1, nLockTimeCutoff))


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.

IsFinalTx is compatible with libconsensus, CheckFinalTx (by using globals) is not.
Consensus functions cannot access globals like chainActive or GetAdjustedTime() directly.

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.

CodeShark · 2015-10-19T02:23:28Z

utACK

btcdrak · 2015-10-19T02:33:48Z

src/main.cpp

+        int64_t nLockTimeCutoff = (nLockTimeFlags & LOCKTIME_MEDIAN_TIME_PAST)
+                                ? pindexPrev->GetMedianTimePast()
+                                : block.GetBlockTime();
+        if (!IsFinalTx(tx, nHeight, nLockTimeCutoff)) {
            return state.DoS(10, error("%s: contains a non-final transaction", __func__), REJECT_INVALID, "bad-txns-nonfinal");
        }


style nit: braces not required for this if

Braces already existed. Principle of smallest diff :)

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

if (something) do_something_else(); goto handle_error;

OK, agreed.

btcdrak · 2015-10-19T09:04:23Z

utACK

maaku · 2015-10-19T21:09:35Z

Nits addressed and rebased.

btcdrak · 2015-10-19T22:33:42Z

Looks like Travis randomly borked. Needs another force push.

maaku · 2015-10-20T00:32:09Z

Travis is happy.

dcousens · 2015-10-20T10:16:36Z

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

btcdrak · 2015-10-20T11:20:41Z

@dcousens These rules will be eventually enforced through a soft-fork.

btcdrak · 2015-10-20T13:03:57Z

@dcousens As discussed at the RC meeting on the 15th, we aim to roll this out with CLTV using standard ISM.

rustyrussell · 2015-10-21T00:03:22Z

Ack. Please apply soon :)

luke-jr · 2015-10-23T02:37:54Z

src/miner.cpp

@@ -162,7 +163,12 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)
             mi != mempool.mapTx.end(); ++mi)
        {
            const CTransaction& tx = mi->GetTx();
-            if (tx.IsCoinBase() || !IsFinalTx(tx, nHeight, pblock->nTime))
+
+            int64_t nLockTimeCutoff = (STANDARD_LOCKTIME_VERIFY_FLAGS & LOCKTIME_MEDIAN_TIME_PAST)


I don't see why this can't be moved to a const like nMedianTimePast, and skip calculating it for every mempool tx?

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.

luke-jr · 2015-10-23T02:41:27Z

@gmaxwell The max is only in CheckFinalTx (not IsFinal), which is never used for consensus-critical code...

luke-jr · 2015-10-23T02:42:35Z

utACK, although prefer if at least this nit is done before merge.

…t 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.

…me 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.

maaku · 2015-10-23T16:03:17Z

I believe that nits have been addressed and this is ready for merge. @laanwj ?

dea8d21 Enable policy enforcing GetMedianTimePast as the end point of lock-time constraints (Mark Friedenbach) 9d55050 Add rules--presently disabled--for using GetMedianTimePast as endpoint for lock-time calculations (Mark Friedenbach)

NicolasDorier · 2015-10-27T02:00:05Z

src/main.cpp

+        int nLockTimeFlags = 0;
+        int64_t nLockTimeCutoff = (nLockTimeFlags & LOCKTIME_MEDIAN_TIME_PAST)
+                                ? pindexPrev->GetMedianTimePast()
+                                : block.GetBlockTime();


nLockTimeCutoff assignment could be move outside the loop.
Is it because nLockTimeFlags will eventually be decided by the version of the transaction ?

luke-jr · 2015-11-01T18:57:20Z

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.

sipa · 2015-11-01T19:02:39Z

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.

maaku · 2015-11-01T19:03:50Z

Could you explain why it is a hard fork?
On Nov 1, 2015 10:58 AM, "Luke-Jr" [email protected] 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).

maaku · 2015-11-01T19:05:49Z

A valid block cannot have a nTime less than GetMedianTimePast + 1.
On Nov 1, 2015 11:03 AM, "Pieter Wuille" [email protected] 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).

sipa · 2015-11-01T19:09:40Z

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.

gmaxwell · 2015-11-02T04:49:02Z

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

NicolasDorier · 2015-11-02T05:50:49Z

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.

sipa · 2015-11-02T05:58:47Z

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.

NicolasDorier · 2015-11-02T07:33:37Z

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.

sipa · 2015-11-02T07:47:41Z

The requirement is the the blocktime is above the txlocktime, not the other way around.

NicolasDorier · 2015-11-02T07:59:25Z

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. :(

ghost

^^

fa8d4d9 scripted-diff: Clarify CheckFinalTxAtTip name (MarcoFalke) fa4e30b policy: Remove unused locktime flags (MarcoFalke) Pull request description: The locktime flags have many issues: * They are passed in by a default argument, which is fragile. It has already lead to bugs like the one fixed in commit e30b6ea. * They are negative (signed), which doesn't make sense for flags (unsigned in general). According to the review comments when the code was added: "The max on the flags is a fairly weird operation." (#6566 (comment)) * No call site relies on the default argument and they all pass in a single compile-time constant, rendering most of the code dead and untested. * The dead code calls `GetAdjustedTime` (network adjusted time), which has its own issues. See #4521 Fix all issues by removing them ACKs for top commit: ajtowns: ACK fa8d4d9 theStack: Code-review ACK fa8d4d9 glozow: ACK fa8d4d9, agree the default arg `flags` is a massive footgun and just setting max flags is weird. Adding `AtTip` to the names makes sense to me, since they're both testing for *next* block and only ever used for {,re}addition to mempool. Tree-SHA512: 79f4a52f34909eb598d88bbae7afe8abe5f85f45c128483d16aa83dacd0e5579e561b725d01b1e9a931d1821012a51ad2bc6fb2867f8d09ee541f9d234d696f8

fa8d4d9 scripted-diff: Clarify CheckFinalTxAtTip name (MarcoFalke) fa4e30b policy: Remove unused locktime flags (MarcoFalke) Pull request description: The locktime flags have many issues: * They are passed in by a default argument, which is fragile. It has already lead to bugs like the one fixed in commit e30b6ea. * They are negative (signed), which doesn't make sense for flags (unsigned in general). According to the review comments when the code was added: "The max on the flags is a fairly weird operation." (bitcoin#6566 (comment)) * No call site relies on the default argument and they all pass in a single compile-time constant, rendering most of the code dead and untested. * The dead code calls `GetAdjustedTime` (network adjusted time), which has its own issues. See bitcoin#4521 Fix all issues by removing them ACKs for top commit: ajtowns: ACK fa8d4d9 theStack: Code-review ACK fa8d4d9 glozow: ACK fa8d4d9, agree the default arg `flags` is a massive footgun and just setting max flags is weird. Adding `AtTip` to the names makes sense to me, since they're both testing for *next* block and only ever used for {,re}addition to mempool. Tree-SHA512: 79f4a52f34909eb598d88bbae7afe8abe5f85f45c128483d16aa83dacd0e5579e561b725d01b1e9a931d1821012a51ad2bc6fb2867f8d09ee541f9d234d696f8

maaku changed the title ~~Medianpasttimelock~~ Mempool-only Median time-past as endpoint for lock-time calculations Aug 18, 2015

laanwj added Mempool Consensus labels Aug 20, 2015

btcdrak mentioned this pull request Sep 16, 2015

CHECKLOCKTIMEVERIFY (BIP65) IsSuperMajority() soft-fork #6351

Merged

maaku changed the title ~~Mempool-only Median time-past as endpoint for lock-time calculations~~ BIP-113: Mempool-only median time-past as endpoint for lock-time calculations Sep 24, 2015

maaku force-pushed the medianpasttimelock branch 3 times, most recently from d7d811a to 8167805 Compare October 1, 2015 01:16

maaku force-pushed the medianpasttimelock branch from 8167805 to 59ca129 Compare October 5, 2015 22:00

petertodd mentioned this pull request Oct 8, 2015

BIP-112: Mempool-only CHECKSEQUENCEVERIFY #6564

Closed

maaku force-pushed the medianpasttimelock branch from 59ca129 to 6bd1f3c Compare October 16, 2015 00:24

CodeShark reviewed Oct 19, 2015
View reviewed changes

btcdrak reviewed Oct 19, 2015
View reviewed changes

maaku force-pushed the medianpasttimelock branch 2 times, most recently from 29059c1 to d0e1799 Compare October 19, 2015 21:07

maaku force-pushed the medianpasttimelock branch 3 times, most recently from d0e1799 to 3bd1769 Compare October 19, 2015 23:56

luke-jr reviewed Oct 23, 2015
View reviewed changes

maaku added 2 commits October 23, 2015 09:02

maaku force-pushed the medianpasttimelock branch from 3bd1769 to dea8d21 Compare October 23, 2015 16:03

btcdrak mentioned this pull request Oct 23, 2015

Backport #6566, median-past locktime, rebased against 0.11 #6884

Merged

laanwj merged commit dea8d21 into bitcoin:master Oct 26, 2015

afk11 mentioned this pull request Oct 26, 2015

BIP-68: Mempool-only sequence number constraint verification #6312

Closed

NicolasDorier reviewed Oct 27, 2015
View reviewed changes

afk11 mentioned this pull request Mar 18, 2016

BIP9 versionbits softfork for BIP68, BIP112 and BIP113 #7648

Merged

ghost reviewed Jun 12, 2018

View reviewed changes

maflcko mentioned this pull request Jan 16, 2022

policy: Remove unused locktime flags #24080

Merged

Sjors mentioned this pull request Jun 22, 2022

Remove LOCKTIME_MEDIAN_TIME_PAST constant #24565

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

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

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

Conversation

maaku commented Aug 18, 2015

btcdrak commented Sep 23, 2015

maaku commented Oct 16, 2015

dcousens commented Oct 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CodeShark commented Oct 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

btcdrak commented Oct 19, 2015

maaku commented Oct 19, 2015

btcdrak commented Oct 19, 2015

maaku commented Oct 20, 2015

dcousens commented Oct 20, 2015

btcdrak commented Oct 20, 2015

btcdrak commented Oct 20, 2015

rustyrussell commented Oct 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luke-jr commented Oct 23, 2015

luke-jr commented Oct 23, 2015

maaku commented Oct 23, 2015

Choose a reason for hiding this comment

luke-jr commented Nov 1, 2015

sipa commented Nov 1, 2015

maaku commented Nov 1, 2015

maaku commented Nov 1, 2015

sipa commented Nov 1, 2015

gmaxwell commented Nov 2, 2015

NicolasDorier commented Nov 2, 2015

sipa commented Nov 2, 2015

NicolasDorier commented Nov 2, 2015

sipa commented Nov 2, 2015

NicolasDorier commented Nov 2, 2015

ghost left a comment

Choose a reason for hiding this comment