Fixes #34262
getblocktemplate currently asserts when miner.createNewBlock() returns nullptr. This can happen during shutdown or when the mining subsystem is unavailable.
Replace the assertion with proper error handling that returns RPC_CLIENT_NOT_CONNECTED instead, indicating the service is temporarily unavailable.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34357.
See the guideline for information on the review process. A summary of reviews will appear here.
Reviewers, this pull request conflicts with the following ones:
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.
🚧 At least one of the CI tasks failed.
Task lint: https://github.com/bitcoin/bitcoin/actions/runs/21194965976/job/61189286769
LLM reason (✨ experimental): Lint failure: tabs used as whitespace detected by the tabs_whitespace check, causing exit code 1.
Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:
Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
@maflcko Thanks for the feedback!
I see two possible approaches here:
Approach 1 (my current fix): Add a defensive null-check in the RPC layer. When createNewBlock() returns nullptr, we return a clear error message to the caller instead of crashing. This handles the symptom gracefully.
Approach 2 (root cause fix): Ensure the RPC server only accepts requests after all subsystems (including mining) are fully initialized, and stops accepting requests before any subsystem shuts down. This prevents the nullptr scenario from happening in the first place.
Should I focus on fixing the startup/shutdown initialization order instead, or should we combine both approaches?
The defensive check still seems useful as a safety net (defense in depth), but I understand your concern that we should address why the RPC is accepting requests when the mining subsystem isn’t ready.
Approach 1 (my current fix): Add a defensive null-check in the RPC layer. When
createNewBlock()returns nullptr, we return a clear error message to the caller instead of crashing. This handles the symptom gracefully.
There is intentionally no crash in release builds the name also says: CHECK_NONFATAL. There is only a crash in debug builds, but this is just for increasing debug UX.
Do you understand the changes you are making, or is this generated by an LLM bot?
Yes I understand what I’m changing. The CHECK_NONFATAL was failing when createNewBlock returns null during shutdown/startup. I replaced it with a null check that returns an RPC error instead.
I see your point about it only crashing in debug builds though. I was looking at the logs from issue #34262 and saw both cases happened during startup or shutdown, so I thought handling the nullptr would help.
This is not generated by any LLM bot. I spent time debugging the issue and reading through the code.
Ok, fair enough.
i just think the current approach (1) is worse than CHECK_NONFATAL, because:
CHECK_NONFATAL is the right choice.Recall that release behavior is not changed at all by your change (in both cases an exception is thrown), just the message is changed.
I don’t see why this should be done, when it would be better to just fix the root cause.