Signal m_tip_block_cv when Ctrl-C is pressed or SIGTERM is received, the same way it is currently signaled when the stop RPC is called. This lets RPC calls like waitforblockheight and IPC calls like waitTipChanged be interrupted, instead of waiting for their original timeouts and delaying shutdown.
This issue was reported by plebhash in #33463. These hangs have been present since #30409. A similar bug was also fixed previously in Qt in #18452 and this PR simplifies that fix.
Currently when CTRL-C is pressed and there is an active `waitforblockheight`,
or `waitforblock`, or `waitfornewblock` RPC call, or a mining interface
`waitTipChanged` IPC call with a long timeout, the node will not shut down
right away, and will wait for the timeout to be reached before exiting.
This behavior is not ideal and only happens when the node is stopped with
CTRL-C or SIGTERM. When the node is stopped with `bitcoin-cli stop`, the wait
calls are interrupted and the node does shut down right away.
The next commit improves node behavior. This commit just adds test coverage to
simplify the next commit and clarify the change in behavior there.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33511.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | Sjors, TheCharlatan |
| Concept ACK | enirox001 |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
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.
I’m confused by windows CI failures: 1 2
They seem to show py -3 test_runner.py being called, and this just with failing with no output after around 10 minutes, with CI reporting Process completed with exit code -1073741510. where -1073741510 is 0xc000013a or STATUS_CONTROL_C_EXIT. So it seems like sending ctrl-c to the node might be killing the whole test?
EDIT: Turns out this is expected, the test needs to start the node with creationflags=subprocess.CREATE_NEW_PROCESS_GROUP on windows, otherwise ctrl-c does kill all processes.
Signal m_tip_block_cv when Ctrl-C is pressed or SIGTERM is received, the same
way it is currently signalled when the `stop` RPC is called. This lets RPC
calls like `waitforblockheight` and IPC calls like `waitTipChanged` be
interrupted, instead of waiting for their original timeouts and delaying
shutdown.
Historical notes:
- The behavior where `stop` RPC signals `m_tip_block_cv`, but CTRL-C does not,
has been around since the condition variable was introduced in #30409
(7eccdaf16081d6f624c4dc21df75b0474e049d2b).
- The signaling was later moved without changing behavior in #30967
(5ca28ef28bcca1775ff49921fc2528d9439b71ab). This commit moves it again to
the Interrupt() function, which is probably the place it should have been
added initially, so it works for Ctrl-C shutdowns as well as `stop`
shutdowns.
- A Qt shutdown bug calling wait methods was fixed previously in #18452
(da73f1513a637a9f347b64de66564d6cdb2541f8), and this change updates that
fix to avoid the hang happening again in Qt.
pr/sigwait.1 -> pr/sigwait.2, compare) to fix CI failures, also added more documentation and explanation in commit description
ACK 68cad90dace40f7a015ca4ff81b878fc8fdc1dd5
This should be backported in my opinion.
Mmm, the GUI experience is less great (testnet4, ctrl + c from the terminal where I launched it):
This seems to stay stuck.
Doesn’t happen on the first commit, so might be a regression.
Mmm, the GUI experience is less great (testnet4, ctrl + c from the terminal where I launched it): […] Doesn’t happen on the first commit, so might be a regression.
Oh, thanks for testing. That does sound like a regression. Will mark as draft until this is debugged
This should be backported in my opinion.
Not going to block 30.0 if it’s not ready, but marked for backport.
pr/sigwait.2 -> pr/sigwait.3, compare) fixing regression in previous version that caused Qt shutdown to hang during wait calls from the GUI console
tACK c25a5e670b27d3b6eb958ce437dbe89678bd1511
It’s also a nice simplification.
Concept ACK c25a5e6
Replicated the issue; fix works. Good refactor in init and interfaces for interrupt support.
Tested on Linux with Ctrl+C to trigger the signal interruption. Please clarify testnet behavior.
270+ # so it is possible to distinguish whether it times out or returns
271+ # early. If it times out it will throw an exception, and if it
272+ # returns early it will return the current block height.
273+ self.log.debug(f"Calling waitforblockheight with {self.rpc_timeout} sec RPC timeout")
274+ fut = ex.submit(node.waitforblockheight, height=current_height+1, timeout=self.rpc_timeout*1000*2)
275+ time.sleep(1)
sleep in the tests. They are confusing and brittle. Fixed in https://github.com/bitcoin/bitcoin/pull/34137