Repository: zcash/zcash (language: rust)

Issue:
Block template construction fails to properly clamp the block time to satisfy the "future timestamp" rule
From protocol spec [§ 7.6 Block Header Encoding and Consensus](https://zips.z.cash/protocol/protocol.pdf#blockheader):

> For each block at block height 2 or greater on Mainnet, or block height 653606 or greater on Testnet, `nTime`
**MUST** be less than or equal to the median-time-past of that block plus 90 · 60 seconds.

This rule is not correctly applied by zcashd's block template construction.

If the median-time-past is more than 90 minutes ago, then a new block with `nTime` set to the current time would violate the rule. At `miner.cpp` lines 74-76 in `UpdateTime`: https://github.com/zcash/zcash/blob/1c0c425fdea00d5c6382a11245cfc4ee7bd492cd/src/miner.cpp#L68-L79
there is code that is intended to allow recovering from that situation by clamping `nTime` to the range allowed by this consensus rule (and also by the "nTime must be greater than median-time-past" rule).

However, that code does not work as intended, because `nOldTime` is set to `pblock->nTime` which is initially set to the current time at https://github.com/zcash/zcash/blob/1c0c425fdea00d5c6382a11245cfc4ee7bd492cd/src/miner.cpp#L369

`UpdateTime` is only called to update `nTime` for an existing block template, not to set it initially. So, if `nTime` starts at a value that violates the rule, the condition `nOldTime < nNewTime` does not hold when `UpdateTime` is called, and so `nTime` remains at the invalid value rather than being set to the clamped value.

(The intent of the `if (nOldTime < nNewTime)` is to not allow `nTime` to go backwards for a given template. This would be fine by itself absent the initialization problem.)

Note that if a block has not been mined on testnet for more than 24 hours, an additional problem is that (with default options) zcashd nodes will never exit Initial Block Download, and so will not start mining. However, that can be worked around by setting the `maxtipage` config option. Once it is set, the above bug will be exposed (and it is also exposed if the median-time-past is between 90 minutes and 24 hours ago).

This is in practice a testnet-only bug, because it is vanishingly unlikely that a block would fail to be mined on mainnet for 24h.

The fix is to initialize `nTime` to 0 and then immediately call `UpdateTime`, rather than initializing it to `GetTime()`.


Discussion / hints:

Fix patch:
diff --git a/src/miner.cpp b/src/miner.cpp
index 878c23a1df9..84a41a1d6dc 100644
--- a/src/miner.cpp
+++ b/src/miner.cpp
@@ -75,6 +75,7 @@ int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParam
         nNewTime = std::min(nNewTime, medianTimePast + MAX_FUTURE_BLOCK_TIME_MTP);
     }
 
+    // The timestamp of a given block template should not go backwards.
     if (nOldTime < nNewTime)
         pblock->nTime = nNewTime;
 
@@ -366,7 +367,11 @@ CBlockTemplate* BlockAssembler::CreateNewBlock(
     if (chainparams.MineBlocksOnDemand())
         pblock->nVersion = GetArg("-blockversion", pblock->nVersion);
 
-    pblock->nTime = GetTime();
+    // Setting nTime to 0 and then calling UpdateTime ensures that it is set to the
+    // nearest timestamp to the current time in the consensus-valid range (see #6960).
+    pblock->nTime = 0;
+    UpdateTime(pblock, chainparams.GetConsensus(), pindexPrev);
+
     const int64_t nMedianTimePast = pindexPrev->GetMedianTimePast();
     CCoinsViewCache view(pcoinsTip);
 


Test patch:
diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp
index 1141e7c8036..e73714b7840 100644
--- a/src/test/miner_tests.cpp
+++ b/src/test/miner_tests.cpp
@@ -372,18 +372,44 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
     // Stop here if we needed to mine more blocks.
     assert(nblocks == sizeof(blockinfo)/sizeof(*blockinfo));
 
+    auto nTime = pblocktemplate->block.nTime;
+    delete pblocktemplate;
+
     // Set the clock to be just ahead of the last "mined" block, to ensure we satisfy the
     // future timestamp soft fork rule.
-    auto curTime = GetTime();
-    OffsetClock::SetGlobal();
-    OffsetClock::Instance()->Set(std::chrono::seconds(-curTime + pblocktemplate->block.nTime));
+    FixedClock::SetGlobal();
+    FixedClock::Instance()->Set(std::chrono::seconds(nTime));
 
+    // Just to make sure we can still make simple blocks
+    BOOST_CHECK(pblocktemplate = BlockAssembler(chainparams).CreateNewBlock(scriptPubKey));
+    BOOST_CHECK_EQUAL(pblocktemplate->block.nTime, nTime);
+
+    auto mtp = chainActive.Tip()->GetMedianTimePast();
     delete pblocktemplate;
+    auto minTime = mtp + 1;
+    auto maxTime = mtp + MAX_FUTURE_BLOCK_TIME_MTP;
 
-    // Just to make sure we can still make simple blocks
+    // Set the clock to be too far ahead of the MTP, violating the future timestamp rule.
+    FixedClock::Instance()->Set(std::chrono::seconds(maxTime + 1));
+
+    // This should succeed, and nTime should be clamped to the maximum consensus-valid value.
+    BOOST_CHECK(pblocktemplate = BlockAssembler(chainparams).CreateNewBlock(scriptPubKey));
+    BOOST_CHECK_EQUAL(pblocktemplate->block.nTime, maxTime);
+    delete pblocktemplate;
+
+    // Set the clock to be equal to the median-time-past, violating the rule that it must
+    // be after the MTP.
+    FixedClock::Instance()->Set(std::chrono::seconds(minTime - 1));
+
+    // This should succeed, and nTime should be clamped to the minimum consensus-valid value.
     BOOST_CHECK(pblocktemplate = BlockAssembler(chainparams).CreateNewBlock(scriptPubKey));
+    BOOST_CHECK_EQUAL(pblocktemplate->block.nTime, minTime);
     delete pblocktemplate;
 
+    auto curTime = GetTime();
+    OffsetClock::SetGlobal();
+    OffsetClock::Instance()->Set(std::chrono::seconds(-curTime + nTime));
+
     TestMemPoolEntryHelper entry;
     entry.nFee = 0;
     entry.nHeight = 0;