Prevent block.nTime from decreasing #6177

Merged
merged 1 commit into from Aug 6, 2015

Conversation

Projects
None yet
9 participants
@maaku
Contributor

maaku commented May 22, 2015

Under some circumstances it is possible for there to be a significant,
discontinuous jump in a node's clock value. On mining nodes, this can
result in block templates which are no longer valid due to time-based
nLockTime constraints. UpdateTime() is modified so that it will never
decrease a block's nLockTime.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr May 23, 2015

Member

Commit description does not match. Also might be good to trigger a new CreateNewBlock in getblocktemplate, but not required I guess. Other than those, utACK.

Member

luke-jr commented May 23, 2015

Commit description does not match. Also might be good to trigger a new CreateNewBlock in getblocktemplate, but not required I guess. Other than those, utACK.

@maaku

This comment has been minimized.

Show comment
Hide comment
@maaku

maaku May 23, 2015

Contributor

I think the wording matches both the intent and the source code. Did I make a mistake somewhere? There was a grammar error which I fixed.

Contributor

maaku commented May 23, 2015

I think the wording matches both the intent and the source code. Did I make a mistake somewhere? There was a grammar error which I fixed.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr May 23, 2015

Member

"Fail a block template if block.nTime decreases", but nothing fails?

Member

luke-jr commented May 23, 2015

"Fail a block template if block.nTime decreases", but nothing fails?

@maaku maaku changed the title from Fail a block template if block.nTime decreases to Prevent block.nTime from decreasing May 23, 2015

@maaku

This comment has been minimized.

Show comment
Hide comment
@maaku

maaku May 23, 2015

Contributor

Right, fixed.

Contributor

maaku commented May 23, 2015

Right, fixed.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd May 23, 2015

Contributor

Given that we already are guaranteed to mine valid blocks regardless by the GetMedianTimePast() check, how important is it really to avoid letting time go backwards?

As for the specific getblocktemplate issue have you actually seen this in practice? CreateNewBlock() checks every transaction against IsFinalTx() prior to including it in the template, so if I'm understanding the code correctly the worst that would happen is a transaction would be delayed from being mined and would sit in the mempool.

Additionally I and others have been thinking about changing the criteria for time-based nLockTime's to be compared against the median time rather than the block time for a number of incentive reasons. (possibly with a time-warp fix as well) At the very least I'm thinking of doing a patch to change the mempool behavior to accept txs based on the median rather than adjusted time.

Contributor

petertodd commented May 23, 2015

Given that we already are guaranteed to mine valid blocks regardless by the GetMedianTimePast() check, how important is it really to avoid letting time go backwards?

As for the specific getblocktemplate issue have you actually seen this in practice? CreateNewBlock() checks every transaction against IsFinalTx() prior to including it in the template, so if I'm understanding the code correctly the worst that would happen is a transaction would be delayed from being mined and would sit in the mempool.

Additionally I and others have been thinking about changing the criteria for time-based nLockTime's to be compared against the median time rather than the block time for a number of incentive reasons. (possibly with a time-warp fix as well) At the very least I'm thinking of doing a patch to change the mempool behavior to accept txs based on the median rather than adjusted time.

@cinnamoncoin

This comment has been minimized.

Show comment
Hide comment
@cinnamoncoin

cinnamoncoin May 23, 2015

If I understand correctly you want to prevent a solved block from having an epoch time stamp earlier than any prior block ? (Is the concern to prevent selfish/dishonest miners to possibly orphan the next block solved?)

If I understand correctly you want to prevent a solved block from having an epoch time stamp earlier than any prior block ? (Is the concern to prevent selfish/dishonest miners to possibly orphan the next block solved?)

@maaku

This comment has been minimized.

Show comment
Hide comment
@maaku

maaku May 23, 2015

Contributor

Peter, in the current code it is possible for the block's timestamp to be
less than timestamp that was used during transaction selection, which means
it is possible for invalid work to be generated: a block which contains a
transaction whose nLockTime is not satisfied by block.nTime. This patch
fixes that edge case.

Further constraining the validity rules as you describe would also solve
the problem, but probably see some public debate.

On Sat, May 23, 2015 at 8:18 AM, Peter Todd notifications@github.com
wrote:

Given that we already are guaranteed to mine valid blocks regardless by
the GetMedianTimePast() check, how important is it really to avoid letting
time go backwards?

As for the specific getblocktemplate issue have you actually seen this in
practice? CreateNewBlock() checks every transaction against IsFinalTx()
prior to including it in the template, so if I'm understanding the code
correctly the worst that would happen is a transaction would be delayed
from being mined and would sit in the mempool.

Additionally I and others have been thinking about changing the criteria
for time-based nLockTime's to be compared against the median time rather
than the block time for a number of incentive reasons. (possibly with a
time-warp fix as well) At the very least I'm thinking of doing a patch to
change the mempool behavior to accept txs based on the median rather than
adjusted time.


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

Contributor

maaku commented May 23, 2015

Peter, in the current code it is possible for the block's timestamp to be
less than timestamp that was used during transaction selection, which means
it is possible for invalid work to be generated: a block which contains a
transaction whose nLockTime is not satisfied by block.nTime. This patch
fixes that edge case.

Further constraining the validity rules as you describe would also solve
the problem, but probably see some public debate.

On Sat, May 23, 2015 at 8:18 AM, Peter Todd notifications@github.com
wrote:

Given that we already are guaranteed to mine valid blocks regardless by
the GetMedianTimePast() check, how important is it really to avoid letting
time go backwards?

