test: cover unsatisfiable mining timestamp #36192

Sjors commented at 11:46 AM on September 8, 2026: member

This PR adds test coverage for what happens in the (extremely unlikely) event that the miner can't satisfy the constraints. This is inspired by the recently added timewarp-protection and Murch-Zawy rules (for the miner, not for consensus, see #30681 and #35949), but this is a pre-existing issue. It is not made worse by these rules.

The test has an attacker mine 6 blocks, which set MTP at the victim's future-time limit. It calls getblocktemplate and the equivalent IPC method, which both fail. It then moves mock time one second forward, and demonstrates mining works again (and we don't drop the IPC connection).

This (impractical) attack can be done at any height, be we illustrate it for blocks 142, 143 and 0 of a retarget period, to clarify that the timewarp-protection and Murch-Zawy rules do not matter.

The test was added to ipc_mining.py, to avoid having to add IPC support to mining_basic.py. Mining coverage is currently split between various test files and not always mirrored between RPC and IPC. A followup could unify these, while still skipping the IPC side when that's not compiled.

A few refactor commits to prepare:

fix incorrect name: REGTEST_RETARGET_PERIOD -> HALVING_INTERVAL
drop REGTEST_ prefix from REGTEST_N_BITS and REGTEST_TARGET (consistent with HALVING_INTERVAL, DIFFICULTY_ADJUSTMENT_INTERVAL and TIME_GENESIS_BLOCK).
move DIFFICULTY_ADJUSTMENT_INTERVAL to the framework

The last commit picks up a Murch-Zawy followup suggested in #35949 (review) and takes advantage of our refactor, but is otherwise unrelated.

DrahtBot added the label Tests on Sep 8, 2026

DrahtBot commented at 11:46 AM on September 8, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36192.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	fjahr
Concept ACK	adezo24h1

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36257 (qa: assert_equals -> assert_true/assert_false by hodlinator)
#35793 (Implement BIP 54 (Consensus Cleanup) without mainnet activation by darosior)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

fanquake requested review from fjahr on Sep 8, 2026

fanquake requested review from darosior on Sep 8, 2026

test: rename REGTEST_RETARGET_PERIOD to HALVING_INTERVAL

The constant is the regtest nSubsidyHalvingInterval and is only used
as the default halving_period in create_coinbase.

8a45b25d3b

Sjors force-pushed on Sep 8, 2026

DrahtBot added the label CI failed on Sep 8, 2026

Sjors force-pushed on Sep 8, 2026

DrahtBot removed the label CI failed on Sep 8, 2026

in test/functional/test_framework/blocktools.py:72 in 8a45b25d3b outdated

  68 | @@ -69,7 +69,7 @@
  69 |  VERSIONBITS_LAST_OLD_BLOCK_VERSION = 4
  70 |  MIN_BLOCKS_TO_KEEP = 288
  71 |  
  72 | -REGTEST_RETARGET_PERIOD = 150
  73 | +HALVING_INTERVAL = 150  # regtest nSubsidyHalvingInterval

fjahr commented at 2:42 PM on September 8, 2026:

nit: I would have preferred to the REGTEST_ prefix

Sjors commented at 12:18 PM on September 10, 2026:

I prefer to only prefix other networks.

fjahr commented at 1:55 PM on September 12, 2026:

Could your remove them from REGTEST_N_BITS and REGTEST_TARGET then so it is at least consistent?

Sjors commented at 12:36 PM on September 14, 2026:

Done.

I did look at either dropping REGTEST_ from REGTEST_N_BITS and REGTEST_TARGET, or adding it to DIFFICULTY_ADJUSTMENT_INTERVAL and TIME_GENESIS_BLOCK. The latter is a bigger change, and leads to longer names.

in test/functional/test_framework/blocktools.py:74 in 4a9fac04ac outdated

  70 | @@ -71,6 +71,7 @@
  71 |  
  72 |  HALVING_INTERVAL = 150  # regtest nSubsidyHalvingInterval
  73 |  
  74 | +DIFFICULTY_ADJUSTMENT_INTERVAL = 144  # regtest nPowTargetTimespan / nPowTargetSpacing

fjahr commented at 2:45 PM on September 8, 2026:

nit: Similarly, would suggest to have a REGTEST_ prefix here too

fjahr commented at 4:19 PM on September 8, 2026: contributor

Concept ACK on the first three commits, not sure yet about the unsatisfiable test

The PR description was pretty confusing to me. The third commit adds the suggested test edits from @sedited which are in the MZ-related test. But then the new test for the unsatisfiable mining timestamp doesn't seem to be related to MZ. The PR description kind of makes it seem like it is.

