#36192 test: cover unsatisfiable mining timestamp
| Reviewability | Reviews | Agreement | Size | |||||||
|---|---|---|---|---|---|---|---|---|---|---|
| #36192 test: cover unsatisfiable mining timestamp | Needs rebase | 2 | Strong | 0 + 124 tests | ||||||
|
|
|
|
| ||||||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | ||||||||||
Goal
This PR adds functional test coverage for an edge case where an attacker mines blocks with timestamps set to the victim node's future-time limit, making median time past (MTP) equal to the maximum allowable block time. It tests that both getblocktemplate and IPC createNewBlock fail cleanly and recover one second later without dropping the IPC connection. It also standardizes regtest timing constants across functional tests and adds a follow-up test assertion for Murch-Zawy boundary heights.
Problem: When an attacker pushes MTP to the node's maximum future block time, template creation cannot satisfy both constraints (nTime > MTP and nTime <= now + 2 hours). While practically infeasible on mainnet, the scenario was untested and it was unverified whether IPC connections survive the resulting error.
Reviewability
Ready · Needs rebase
Conflicts with master; review the logic now, the rebase is mechanical.
Waiting on (from the thread read):
- reviewer: darosior to comment or follow up as requested by fanquake 2026-10-21
Agreement
Strong
substantive support, no open objection (fjahr, ismaelsadeeq)
Thread notes: Reclassified fjahr's test simplification suggestion from objection to suggestion under updated rules. Added ismaelsadeeq's review ACK along with their suggestions regarding test location, runtime, and block invalidation, all of which were treated as non-blocking and dropped. / Reclassified rc:3996509310 as a suggestion under the current schema. Added ismaelsadeeq's review ACK and suggestions, and fanquake's ping to darosior.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| fjahr (member) | suggestion, approach | simplify the test to not mine to the end of the difficulty period since the edge case works at any height | agreed_to_disagree · author preferred testing retarget boundary heights for documentation; reviewer noted regtest mining is cheap enough and not a blocker | answered | yes | 2026-09-12: '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.' Status from: 2026-09-14: fjahr: '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.' |
| ismaelsadeeq (member) | suggestion, approach | move test to mining_basic.py next to timewarp and murch-zawy tests rather than in interface_ipc_mining.py | accepted · author explained testing IPC failure mode; reviewer confirmed not a blocker | answered | yes | 2026-10-21: 'This test seems to belong in mining_basic.py, not here, it sits right next to `test_timewarp()` and `test_murch_zawy_mintime()`.' Status from: 2026-10-21: ismaelsadeeq: 'It's fine to use one stone to kill two birds here, but we don't have to use it as a reason to add the test where it doesn't belong (Not a blocker as well).' |
| ismaelsadeeq (member) | suggestion, approach | reduce the test to a single boundary case instead of three rounds to reduce test runtime | accepted · reviewer noted ignoring the suggestion is not a blocker, author left code as-is | answered | yes | 2026-10-21: 'I'd also prefer this to just be a single test at any one boundary. The subtest currently runs ~3s; making it a single case drops it to ~1s.' Status from: 2026-10-21: ismaelsadeeq: 'feel free to push, will reACK or ignore not a blocker.' |
| ismaelsadeeq (member) | suggestion, approach | reduce block count by testing sequentially using invalidateblock rather than mining from scratch | accepted · author replied invalidateblock adds complexity; reviewer agreed it is not ideal and was just surfacing the idea | answered | yes | 2026-10-21: 'We could also reduce the number of blocks generated by making it a linear test' Status from: 2026-10-21: ismaelsadeeq: 'Yeah, not ideal, just surfacing the idea.' |
Support:
- fjahr (member) (ACK, 2026-09-14): Thanks for addressing my feedback!
- ismaelsadeeq (member) (ACK, 2026-10-21) [read: test/functional/interface_ipc_mining.py, test/functional/mining_basic.py]: (no reason given)
Participants:
- fjahr (member, support, 9 statements 2026-09-08 to 2026-09-14): reviewed PR, suggested test simplifications and consistency fixes, then gave code review ACK on db7a485c33
- adezo24h1 (none, neutral, 2 statements 2026-09-11 to 2026-09-14): tested functional tests on Ubuntu WSL without IPC, explicitly noted it is not an ACK
- fanquake (member, neutral, 1 statements 2026-10-21 to 2026-10-21): pinged darosior asking for follow-up or conceptual comment
- ismaelsadeeq (member, support, 2 statements 2026-10-21 to 2026-10-21): ACKed db7a485c33, suggested test placement and runtime simplifications, noting none are blockers
Files
153 lines under test/bench/ci.
- test/functional/interface_ipc_mining.py +92/-0
- test/functional/mining_basic.py +10/-8
- test/functional/rpc_blockchain.py +7/-7
- test/functional/test_framework/blocktools.py +7/-6
- test/functional/feature_assumeutxo.py +4/-4
- test/functional/feature_block.py +4/-4
Card
The short text the ranking pass compares across PRs.
This PR adds functional test coverage verifying that block template generation fails cleanly and recovers once time advances when an attacker temporarily pushes median time past to the node's future-time limit. The test covers both getblocktemplate and the IPC createNewBlock interface, ensuring the remote IPC connection is not dropped on error. It also cleans up shared regtest block constants in the test framework and adds a follow-up assertion for Murch-Zawy difficulty boundaries. The PR has a code review ACK from fjahr and is ready for merge.
Data
thread read through 2026-10-21 (22 statements), head db7a485c33
labels: Tests, Needs rebase
Categories
IPC / multiprocess
P3 · test coverage
Verifies that IPC connections survive and recover when template creation encounters an unsatisfiable timestamp error. While the edge case is practically infeasible on mainnet, confirming that the IPC connection does not drop under interface failures provides valuable stability verification.
Membership: Adds functional tests in test/functional/interface_ipc_mining.py verifying that IPC connections survive createNewBlock error conditions.
Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 0
Mining
P3 · test coverage
Pins down expected block template generation failure and recovery when MTP reaches the maximum future-time limit. The condition is practically infeasible on mainnet but was previously untested.
Membership: Adds tests for block template creation behavior (getblocktemplate and IPC createNewBlock) under extreme timestamp constraints across multiple mining test files.
Factors: security/stability 1, bug 0, performance 0, user value 0, leverage 0