As for the specific getblocktemplate issue have you actually seen this in
practice? CreateNewBlock() checks every transaction against IsFinalTx()
prior to including it in the template, so if I'm understanding the code
correctly the worst that would happen is a transaction would be delayed
from being mined and would sit in the mempool.

Additionally I and others have been thinking about changing the criteria
for time-based nLockTime's to be compared against the median time rather
than the block time for a number of incentive reasons. (possibly with a
time-warp fix as well) At the very least I'm thinking of doing a patch to
change the mempool behavior to accept txs based on the median rather than
adjusted time.


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

@maaku

This comment has been minimized.

Show comment
Hide comment
@maaku

maaku May 23, 2015

Contributor

No, this has nothing to do with the timestamps of previous blocks. It is to
prevent invalid blocks from being generated, blocks which contain
time-locked transactions which have not yet matured because the timestamp
was altered after the transaction was selected.

On Sat, May 23, 2015 at 11:07 AM, cinnamon_carter notifications@github.com
wrote:

If I understand correctly you want to prevent a solved block from having
an epoch time stamp earlier than any prior block ? (Is the concern to
prevent selfish/dishonest miners to possibly orphan the next block solved?)


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

Contributor

maaku commented May 23, 2015

No, this has nothing to do with the timestamps of previous blocks. It is to
prevent invalid blocks from being generated, blocks which contain
time-locked transactions which have not yet matured because the timestamp
was altered after the transaction was selected.

On Sat, May 23, 2015 at 11:07 AM, cinnamon_carter notifications@github.com
wrote:

If I understand correctly you want to prevent a solved block from having
an epoch time stamp earlier than any prior block ? (Is the concern to
prevent selfish/dishonest miners to possibly orphan the next block solved?)


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

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd May 23, 2015

Contributor

@maaku Ah, yeah, I'd forgotten we modify the block's nTime after creation, ugh; there's another usage of UpdateTime() in rpcmining.cpp in the getblocktemplate code that needs fixing too.

An alternative would be for UpdateTime() to simply never reduce the block nTime, which would remove the seldom tested codepath where the block template fails due to time going backwards. Basically the difference between what you've done and that option is that if GetAdjustedTime() somehow goes backwards by > 2 hours you risk creating a block that is invalid due to being too far into the future. Equally though, if GetAdjustedTime() goes backwards sufficiently far back that GetMedianTimePast() > GetAdjustedTime() + 2hrs you're also equally screwed - I have to wonder if the types of problems that would cause the latter are all that much less likely than the problems that would cause the former.

In any case, this pull-req needs more comments explaining what's going on, e.g. explain why UpdateTime() can cause the mining loop to break on line https://github.com/maaku/bitcoin/blob/blocktime-monotonic/src/miner.cpp#L539

re: constraining the validity rules - of course, that needs a -dev mailing list post for the mempool change, and a full BIP if we eventually do a soft-fork.

Contributor

petertodd commented May 23, 2015

@maaku Ah, yeah, I'd forgotten we modify the block's nTime after creation, ugh; there's another usage of UpdateTime() in rpcmining.cpp in the getblocktemplate code that needs fixing too.

An alternative would be for UpdateTime() to simply never reduce the block nTime, which would remove the seldom tested codepath where the block template fails due to time going backwards. Basically the difference between what you've done and that option is that if GetAdjustedTime() somehow goes backwards by > 2 hours you risk creating a block that is invalid due to being too far into the future. Equally though, if GetAdjustedTime() goes backwards sufficiently far back that GetMedianTimePast() > GetAdjustedTime() + 2hrs you're also equally screwed - I have to wonder if the types of problems that would cause the latter are all that much less likely than the problems that would cause the former.

In any case, this pull-req needs more comments explaining what's going on, e.g. explain why UpdateTime() can cause the mining loop to break on line https://github.com/maaku/bitcoin/blob/blocktime-monotonic/src/miner.cpp#L539

re: constraining the validity rules - of course, that needs a -dev mailing list post for the mempool change, and a full BIP if we eventually do a soft-fork.

Prevent block.nTime from decreasing
Under some circumstances it is possible for there to be a significant,
discontinuous jump in a node's clock value. On mining nodes, this can
result in block templates which are no longer valid due to time-based
nLockTime constraints. UpdateTime() is modified so that it will never
decrease a block's nLockTime, thereby preventing such invalidations.
@maaku

This comment has been minimized.

Show comment
Hide comment
@maaku

maaku May 28, 2015

Contributor

Fixed nit regarding code comments.

Contributor

maaku commented May 28, 2015

Fixed nit regarding code comments.

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jun 2, 2015

Member

utACK

Member

luke-jr commented Jun 2, 2015

utACK

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jun 2, 2015

Member

utACK

Member

jtimon commented Jun 2, 2015

utACK

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 14, 2015

Member

Untested ACK

Member

sipa commented Jun 14, 2015

Untested ACK

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Jun 22, 2015

Member

utACK

Member

btcdrak commented Jun 22, 2015

utACK

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jul 23, 2015

Contributor

ut ACK

Contributor

jgarzik commented Jul 23, 2015

ut ACK

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Aug 6, 2015

Member

ping @laanwj This looks ready for merging.

Member

btcdrak commented Aug 6, 2015

ping @laanwj This looks ready for merging.

@laanwj laanwj merged commit ef8dfe4 into bitcoin:master Aug 6, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Aug 6, 2015

Merge pull request #6177
ef8dfe4 Prevent block.nTime from decreasing (Mark Friedenbach)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment