Merge 5ba770089e into 433412fd84

On testnet4 with the timewarp mitigation active, when pool software ignores the curtime and mintime fields provided by the getblocktemplate RPC or by createNewBlock() in the Mining interface, they are vulnerable to a griefing attack.

The test is expanded to illustrate this.

src/node/miner.cpp

@@ -28,18 +28,24 @@
 #include <utility>
 
 namespace node {
-int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev)
-{
-    int64_t nOldTime = pblock->nTime;
-    int64_t nNewTime{std::max<int64_t>(pindexPrev->GetMedianTimePast() + 1, TicksSinceEpoch<std::chrono::seconds>(NodeClock::now()))};
 
+int64_t GetMinimumTime(const CBlockIndex* pindexPrev, const Consensus::Params& consensusParams)
+{
+    int64_t min_time{pindexPrev->GetMedianTimePast() + 1};
     if (consensusParams.enforce_BIP94) {
         // Height of block to be mined.
         const int height{pindexPrev->nHeight + 1};
         if (height % consensusParams.DifficultyAdjustmentInterval() == 0) {
-            nNewTime = std::max<int64_t>(nNewTime, pindexPrev->GetBlockTime() - MAX_TIMEWARP);
+            min_time = std::max<int64_t>(min_time, pindexPrev->GetBlockTime() - MAX_TIMEWARP);
         }
     }
+    return min_time;
+}
+
+int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev)
+{
+    int64_t nOldTime = pblock->nTime;
+    int64_t nNewTime{std::max<int64_t>(GetMinimumTime(pindexPrev, consensusParams), TicksSinceEpoch<std::chrono::seconds>(NodeClock::now()))};
 
     if (nOldTime < nNewTime) {
         pblock->nTime = nNewTime;

src/node/miner.h

@@ -207,6 +207,8 @@ private:
     void SortForBlock(const CTxMemPool::setEntries& package, std::vector<CTxMemPool::txiter>& sortedEntries);
 };
 
+int64_t GetMinimumTime(const CBlockIndex* pindexPrev, const Consensus::Params& consensusParams);
+
 int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev);
 
 /** Update an old GenerateCoinbaseCommitment from CreateNewBlock after the block txs have changed */

src/rpc/mining.cpp

@@ -49,6 +49,7 @@
 using interfaces::BlockTemplate;
 using interfaces::Mining;
 using node::BlockAssembler;
+using node::GetMinimumTime;
 using node::NodeContext;
 using node::RegenerateCommitments;
 using node::UpdateTime;
@@ -954,7 +955,7 @@ static RPCHelpMan getblocktemplate()
     result.pushKV("coinbasevalue", (int64_t)block.vtx[0]->vout[0].nValue);
     result.pushKV("longpollid", tip.GetHex() + ToString(nTransactionsUpdatedLast));
     result.pushKV("target", hashTarget.GetHex());
-    result.pushKV("mintime", (int64_t)pindexPrev->GetMedianTimePast()+1);
+    result.pushKV("mintime", GetMinimumTime(pindexPrev, consensusParams));
     result.pushKV("mutable", std::move(aMutable));
     result.pushKV("noncerange", "00000000ffffffff");
     int64_t nSigOpLimit = MAX_BLOCK_SIGOPS_COST;

test/functional/mining_basic.py

@@ -136,20 +136,39 @@ class MiningTest(BitcoinTestFramework):
 
         for _ in range(n):
             t += 600
-            self.nodes[0].setmocktime(t)
+            node.setmocktime(t)
             self.generate(self.wallet, 1, sync_fun=self.no_op)
 
