test: cover unsatisfiable mining timestamp #36192

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

   The Murch-Zawy rule introduced in #35949 (for the miner, not for consensus) adds yet another constraint to the timestamp for our block templates. 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.

   This PR adds test coverage for what happens in the (extremely unlikely) event that the miner can't satisfy the constraints.

   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 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 attack is impractical, but the code is reachable, so deserves a test.

   The first commit fixes the incorrectly named REGTEST_RETARGET_PERIOD. The second commit moves (actually) DIFFICULTY_ADJUSTMENT_INTERVAL to the framework, so the next two commits can use it. The third commit contains the new test.

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

2. DrahtBot added the label Tests on Sep 8, 2026
3. 
   DrahtBot commented at 11:46 AM on September 8, 2026: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--006a51241073e994b41acfe9ec718e94-->

   Code Coverage & Benchmarks

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

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

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

   Type	Reviewers
   Concept ACK	fjahr

   If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

   <!--5faf32d7da4f0f540f40219e4f7537a3-->

4. fanquake requested review from fjahr on Sep 8, 2026
5. fanquake requested review from darosior on Sep 8, 2026
6. 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
7. test: move DIFFICULTY_ADJUSTMENT_INTERVAL to blocktools

   Both mining_basic.py and rpc_blockchain.py defined their own copy of
   the regtest difficulty adjustment interval.

   4a9fac04ac
8. Sjors force-pushed on Sep 8, 2026
9. DrahtBot added the label CI failed on Sep 8, 2026
10. Sjors force-pushed on Sep 8, 2026
11. DrahtBot removed the label CI failed on Sep 8, 2026
12. 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.

13. 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

14. 
    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.

15. 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.

    fd76837de0
16. test: assert Murch-Zawy period boundaries

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

    1f6694ecb3
17. Sjors force-pushed on Sep 10, 2026
18. 
    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).

Contributors

SjorsDrahtBotfjahr

Review Requested
darosior