Merge bitcoin/bitcoin#33511: init: Fix Ctrl-C shutdown hangs during wait calls

c25a5e670b27d3b6eb958ce437dbe89678bd1511 init: Signal m_tip_block_cv on Ctrl-C (Ryan Ofsky)
6a29f79006a9d60b476893dface5eea8f9bf271c test: Test SIGTERM handling during waitforblockheight call (Ryan Ofsky)

Pull request description:

  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.

ACKs for top commit:
  Sjors:
    tACK c25a5e670b27d3b6eb958ce437dbe89678bd1511
  TheCharlatan:
    ACK c25a5e670b27d3b6eb958ce437dbe89678bd1511
  enirox001:
    Concept ACK c25a5e6

Tree-SHA512: 320aaa74fd308e826521c48c9a8aca4bd5f5530064cda2303d251d8e93e50c474bcd0db760ce04921928e73abefe4847aff797ac9ca7c89e74e5051bbed061cd

src/init.cpp

@@ -215,8 +215,6 @@ void InitContext(NodeContext& node)
     node.shutdown_request = [&node] {
         assert(node.shutdown_signal);
         if (!(*node.shutdown_signal)()) return false;
-        // Wake any threads that may be waiting for the tip to change.
-        if (node.notifications) WITH_LOCK(node.notifications->m_tip_block_mutex, node.notifications->m_tip_block_cv.notify_all());
         return true;
     };
 }
@@ -267,6 +265,8 @@ void Interrupt(NodeContext& node)
 #if HAVE_SYSTEM
     ShutdownNotify(*node.args);
 #endif
+    // Wake any threads that may be waiting for the tip to change.
+    if (node.notifications) WITH_LOCK(node.notifications->m_tip_block_mutex, node.notifications->m_tip_block_cv.notify_all());
     InterruptHTTPServer();
     InterruptHTTPRPC();
     InterruptRPC();

src/node/interfaces.cpp

@@ -132,7 +132,6 @@ public:
     }
     void appShutdown() override
     {
-        Interrupt(*m_context);
         Shutdown(*m_context);
     }
     void startShutdown() override
@@ -141,12 +140,7 @@ public:
         if (!(Assert(ctx.shutdown_request))()) {
             LogError("Failed to send shutdown signal\n");
         }
-
-        // Stop RPC for clean shutdown if any of waitfor* commands is executed.
-        if (args().GetBoolArg("-server", false)) {
-            InterruptRPC();
-            StopRPC();
-        }
+        Interrupt(*m_context);
     }
     bool shutdownRequested() override { return ShutdownRequested(*Assert(m_context)); };
     bool isSettingIgnored(const std::string& name) override

test/functional/feature_init.py

@@ -3,12 +3,14 @@
 # Distributed under the MIT software license, see the accompanying
 # file COPYING or http://www.opensource.org/licenses/mit-license.php.
 """Tests related to node initialization."""
+from concurrent.futures import ThreadPoolExecutor
 from pathlib import Path
 import os
 import platform
 import shutil
 import signal
 import subprocess
+import time
 
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.test_node import (
@@ -240,9 +242,58 @@ class InitTest(BitcoinTestFramework):
         self.stop_node(0)
         assert not custom_pidfile_absolute.exists()
 
+    def break_wait_test(self):
+        """Test what happens when a break signal is sent during a
+        waitforblockheight RPC call with a long timeout. Ctrl-Break is sent on
+        Windows and SIGTERM is sent on other platforms, to trigger the same node
+        shutdown sequence that would happen if Ctrl-C were pressed in a
+        terminal. (This can be different than the node shutdown sequence that
+        happens when the stop RPC is sent.)
+
+        The waitforblockheight call should be interrupted and return right away,
+        and not time out."""
+
+        self.log.info("Testing waitforblockheight RPC call followed by break signal")
+        node = self.nodes[0]
+
+        if platform.system() == 'Windows':
+            # CREATE_NEW_PROCESS_GROUP prevents python test from exiting
+            # with STATUS_CONTROL_C_EXIT (-1073741510) when break is sent.
+            self.start_node(node.index, creationflags=subprocess.CREATE_NEW_PROCESS_GROUP)
+        else:
+            self.start_node(node.index)
+
+        current_height = node.getblock(node.getbestblockhash())['height']
+
+        with ThreadPoolExecutor(max_workers=1) as ex:
+            # Call waitforblockheight with wait timeout longer than RPC timeout,
+            # so it is possible to distinguish whether it times out or returns
+            # early. If it times out it will throw an exception, and if it
+            # returns early it will return the current block height.
+            self.log.debug(f"Calling waitforblockheight with {self.rpc_timeout} sec RPC timeout")
+            fut = ex.submit(node.waitforblockheight, height=current_height+1, timeout=self.rpc_timeout*1000*2)
+            time.sleep(1)
+
+            self.log.debug(f"Sending break signal to pid {node.process.pid}")
+            if platform.system() == 'Windows':
+                # Note: CTRL_C_EVENT should not be sent here because unlike
+                # CTRL_BREAK_EVENT it can not be targeted at a specific process
+                # group and may behave unpredictably.
+                node.process.send_signal(signal.CTRL_BREAK_EVENT)
+            else:
+                # Note: signal.SIGINT would work here as well
+                node.process.send_signal(signal.SIGTERM)
+            node.process.wait()
+
+            result = fut.result()
+            self.log.debug(f"waitforblockheight returned {result!r}")
+            assert_equal(result["height"], current_height)
+            node.wait_until_stopped()
+
     def run_test(self):
         self.init_pid_test()
         self.init_stress_test()
+        self.break_wait_test()
 
 
 if __name__ == '__main__':