-        self.log.info("Create block two hours in the future")
-        self.nodes[0].setmocktime(t + MAX_FUTURE_BLOCK_TIME)
+        self.log.info("Create block MAX_TIMEWARP < t < MAX_FUTURE_BLOCK_TIME in the future")
+        # A timestamp that's more than MAX_TIMEWARP seconds in the future can
+        # happen by accident, due to a combination of pool software that doesn't
+        # use "curtime" AND has a faulty clock.
+        #
+        # But it could also be intentional, at the end of a retarget period, in
+        # order to make the next block miner violate the time-timewarp-attack rule.
+        # For this attack to succeed the victim miner needs to ignore both our
+        # "curtime" and "mintime" values AND use wall clock time. This is true even
+        # if the victim miner implements the MTP rule.
+        #
+        # The attack is illustrated below.
+        #
+        # Force the next block to have a timestamp in the future:
+        future = t + MAX_TIMEWARP + 1
+        # Witout violating the 2 hour in the future rule
+        assert_greater_than_or_equal(t + MAX_FUTURE_BLOCK_TIME, future)
+        node.setmocktime(future)
         self.generate(self.wallet, 1, sync_fun=self.no_op)
-        assert_equal(node.getblock(node.getbestblockhash())['time'], t + MAX_FUTURE_BLOCK_TIME)
+        assert_equal(node.getblock(node.getbestblockhash())['time'], future)
 
         self.log.info("First block template of retarget period can't use wall clock time")
-        self.nodes[0].setmocktime(t)
-        # The template will have an adjusted timestamp, which we then modify
+        node.setmocktime(t)
+        # The template will have an adjusted timestamp.
         tmpl = node.getblocktemplate(NORMAL_GBT_REQUEST_PARAMS)
-        assert_greater_than_or_equal(tmpl['curtime'], t + MAX_FUTURE_BLOCK_TIME - MAX_TIMEWARP)
+        assert_equal(tmpl['curtime'], t + 1)
+        # mintime and curtime should match
+        assert_equal(tmpl['mintime'], tmpl['curtime'])
 
+        # Check that the adjusted timestamp results in a valid block
         block = CBlock()
         block.nVersion = tmpl["version"]
         block.hashPrevBlock = int(tmpl["previousblockhash"], 16)
@@ -161,18 +180,29 @@ class MiningTest(BitcoinTestFramework):
         assert_template(node, block, None)
 
         bad_block = copy.deepcopy(block)
+        # Use wall clock instead of the adjusted timestamp. This could happen
+        # by accident if pool software ignores mintime and curtime.
         bad_block.nTime = t
         bad_block.solve()
         assert_raises_rpc_error(-25, 'time-timewarp-attack', lambda: node.submitheader(hexdata=CBlockHeader(bad_block).serialize().hex()))
 
-        self.log.info("Test timewarp protection boundary")
-        bad_block.nTime = t + MAX_FUTURE_BLOCK_TIME - MAX_TIMEWARP - 1
+        # It can also happen if the pool implements its own logic to adjust its
+        # timestamp to MTP + 1, but doesn't take the new timewarp rule into
+        # account (and ignores mintime).
+        mtp = node.getblock(node.getbestblockhash())["mediantime"] + 1
+        bad_block.nTime = mtp + 1
         bad_block.solve()
         assert_raises_rpc_error(-25, 'time-timewarp-attack', lambda: node.submitheader(hexdata=CBlockHeader(bad_block).serialize().hex()))
 
-        bad_block.nTime = t + MAX_FUTURE_BLOCK_TIME - MAX_TIMEWARP
+        self.log.info("Test timewarp protection boundary")
+        bad_block.nTime = future - MAX_TIMEWARP - 1
         bad_block.solve()
-        node.submitheader(hexdata=CBlockHeader(bad_block).serialize().hex())
+        assert_raises_rpc_error(-25, 'time-timewarp-attack', lambda: node.submitheader(hexdata=CBlockHeader(bad_block).serialize().hex()))
+
+        good_block = copy.deepcopy(bad_block)
+        good_block.nTime = future - MAX_TIMEWARP
+        good_block.solve()
+        node.submitheader(hexdata=CBlockHeader(good_block).serialize().hex())
 
     def test_pruning(self):
         self.log.info("Test that submitblock stores previously pruned block")