I am also not sure this test has to be in ipc interface test. It looks nice the way it’s implemented but adding it in mining_basic is probably possible too and runs this test more regularly. But then again, this is more for documentation purposes and not so much regression testing, so I guess it doesn’t matter. Would still be good to explicitly state the motivation though.

Sjors force-pushed on Sep 10, 2026

Sjors commented at 12:31 PM on September 10, 2026: member

@fjahr I swapped the last two commits and clarified the PR description to point out that:

Murch-Zawy was the inspiration, but no the actual issue.
the last commit is just a hitch-hiker

I am also not sure this test has to be in ipc interface test. It looks nice the way it’s implemented but adding it in mining_basic is probably possible too and runs this test more regularly.

Both are covered by CI. And we should cover both IPC and RPC because they have different failure modes (in particular the IPC test checks it doesn't disconnect).

Adding it to mining_basic means having to add IPC support to that test, and skipping part of the test if that's not compiled (which is less clear than skipping the whole test).

adezo24h1 commented at 7:36 PM on September 11, 2026: none

Tested 1f6694e on Ubuntu 24.04 WSL (ENABLE_IPC=OFF, bitcoind from ~/src/bitcoin): python3 build/test/functional/test_runner.py mining_basic.py rpc_blockchain.py Result: pass Did not run interface_ipc_mining.py (IPC not built). I read the Python test files but I am still learning the timestamp / MTP logic, so this is not an ACK ;-)

DrahtBot requested review from fjahr on Sep 11, 2026

in test/functional/mining_basic.py:55 in 1f6694ecb3

  51 | @@ -51,7 +52,6 @@
  52 |  )
  53 |  
  54 |  
  55 | -DIFFICULTY_ADJUSTMENT_INTERVAL = 144
  56 |  MAX_FUTURE_BLOCK_TIME = 2 * 3600

fjahr commented at 2:09 PM on September 12, 2026:

Can import MAX_FUTURE_BLOCK_TIME from blocktools.py as well.

in test/functional/interface_ipc_mining.py:801 in 1f6694ecb3

 796 | +            (victim.getblockcount() + 1) % DIFFICULTY_ADJUSTMENT_INTERVAL,
 797 | +            DIFFICULTY_ADJUSTMENT_INTERVAL - 1,
 798 | +        )
 799 | +        assert_equal(victim.getblockchaininfo()["mediantime"], attacker_time)
 800 | +
 801 | +        # The next block must be later than MTP, but future timestamps are only

fjahr commented at 2:32 PM on September 12, 2026:

The test can be simplified because it doesn't seem to need the mining to the end of the difficulty period in order to work.

