Prevent block.nTime from decreasing

maaku commented at 11:59 pm on May 22, 2015: contributor

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 commented at 0:12 am on 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.

maaku force-pushed on May 23, 2015

maaku commented at 1:27 am on 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.

luke-jr commented at 1:48 am on May 23, 2015: member

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

maaku force-pushed on May 23, 2015

maaku renamed this:
~~Fail a block template if block.nTime decreases~~
Prevent block.nTime from decreasing
on May 23, 2015

maaku commented at 12:41 pm on May 23, 2015: contributor

Right, fixed.

petertodd commented at 3:18 pm on 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.

cinnamoncoin commented at 6:07 pm on May 23, 2015: none

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 commented at 7:37 pm on 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).

maaku commented at 7:38 pm on 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).

petertodd commented at 11:11 pm on 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.

laanwj added the label Mining on May 26, 2015

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.

ef8dfe41d1

maaku force-pushed on May 27, 2015