mining: pr 33966 followups (disentangle miner startup defaults) #35403

pull Sjors wants to merge 5 commits into bitcoin:master from Sjors:2026/05/pr-33966-followups changing 4 files +18 −12

Sjors commented at 10:17 AM on May 28, 2026: member

This implement the suggested followups from #33966. Each commit links to the original comment.

The most important change is the extra asserts added in miner: ensure block_max_weight is flattened before limit checks.
DrahtBot added the label Mining on May 28, 2026
DrahtBot commented at 10:17 AM on May 28, 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/35403.



Reviews

See the guideline for information on the review process. A summary of reviews will appear here.



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #33922 (mining: add getMemoryLoad() and track template non-mempool memory footprint by Sjors)
- #33421 (node: add BlockTemplateCache by ismaelsadeeq)
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.



LLM Linter (✨ experimental)

Possible places where comparison-specific test macros should replace generic comparisons:
- [test/functional/interface_ipc_mining.py] assert template7 is None -> assert_equal(template7, None)
<sup>2026-05-29 07:52:25</sup>
Sjors commented at 5:52 PM on May 28, 2026: member

Pushed another commit to fix a missing MiniWallet rescan during test cleanup, noticed while working on #33922.
Sjors commented at 6:56 PM on May 28, 2026: member

The Alpine CI failure log is 278 MB, is that sane? https://github.com/bitcoin/bitcoin/actions/runs/26592252276/job/78354332966?pr=35403

It hit feature_dbcrash.py, so that's probably #35379. (I asked about the log size there)

CI ran on the new warp drive (since #35378).
DrahtBot added the label CI failed on May 28, 2026

test: use shared default_ipc_timeout

This commit does not change behavior.

Suggested in https://github.com/bitcoin/bitcoin/pull/33966#discussion_r3303357712

978e7216e6

mining: clarify test_block_validity comment

The option defaults to true, so describe the intended exceptional use
case as disabling validation for tests and benchmarks.

Suggested in https://github.com/bitcoin/bitcoin/pull/33966#discussion_r3303387586

65bd3164fb

miner: ensure block_max_weight is flattened before limit checks

Suggested in https://github.com/bitcoin/bitcoin/pull/33966#discussion_r3303462087

280ce6a0ae

test: merge mining options in package feerate check

See https://github.com/bitcoin/bitcoin/pull/33966#discussion_r3317409504

f4e643cb15

test: refresh MiniWallet after node restart b847626562
in test/functional/interface_ipc_mining.py:580 in 2290cf7311
```
 575 | @@ -577,9 +576,11 @@ async def async_routine():
 576 |  
 577 |      def run_test(self):
 578 |          self.miniwallet = MiniWallet(self.nodes[0])
 579 | +        # Amount of time the test is allowed to wait or be idle before it should fail.
 580 | +        self.default_ipc_timeout = 1000.0 * self.options.timeout_factor
```
maflcko commented at 7:37 AM on May 29, 2026:

Why are you removing the comment that says the type is milliseconds?

Sjors commented at 7:58 AM on May 29, 2026:

Added unit to the new variable.

It would be nice if Python had something like std::chrono so we wouldn't need comments to substitute a type system :-)

maflcko commented at 9:23 AM on May 29, 2026:

It would be nice if Python had something like std::chrono so we wouldn't need comments to substitute a type system :-)

Welcome to Python :)

llm nit: Looks like the latest push itched the llm:

[test/functional/interface_ipc_mining.py] assert template7 is None -> assert_equal(template7, None)

Sjors commented at 10:51 AM on May 29, 2026:

It keeps bringing that up, but it's not a line we're touching. I'm also not sure if it's actually better.

maflcko commented at 11:52 AM on May 29, 2026:

The benefit is only visible on a failure: Instead of just knowing that template7 is not None, the value of template7 will also be printed. If that isn't better, then assert_equal should just be removed from the utils.

Sjors commented at 11:54 AM on May 29, 2026:

assert_equal is useful when you care about the actual result. That's often the case, but not here. Anything other than None is a bug.
maflcko commented at 12:37 PM on May 29, 2026:

assert_equal is useful when you care about the actual result. That's often the case, but not here.

Why not? In other mining contexts we also compare that the result is None, because if it wasn't None and the test fails intermittently, it is useful to know why exactly it failed (was it a duplicate block, or an intermittently invalid one, or a bug in the test code itself [return False vs return None], etc ...)

E.g.:

test/functional/p2p_segwit.py: assert_equal(None, self.nodes[0].submitblock(block.serialize().hex()))

Generally I think, if a pattern makes sense, it should be used consistently.
Sjors force-pushed on May 29, 2026
DrahtBot removed the label CI failed on May 29, 2026

Contributors

Labels

Linked (view graph)

#28722 Multiprocess tracking issue #33777 Mining interface tracking issue #33966 refactor: disentangle miner startup defaults from runtime options

mining: pr 33966 followups (disentangle miner startup defaults) #35403

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)