Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Oct 26, 2015

Conversation

maaku
Copy link
Contributor

@maaku maaku commented Aug 18, 2015

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.

@maaku maaku changed the title Medianpasttimelock Mempool-only Median time-past as endpoint for lock-time calculations Aug 18, 2015
@btcdrak
Copy link
Contributor

btcdrak commented Sep 23, 2015

needs rebase

@maaku 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 maaku force-pushed the medianpasttimelock branch 3 times, most recently from d7d811a to 8167805 Compare Oct 1, 2015
@maaku
Copy link
Contributor Author

maaku commented Oct 16, 2015

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

@dcousens
Copy link
Contributor

dcousens commented Oct 16, 2015

@maaku awesome.

// 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))
Copy link
Contributor

@CodeShark CodeShark Oct 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jtimon jtimon Oct 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@maaku maaku Oct 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

CodeShark commented Oct 19, 2015

utACK

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");
}
Copy link
Contributor

@btcdrak btcdrak Oct 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

style nit: braces not required for this if

Copy link
Contributor Author

@maaku maaku Oct 19, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Braces already existed. Principle of smallest diff :)

Copy link
Member

@laanwj laanwj Oct 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copy link
Contributor

@btcdrak btcdrak Oct 22, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, agreed.

@btcdrak
Copy link
Contributor

btcdrak commented Oct 19, 2015

utACK

@maaku maaku force-pushed the medianpasttimelock branch 2 times, most recently from 29059c1 to d0e1799 Compare Oct 19, 2015
@maaku
Copy link
Contributor Author

maaku commented Oct 19, 2015

Nits addressed and rebased.

@btcdrak
Copy link
Contributor

btcdrak commented Oct 19, 2015

Looks like Travis randomly borked. Needs another force push.

@maaku maaku force-pushed the medianpasttimelock branch 3 times, most recently from d0e1799 to 3bd1769 Compare Oct 19, 2015
@maaku
Copy link
Contributor Author

maaku commented Oct 20, 2015

Travis is happy.

@dcousens
Copy link
Contributor

dcousens commented Oct 20, 2015

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
Copy link
Contributor

btcdrak commented Oct 20, 2015

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

@btcdrak
Copy link
Contributor

btcdrak commented Oct 20, 2015

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

@rustyrussell
Copy link
Contributor

rustyrussell commented Oct 21, 2015

Ack. Please apply soon :)

@@ -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)
Copy link
Member

@luke-jr luke-jr Oct 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@maaku maaku Oct 23, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

luke-jr commented Oct 23, 2015

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

@luke-jr
Copy link
Member

luke-jr commented Oct 23, 2015

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

maaku added 2 commits Oct 23, 2015
…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
Copy link
Contributor Author

maaku commented Oct 23, 2015

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

@laanwj laanwj merged commit dea8d21 into bitcoin:master Oct 26, 2015
laanwj added a commit that referenced this pull request Oct 26, 2015
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)
int nLockTimeFlags = 0;
int64_t nLockTimeCutoff = (nLockTimeFlags & LOCKTIME_MEDIAN_TIME_PAST)
? pindexPrev->GetMedianTimePast()
: block.GetBlockTime();
Copy link
Contributor

@NicolasDorier NicolasDorier Oct 27, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@luke-jr
Copy link
Member

luke-jr commented Nov 1, 2015

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
Copy link
Member

sipa commented Nov 1, 2015

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
Copy link
Contributor Author

maaku commented Nov 1, 2015

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

@maaku
Copy link
Contributor Author

maaku commented Nov 1, 2015

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

@sipa
Copy link
Member

sipa commented Nov 1, 2015

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
Copy link
Contributor

gmaxwell commented Nov 2, 2015

@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
Copy link
Contributor

NicolasDorier commented Nov 2, 2015

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
Copy link
Member

sipa commented Nov 2, 2015

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
Copy link
Contributor

NicolasDorier commented Nov 2, 2015

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
Copy link
Member

sipa commented Nov 2, 2015

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

@NicolasDorier
Copy link
Contributor

NicolasDorier commented Nov 2, 2015

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

Copy link

@ghost ghost left a comment

^^

MarcoFalke added a commit that referenced this pull request Mar 14, 2022
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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Mar 14, 2022
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet