UpdateTime() needs to account for timewarp fix on testnet4

[Sjors]

In (miner.cpp) we set the block (template) timestamp as follows:

pblock->nTime = TicksSinceEpoch<std::chrono::seconds>(NodeClock::now());
...
UpdateTime(pblock, ...)


int64_t UpdateTime(CBlockHeader* pblock, ...)
{
    int64_t nOldTime = pblock->nTime;
    int64_t nNewTime{std::max<int64_t>(pindexPrev->GetMedianTimePast() + 1, TicksSinceEpoch<std::chrono::seconds>(NodeClock::now()))};

    if (nOldTime < nNewTime) {
        pblock->nTime = nNewTime;
    }

In other words, we pick the system time, and if needed bump it to MTP + 1.

This doesn't take the timewarp mitigation for testnet4 into account. The first block must have a timestamp no less than 7200 seconds (2 hours) before the last block. There's discussion about lowering that to 600 from the original proposal.

An adversarial miner could set the timestamp 2 hours in the future. The current code would then set the next block at the current time. With the current value of 7200 this should not cause an issue, even if we immediately find a block. But it seems safer to explicitly account for this and increase the timestamp if needed, especially if we ever lower the value.

We should also add a functional test that uses fake time to test the limit case: we don't want to end up accepting a block exactly 2 hours in the future and then failing to generate a template because there's no possible valid timestamp due to some off-by-one error. (I don't think this can happen though, haven't thought it through)

See also
bitcoin/bips#1658 (comment)

[Sjors]

Although I think we should fix this, it seems potentially problematic to expect other node / miner implementations to also fix this. That's something to consider in the discussion of whether to change the 7200 value.

[garlonicon]

If someone uses 600 seconds as a MAX_TIMEWARP, then that node can be stuck on block 42335. More information: https://bitcointalk.org/index.php?topic=5499150.msg64488234#msg64488234

I wonder, if all blocks after 42335 will be reorged, or if MAX_TIMEWARP will be bigger than 600. I guess v28.0 was released with that value, and some nodes are stuck, because of that.

[Sjors]

This was fixed in #30681.

However someone should have tried a full IBD before changing the testnet4 consensus rules. An incompatible block in the past won't be retroactively rejected by miners, but it will be by new noes.

I'll try that, and open a new issue if needed.

[Sjors]

However someone should have tried a full IBD before changing the testnet4 consensus rules.

I just did. It gets stuck at block 42,335 from yesterday. So at least there was no historical block violating the rule at the time this "soft fork" was introduced.

Opening a fresh issue...