diff --git a/test/functional/interface_ipc_mining.py b/test/functional/interface_ipc_mining.py
index 66aa5e6eddf..43ac5fe2dee 100755
--- a/test/functional/interface_ipc_mining.py
+++ b/test/functional/interface_ipc_mining.py
@@ -10,7 +10,6 @@ from copy import deepcopy
 from decimal import Decimal
 from io import BytesIO
 from test_framework.blocktools import (
-    DIFFICULTY_ADJUSTMENT_INTERVAL,
     MAX_FUTURE_BLOCK_TIME,
     NORMAL_GBT_REQUEST_PARAMS,
     NULL_OUTPOINT,
@@ -777,25 +776,12 @@ class IPCMiningTest(BitcoinTestFramework):
         victim = self.nodes[0]
         attacker = self.nodes[1]

-        # Mine to the end of the difficulty period, with room for six attack
-        # blocks that determine its median time past.
-        num_attack_blocks = 6
-        blocks_to_mine = (
-            DIFFICULTY_ADJUSTMENT_INTERVAL - 2 - num_attack_blocks
-            - victim.getblockcount()
-        ) % DIFFICULTY_ADJUSTMENT_INTERVAL
-        self.generate(attacker, blocks_to_mine)
-
+        # Six of the last eleven blocks determine the median time past.
         victim_time = int(time.time())
         attacker_time = victim_time + MAX_FUTURE_BLOCK_TIME
         victim.setmocktime(victim_time)
         attacker.setmocktime(attacker_time)
-        self.generate(attacker, num_attack_blocks)
-
-        assert_equal(
-            (victim.getblockcount() + 1) % DIFFICULTY_ADJUSTMENT_INTERVAL,
-            DIFFICULTY_ADJUSTMENT_INTERVAL - 1,
-        )
+        self.generate(attacker, 6)
         assert_equal(victim.getblockchaininfo()["mediantime"], attacker_time)

         # The next block must be later than MTP, but future timestamps are only

Sjors commented at 12:36 PM on September 14, 2026:

See #36192 (comment)

fjahr commented at 2:48 PM on September 12, 2026: contributor

Thanks for addressing my comments, I think the description can still be improved unless I have a misunderstanding on my side.

Fortunately it doesn't create a situation where the miner can't satisfy those constraints, but it turns out the previously merged timewarp rule does.

I don't understand how the timewarp rule plays a role here. It's just a conflict between the plain MTP + 1 rule and the 2 hour max future rule, no?

The test has an attacker mine 6 blocks at the end of a difficulty adjustment period, which set MTP at the victim's future-time limit.

It doesn't have to be at the end of the adjustment period since MZ doesn't play a role for this test, it works on any height afaict, see also my comment about simplifying the test.

Unrelated to the description:

And we should cover both IPC and RPC because they have different failure modes (in particular the IPC test checks it doesn't disconnect).

Ok, that's fine for me as a goal, but that probably applies to most of the other tests in miner_basic.py as well and also possible future tests right? Is there a plan to unify those test files so that we don't have to discuss where new mining tests go every time? I guess it would make sense that we have one miner test file that runs every test always in basic mode and then also runs each of them ipc when possible. But I didn't look into what that would look like in practice.

DrahtBot requested review from fjahr on Sep 12, 2026

test: drop REGTEST_ prefix from block difficulty constants

Consistently omit the REGTEST_ prefix for constants.

af7c45ef2f

test: share block timing constants through blocktools

Move DIFFICULTY_ADJUSTMENT_INTERVAL to blocktools, since mining_basic.py
and rpc_blockchain.py both defined the regtest difficulty adjustment
interval. Also import the existing MAX_FUTURE_BLOCK_TIME constant in
mining_basic.py instead of defining it locally.

327bd124cb

test: cover unsatisfiable mining timestamp

Simulate attacker blocks setting MTP at the victim's future-time
limit. Check getblocktemplate and IPC failure, then recovery one
second later.

This attack is impractical, but the code is reachable.

3840a9048a

test: assert Murch-Zawy period boundaries

Co-authored-by: sedited <seb.kung@gmail.com>

db7a485c33

Sjors commented at 12:36 PM on September 14, 2026: member

I don't understand how the timewarp rule plays a role here.

You're right, it doesn't. Since I was confused myself, I modified the test instead to repeat for blocks N - 2, N - 1 and 0 of a retarget period, with brief comments for why neither rule matters. I also updated the PR description.

Is there a plan to unify those test files

There isn't, but I made the suggestion explicit in the PR description. Since we introduced IPC, and its various methods, more new mining test coverage was added on the IPC side, with some of it mirrored to RPC, see e.g. #31981.

Sjors force-pushed on Sep 14, 2026

adezo24h1 commented at 3:30 PM on September 14, 2026: none

Tested db7a485c330ed154c2aa705f553d323c7b2276c6 Not an ACK. Ran on Ubuntu 24.04 WSL (ENABLE_IPC=OFF): python3 build/test/functional/test_runner.py mining_basic.py rpc_blockchain.py Result: pass I didn't run interface_ipc_mining.py, fyi

in test/functional/interface_ipc_mining.py:792 in 3840a9048a outdated

 787 | +        # The six attack blocks use the highest timestamp the victim accepts,
 788 | +        # putting MTP at its future-time limit. The next block needs MTP + 1,
 789 | +        # so template creation fails instead of returning an unmineable template.
 790 | +        error = "TestBlockValidity failed: time-too-new, block timestamp too far in the future"
 791 | +
 792 | +        for period_position in (

fjahr commented at 5:17 PM on September 14, 2026:

nit: I find it pretty heavy that this test now mines ~450 blocks just to prove that these special heights and their BIP54 rules do not have an impact on this super edge case scenario that is documented with the test. I would have prefered that the test had been simplified to be more focussed on the edge case only and with mining only 6 blocks. But regtest mining is cheap enough that this isn't a blocker for me and maybe there is some value in the added documentation here.

Sjors commented at 7:16 AM on September 15, 2026:

An earlier version mined up to N-2 and then performed the check on blocks N - 1, N and 0. But I found that confusing, hence the three rounds. If it's actually a performance problem, then we can drop it, but as you say, regtest is cheap.

fjahr commented at 5:21 PM on September 14, 2026: contributor

Code review ACK db7a485c330ed154c2aa705f553d323c7b2276c6

Thanks for addressing my feedback!