ipc: Handle unclean shutdowns better

ryanofsky commented at 8:58 pm on April 24, 2025: contributor

This PR fixes various problems when IPC connections are broken or hang which were reported in https://github.com/bitcoin-core/libmultiprocess/issues/123, https://github.com/bitcoin-core/libmultiprocess/issues/176, and https://github.com/bitcoin-core/libmultiprocess/pull/182. The different fixes are described in commit messages.

The first two commits of this PR update the libmultiprocess subtree including the following PRs:

The subtree changes can be verified by running test/lint/git-subtree-check.sh src/ipc/libmultiprocess as described in developer notes and lint instructions.

The remaining commits are:

The new commits depend on the subtree update, and because the subtree update includes an incompatible API change, the “Use EventLoopRef” commit needs to be part of the same PR to avoid breaking the build. The other commits also make sense to merge at the same time because the bitcoin & libmultiprocess changes were written and tested together.

This PR is part of the process separation project.

DrahtBot commented at 8:58 pm on April 24, 2025: 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/32345.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	pinheadmz, Sjors, josibake
Concept ACK	darosior, ismaelsadeeq

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32427 ((RFC) kernel: Replace leveldb-based BlockTreeDB with flat-file based store by TheCharlatan)
#31382 (kernel: Flush in ChainstateManager destructor by TheCharlatan)

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.

DrahtBot added the label CI failed on Apr 25, 2025

DrahtBot commented at 0:33 am on April 25, 2025: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/41118314270

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.

fanquake commented at 10:20 am on April 25, 2025: member

https://github.com/bitcoin/bitcoin/actions/runs/14651550530/job/41119318297?pr=32345#step:5:966:

0Assertion failed: fRPCInWarmup, file ./rpc/server.cpp, line 329
1Error: Process completed with exit code 3.

https://cirrus-ci.com/task/5477140512112640?logs=ci#L5037:

0[17:33:37.941] ./test/node_init_tests.cpp(41): error: in "node_init_tests/init_test": check AppInitMain(m_node) has failed
1[17:33:37.941] Error: Cannot resolve -bind address: ''
2[17:33:37.954] ./test/node_init_tests.cpp(32): Leaving test case "init_test"; testing time: 32703us

https://cirrus-ci.com/task/5336403023757312?logs=ci#L2886:

0[18:45:09.651] /ci_container_base/src/ipc/interfaces.cpp:35:5: error: ignoring return value of function declared with 'warn_unused_result' attribute [-Werror,-Wunused-result]
1[18:45:09.651]    35 |     write(STDOUT_FILENO, g_ignore_ctrl_c.data(), g_ignore_ctrl_c.size());
2[18:45:09.651]       |     ^~~~~ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3[18:45:09.651] 1 error generated.

ryanofsky force-pushed on Apr 25, 2025

ryanofsky commented at 5:38 pm on April 25, 2025: contributor

Updated e8372fa3a606d35146c72fc61663e17c67a4621a -> a56468ab0bf2154159919a2c3feba9974486d10e (pr/ipc-stop.1 -> pr/ipc-stop.2, compare) to fix draftbot typos and CI failures, though I could not reproduce the previous releases AppInitMain failure in local CI so it could still be broken

darosior commented at 7:53 pm on April 25, 2025: member

Concept ACK. Will test using my Rust client for #29409 rebased on top of this.

ryanofsky force-pushed on Apr 28, 2025

ryanofsky commented at 4:22 pm on April 28, 2025: contributor

Updated a56468ab0bf2154159919a2c3feba9974486d10e -> 0b72656ecf363d9fc4820a9e3e22afb2c98d4e75 (pr/ipc-stop.2 -> pr/ipc-stop.3, compare) to try to fix init test CI failures https://github.com/bitcoin/bitcoin/actions/runs/14670251419/job/41176416988?pr=32345 and https://cirrus-ci.com/task/6094268256747520 Updated 0b72656ecf363d9fc4820a9e3e22afb2c98d4e75 -> 5b38e62ccbfcdb7f20a6a74fcc4ef2358e3da56c (pr/ipc-stop.3 -> pr/ipc-stop.4, compare) to fix test_bitcoin-qt failures caused by previous fix https://github.com/bitcoin/bitcoin/pull/32345/checks?check_run_id=41288589561

ryanofsky force-pushed on Apr 28, 2025

DrahtBot removed the label CI failed on Apr 28, 2025

maflcko commented at 6:18 am on April 29, 2025: member

There are also some typos, according to #32345 (comment), if you want to fix them if you have to re-touch for other reasons anyway.

DrahtBot added the label CI failed on May 5, 2025

DrahtBot added the label Needs rebase on May 9, 2025

darosior commented at 6:49 pm on May 20, 2025: member

I tested this PR by rebasing #29409 on top of it and running my (updated) core_bdk_wallet against it.

I found that bitcoin-node will (still) segfault if i try to stop it while the client is running. The steps i followed were to 1) start bitcoin-node 2) start my PoC which will connect to it and stall with the open connection for 6 seconds 3) before the 6 seconds have elapsed, stop the bitcoin-node process (in this specific case by sending it a SIGINT). Here are the logs of the bitcoin-node process:

 0[...]
 12025-05-20T18:44:44Z init message: Starting network threads…
 22025-05-20T18:44:44Z DNS seeding disabled
 32025-05-20T18:44:44Z init message: Done loading
 42025-05-20T18:44:44Z initload thread start
 52025-05-20T18:44:44Z msghand thread start
 62025-05-20T18:44:44Z Loading 0 mempool transactions from file...
 72025-05-20T18:44:44Z Imported mempool transactions from file: 0 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
 82025-05-20T18:44:44Z opencon thread start
 92025-05-20T18:44:44Z addcon thread start
102025-05-20T18:44:44Z net thread start
112025-05-20T18:44:44Z initload thread exit
122025-05-20T18:44:44Z New manual v2 peer connected: version: 70016, blocks=113, peer=0
132025-05-20T18:44:44Z Synchronizing blockheaders, height: 113 (~0.47%)
142025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server recv request  [#1](/bitcoin-bitcoin/1/) Init.construct$Params ()
152025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server send response [#1](/bitcoin-bitcoin/1/) Init.construct$Results (threadMap = <external capability>)
162025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server recv request  [#2](/bitcoin-bitcoin/2/) Init.makeChain$Params (context = (thread = <external capability>))
172025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server post request  [#2](/bitcoin-bitcoin/2/) {bitcoin-node-948798/b-capnp-loop-948823 (from )}
182025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server send response [#2](/bitcoin-bitcoin/2/) Init.makeChain$Results (result = <external capability>)
192025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server recv request  [#3](/bitcoin-bitcoin/3/) Chain.initMessage$Params (context = (thread = <external capability>), message = "Oxydation of the Bitcoin Core wallet in progress..")
202025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server post request  [#3](/bitcoin-bitcoin/3/) {bitcoin-node-948798/b-capnp-loop-948823 (from )}
212025-05-20T18:44:45Z init message: Oxydation of the Bitcoin Core wallet in progress..
222025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server send response [#3](/bitcoin-bitcoin/3/) Chain.initMessage$Results ()
232025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server recv request  [#4](/bitcoin-bitcoin/4/) Chain.showProgress$Params (context = (thread = <external capability>), title = "BDK Core startup", progress = 1, resumePossible = false)
242025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server post request  [#4](/bitcoin-bitcoin/4/) {bitcoin-node-948798/b-capnp-loop-948823 (from )}
252025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server send response [#4](/bitcoin-bitcoin/4/) Chain.showProgress$Results ()
262025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server recv request  [#5](/bitcoin-bitcoin/5/) Chain.getHeight$Params (context = (thread = <external capability>))
272025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server post request  [#5](/bitcoin-bitcoin/5/) {bitcoin-node-948798/b-capnp-loop-948823 (from )}
282025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server send response [#5](/bitcoin-bitcoin/5/) Chain.getHeight$Results (result = 113, hasResult = true)
292025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server recv request  [#6](/bitcoin-bitcoin/6/) Chain.getBlockHash$Params (context = (thread = <external capability>), height = 113)
302025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server post request  [#6](/bitcoin-bitcoin/6/) {bitcoin-node-948798/b-capnp-loop-948823 (from )}
312025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server send response [#6](/bitcoin-bitcoin/6/) Chain.getBlockHash$Results (result = "H\\3678\\307#\\020|\\020\\017\\340\\2469\\330\\356\\335\\277\\341dOB{\\220\\327\\354[\\361\\036\\233\\301\\035DD")
322025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server recv request  [#7](/bitcoin-bitcoin/7/) Chain.showProgress$Params (context = (thread = <external capability>), title = "BDK Core startup", progress = 100, resumePossible = true)
332025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server post request  [#7](/bitcoin-bitcoin/7/) {bitcoin-node-948798/b-capnp-loop-948823 (from )}
342025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server send response [#7](/bitcoin-bitcoin/7/) Chain.showProgress$Results ()
352025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server recv request  [#8](/bitcoin-bitcoin/8/) Chain.handleNotifications$Params (context = (thread = <external capability>), notifications = <external capability>)
362025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server post request  [#8](/bitcoin-bitcoin/8/) {bitcoin-node-948798/b-capnp-loop-948823 (from )}
372025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server send response [#8](/bitcoin-bitcoin/8/) Chain.handleNotifications$Results (result = <external capability>)
382025-05-20T18:44:45Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server destroy N2mp11ProxyServerIN3ipc5capnp8messages7HandlerEEE
39^C2025-05-20T18:44:47Z Shutdown: In progress...
402025-05-20T18:44:47Z addcon thread exit
412025-05-20T18:44:47Z opencon thread exit
422025-05-20T18:44:47Z net thread exit
432025-05-20T18:44:47Z msghand thread exit
442025-05-20T18:44:47Z scheduler thread exit
452025-05-20T18:44:47Z Writing 0 mempool transactions to file...
462025-05-20T18:44:47Z Writing 0 unbroadcast transactions to file.
472025-05-20T18:44:47Z Dumped mempool: 0.000s to copy, 0.003s to dump, 27 bytes dumped to file
482025-05-20T18:44:47Z Flushed fee estimates to fee_estimates.dat.
492025-05-20T18:44:47Z [ipc] {bitcoin-node-948798/b-shutoff-948798} IPC client first request from current thread, constructing waiter
502025-05-20T18:44:47Z [ipc] {bitcoin-node-948798/b-shutoff-948798} IPC client send ChainNotifications.chainStateFlushed$Params (context = (thread = <external capability>, callbackThread = <external capability>), role = 0, locator = "\\200\\021\\001\\000\\022H\\3678\\307#\\020|\\020\\017\\340\\2469\\330\\356\\335\\277\\341dOB{\\220\\327\\354[\\361\\036\\233\\301\\035DD\\tGm\\237]YbZDV9\\254\\252f\\205\\001\\323K\\375!\\340\\t\\367\\375\\346\\264W\\207\\000\\2101\\035:\\314\\233\\246\\275\\207\\v\\\\\\\\\\343\\202\\026\\274\\037\\213\\314\\303V\\2116<*+\\352\\255\\026\\204M\\332\\340\\nT\\306\\360Pa\\304\\002\\024\\356\\247\\220\\263\\257l\\230\\350\\362\\027Yl\\300\\316lk\\232\\f\\333\\235,\\234\\336\\204+JE\\263>\\237\\177\\305z\\017\\252\\311@\\341e\\026\\300dk\\305\\212V\\334\\377\\004cb\\217p\\n\\307w\\0046\\2261|\\024\\\\N\\336\\223G\\263~n\\271\\256\\264\\346\\227\\301\\032\\301\\030\\bw\\241Z.\\302\\261\\027 \\005\\374B\\357R\\231\\237Ct\\324\\232I\\322\\370\\303T\\253\\205\\354\\233\\337\\031C\\3215O\\3746\\016\\360b &\\300\\201\\244U\\221\\306 Pql+c\\212\\2524\\314a\\t\\216\\275=\\230b\\207ouv[\\365(\\3...
512025-05-20T18:44:47Z [ipc] {bitcoin-node-948798/b-shutoff-948798} IPC client recv ChainNotifications.chainStateFlushed$Results ()
522025-05-20T18:44:47Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server destroy N2mp11ProxyServerIN3ipc5capnp8messages5ChainEEE
532025-05-20T18:44:47Z [ipc] {bitcoin-node-948798/b-capnp-loop-948800} IPC server destroy N2mp11ProxyServerIN3ipc5capnp8messages4InitEEE
54Segmentation fault

ryanofsky commented at 2:33 am on May 21, 2025: contributor

re: #32345 (comment)

I found that bitcoin-node will (still) segfault if i try to stop it while the client is running.

Thanks for testing this! I created a new issue in https://github.com/bitcoin-core/libmultiprocess/issues/176 to help debug. So far I couldn’t reproduce the problem running a very basic client there, but I can try to extend it to do more of the operations your client is doing and see if I can make the problem happen. It would also help to have a stack trace from the segfault, and I provided instructions for getting a stack trace there.

ryanofsky force-pushed on Jun 5, 2025

ryanofsky commented at 5:50 pm on June 5, 2025: contributor

Rebased 5b38e62ccbfcdb7f20a6a74fcc4ef2358e3da56c -> c1cce04f763e88347cf025fab76b894b4b015dab (pr/ipc-stop.4 -> pr/ipc-stop.5, compare) due to conflicts with #32641 and #32438 Updated c1cce04f763e88347cf025fab76b894b4b015dab -> a180915d3604a2bff14040ccbfd67ee586a8e212 (pr/ipc-stop.5 -> pr/ipc-stop.6, compare) to fix https://github.com/bitcoin-core/libmultiprocess/issues/176 Updated a180915d3604a2bff14040ccbfd67ee586a8e212 -> d2b1e821d48f4d33d5015f3ea3064a26788a3e3c (pr/ipc-stop.6 -> pr/ipc-stop.7, compare) to fix more test problems with globals in node_init_tests https://cirrus-ci.com/task/4606963511721984

DrahtBot removed the label Needs rebase on Jun 5, 2025

ryanofsky force-pushed on Jun 6, 2025

DrahtBot removed the label CI failed on Jun 6, 2025

ryanofsky force-pushed on Jun 13, 2025

ryanofsky commented at 8:08 pm on June 13, 2025: contributor

Rebased d2b1e821d48f4d33d5015f3ea3064a26788a3e3c -> 49646814a40cc2b4199e3e4fee60ac49da468f5d (pr/ipc-stop.7 -> pr/ipc-stop.8, compare) on top of updated base PR https://github.com/bitcoin-core/libmultiprocess/pull/160 pr/eventlock.7 to resolve more issues reported in https://github.com/bitcoin-core/libmultiprocess/pull/182 Rebased 49646814a40cc2b4199e3e4fee60ac49da468f5d -> 616a09631ffb7f7c7e8746957289bdfd4e734a4e (pr/ipc-stop.8 -> pr/ipc-stop.9, compare) on top of updated base PR https://github.com/bitcoin-core/libmultiprocess/pull/160 pr/eventlock.8 to fix spurious thread safety warning https://cirrus-ci.com/task/4956347122319360 Rebased 616a09631ffb7f7c7e8746957289bdfd4e734a4e -> 53b12cac6badc2746e0fbb8f94eae5fe84fc842b (pr/ipc-stop.9 -> pr/ipc-stop.10, compare) on top of updated base R https://github.com/bitcoin-core/libmultiprocess/pull/160 pr/eventlock.9 to fix spurious thread safety warning https://cirrus-ci.com/task/5066891695226880

DrahtBot added the label CI failed on Jun 13, 2025

DrahtBot commented at 11:51 pm on June 13, 2025: contributor

🚧 At least one of the CI tasks failed. Task multiprocess, i686, DEBUG: https://github.com/bitcoin/bitcoin/runs/44074743936 LLM reason (✨ experimental): The CI failure is caused by a compile error due to an unheld mutex lock in src/ipc/libmultiprocess/src/mp/proxy.cpp.

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.

ryanofsky force-pushed on Jun 16, 2025

DrahtBot removed the label CI failed on Jun 16, 2025

pinheadmz commented at 3:24 pm on June 17, 2025: member

concept ACK, solved an edge case I had with #32297

ryanofsky referenced this in commit 258a617c1e on Jun 19, 2025

ryanofsky force-pushed on Jun 24, 2025

ryanofsky commented at 5:13 pm on June 24, 2025: contributor

Rebased 53b12cac6badc2746e0fbb8f94eae5fe84fc842b -> 3ed3f7f979fb8e4cf3023deb64acf9b0a545057a (pr/ipc-stop.10 -> pr/ipc-stop.11, compare) with no changes after https://github.com/bitcoin-core/libmultiprocess/pull/160 merge

Will change status from draft -> ready for review

ryanofsky marked this as ready for review on Jun 24, 2025

DrahtBot added the label CI failed on Jun 24, 2025

DrahtBot commented at 7:03 pm on June 24, 2025: contributor

🚧 At least one of the CI tasks failed. Task multiprocess, i686, DEBUG: https://github.com/bitcoin/bitcoin/runs/44704578313 LLM reason (✨ experimental): The CI failure is due to a test timeout during execution.

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.

DrahtBot removed the label CI failed on Jun 24, 2025

DrahtBot added the label CI failed on Jun 25, 2025

DrahtBot removed the label CI failed on Jun 25, 2025

maflcko commented at 8:29 am on June 25, 2025: member

the CI failure is real, can manifest itself via segfault, timeout, abort, assertion failure, …

Sjors commented at 8:32 am on June 25, 2025: member

I’ve been seeing segfaults etc too in https://github.com/Sjors/bitcoin/pull/90. I’m running it again now with the most recent versions of this and https://github.com/bitcoin-core/libmultiprocess/pull/184 (and #31802 for increased CI coverage). Update: yup, more segfaults.

in src/ipc/interfaces.cpp:35 in 3ed3f7f979 outdated

26@@ -26,6 +27,27 @@
27 
28 namespace ipc {
29 namespace {
30+#ifndef WIN32
31+std::string g_ignore_ctrl_c;
32+
33+void HandleCtrlC(int)
34+{
35+    (void)write(STDOUT_FILENO, g_ignore_ctrl_c.data(), g_ignore_ctrl_c.size());

Sjors commented at 7:31 am on June 27, 2025:

CI on https://github.com/Sjors/bitcoin/pull/90#issuecomment-3011857912 complains of an unused result. Which is odd because the (void) cast should prevent such a warning. This seems to be a gcc thing: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425

ryanofsky commented at 3:18 am on July 1, 2025:

re: #32345 (review)

CI on Sjors#90 (comment) complains of an unused result. Which is odd because the (void) cast should prevent such a warning. This seems to be a gcc thing: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=66425

Thanks! I tried to work around this by adding a ! though reading gcc issue maybe that will not be sufficient.

Sjors commented at 7:32 am on July 1, 2025:

Ok, we’ll find out, if CI passes in the latest https://github.com/Sjors/bitcoin/pull/90

Sjors commented at 9:07 am on July 1, 2025:

Narrator: it does

in src/ipc/interfaces.cpp:21 in 3ed3f7f979 outdated

17@@ -18,6 +18,7 @@
18 #include <cstring>
19 #include <functional>
20 #include <memory>
21+#include <signal.h>

Sjors commented at 7:47 am on June 27, 2025:

<csignal>

ryanofsky commented at 3:13 am on July 1, 2025:

re: #32345 (review)

<csignal>

Thanks, fixed

ryanofsky force-pushed on Jul 1, 2025

ryanofsky commented at 3:21 am on July 1, 2025: contributor

Rebased 3ed3f7f979fb8e4cf3023deb64acf9b0a545057a -> 5024f552924b214fa8a092c1daf9acf92a4c40f0 (pr/ipc-stop.11 -> pr/ipc-stop.12, compare) with update to fix tidy errors and try to incorporate https://github.com/bitcoin-core/libmultiprocess/pull/186 Rebased 5024f552924b214fa8a092c1daf9acf92a4c40f0 -> b06a204dc0f64e3b30f3537e07a5fe1e0c8791c6 (pr/ipc-stop.12 -> pr/ipc-stop.13, compare) on latest https://github.com/bitcoin-core/libmultiprocess/pull/186 and adding test fix 9c7defac7f3965e782d3c6cf250b044e19d5146b forgot to include in previous push Updated b06a204dc0f64e3b30f3537e07a5fe1e0c8791c6 -> d62ec01e3d15865762fad671611555cce5315016 (pr/ipc-stop.13 -> pr/ipc-stop.14, compare) adding comment to explain (void)! Squashed d62ec01e3d15865762fad671611555cce5315016 -> a67f3b07d4a1ea9289d4f7d5d3536fe2033b67b4 (pr/ipc-stop.14 -> pr/ipc-stop.15, compare) after realizing 9c7defac7f3965e782d3c6cf250b044e19d5146b was fixing a bug introduced in an earlier commit

Marked as a draft since this now depends on https://github.com/bitcoin-core/libmultiprocess/pull/186

ryanofsky marked this as a draft on Jul 1, 2025

in src/ipc/interfaces.cpp:36 in 5024f55292 outdated

26@@ -26,6 +27,27 @@
27 
28 namespace ipc {
29 namespace {
30+#ifndef WIN32
31+std::string g_ignore_ctrl_c;
32+
33+void HandleCtrlC(int)
34+{
35+    (void)!write(STDOUT_FILENO, g_ignore_ctrl_c.data(), g_ignore_ctrl_c.size());

Sjors commented at 7:24 am on July 1, 2025:

Maybe comment that the ! is for gcc

ryanofsky commented at 4:38 pm on July 1, 2025:

re: #32345 (review)

Maybe comment that the ! is for gcc

Makes sense, added comment

ryanofsky force-pushed on Jul 1, 2025

ryanofsky referenced this in commit a11e6905c2 on Jul 1, 2025

ryanofsky force-pushed on Jul 1, 2025

ryanofsky commented at 7:13 pm on July 1, 2025: contributor

Rebased a67f3b07d4a1ea9289d4f7d5d3536fe2033b67b4 -> 0a9a1940782fad8dc3494c4aacee0a06c030eb5c (pr/ipc-stop.15 -> pr/ipc-stop.16, compare) after https://github.com/bitcoin-core/libmultiprocess/pull/186 merge. Marking ready for review again

Sjors commented at 10:37 am on July 9, 2025: member

Concept ACK

It’s still marked as draft though.

ryanofsky marked this as ready for review on Jul 9, 2025

ryanofsky commented at 1:10 pm on July 9, 2025: contributor

It’s still marked as draft though.

Thanks, fixed!

in src/ipc/capnp/protocol.cpp:108 in f8fd3959d5 outdated

89@@ -96,6 +90,7 @@ class CapnpProtocol : public Protocol
90     Context m_context;
91     std::thread m_loop_thread;
92     std::optional<mp::EventLoop> m_loop;
93+    std::optional<mp::EventLoopRef> m_loop_ref;

Sjors commented at 12:25 pm on July 10, 2025:

In f8fd3959d51f6f80b428828c4fd965e06a8fa19e ipc: Use EventLoopRef instead of addClient/removeClient: if you have to retouch it would be good to document m_loop and m_loop_ref.

ryanofsky commented at 10:39 am on August 8, 2025:

re: #32345 (review)

In f8fd395 ipc: Use EventLoopRef instead of addClient/removeClient: if you have to retouch it would be good to document m_loop and m_loop_ref.

Good idea, documented both

Sjors approved

Sjors commented at 12:40 pm on July 10, 2025: member

utACK 0a9a1940782fad8dc3494c4aacee0a06c030eb5c

DrahtBot requested review from pinheadmz on Jul 10, 2025

DrahtBot requested review from darosior on Jul 10, 2025

ismaelsadeeq commented at 2:20 pm on August 1, 2025: member

Concept ACK

The unclean shutdown I noticed while looking at #30437 is gone after I rebase the PR on top of this.

Squashed 'src/ipc/libmultiprocess/' changes from 27c7e8e5a581..b4120d34bad2

b4120d34bad2 Merge bitcoin-core/libmultiprocess#192: doc: fix typos
6ecbdcd35a93 doc: fix typos
a11e6905c238 Merge bitcoin-core/libmultiprocess#186: Fix mptest failures in bitcoin CI
6f340a583f2b doc: fix DrahtBot LLM Linter error
c6f7fdf17350 type-context: revert client disconnect workaround
e09143d2ea2f proxy-types: fix UndefinedBehaviorSanitizer: null-pointer-use
84b292fcc4db mptest: fix MemorySanitizer: use-of-uninitialized-value
fe4a188803c6 proxy-io: fix race conditions in disconnect callback code
d8011c83608e proxy-io: fix race conditions in ProxyClientBase cleanup handler
97e82ce19c47 doc: Add note about Waiter::m_mutex and interaction with the EventLoop::m_mutex
81d58f5580e8 refactor: Rename ProxyClient cleanup_it variable
07230f259f55 refactor: rename ProxyClient<Thread>::m_cleanup_it
c0efaa5e8cb1 Merge chaincodelabs/libmultiprocess#187: ci: have bash scripts explicitly opt out of locale dependence.
0d986ff144cd mptest: fix race condition in TestSetup constructor
d2f6aa2e84ef ci: add thread sanitizer job
3a6db38e561f ci: rename configs to .bash
401e0ce1d9c3 ci: add copyright to bash scripts
e956467ae464 ci: export LC_ALL
8954cc0377d8 Merge chaincodelabs/libmultiprocess#184: Add CI jobs and fix clang-tidy and iwyu errors
757e13a75546 ci: add gnu32 cross-compiled 32-bit build
15bf349000eb doc: fix typo found by DrahtBot
1a598d5905f7 clang-tidy: drop 'bitcoin-*' check
cbb1e43fdc6e ci: test libc++ instead of libstdc++ in one job
76313450c2c4 type-context: disable clang-tidy UndefinedBinaryOperatorResult error
4896e7fe51ba proxy-types: fix clang-tidy EnumCastOutOfRange error
060a73926956 proxy-types: fix clang-tidy StackAddressEscape error
977d721020f6 ci: add github actions jobs testing gcc, clang-20, clang-tidy, and iwyu
0d5f1faae5da iwyu: fix add/remove include errors
753d2b10cc27 util: fix clang-tidy modernize-use-equals-default error
ae4f1dc2bb1a type-number: fix clang-tidy modernize-use-nullptr error
07a741bf6946 proxy-types: fix clang-tidy bugprone-use-after-move error
3673114bc9d9 proxy-types: fix clang-tidy bugprone-use-after-move error
422923f38485 proxy-types: fix clang-tidy bugprone-use-after-move error
c6784c6adefa mpgen: disable clang-tidy misc-no-recursion error
c5498aa11ba6 tidy: copy clang-tidy file from bitcoin core
258a617c1eec Merge chaincodelabs/libmultiprocess#160: refactor: EventLoop locking cleanups + client disconnect exception
84cf56a0b5f4 test: Test disconnects during IPC calls
949573da8411 Prevent IPC server crash if disconnected during IPC call
019839758085 Merge chaincodelabs/libmultiprocess#179: scripted-diff: Remove copyright year (ranges)
ea38392960e1 Prevent EventLoop async cleanup thread early exit during shutdown
616d9a75d20a doc: Document ProxyClientBase destroy_connection option
56fff76f940b Improve IPC client disconnected exceptions
9b8ed3dc5f87 refactor: Add clang thread safety annotations to EventLoop
52256e730f51 refactor: Remove DestructorCatcher and AsyncCallable
f24894794adf refactor: Drop addClient/removeClient methods
2b830e558e61 refactor: Use EventLoopRef instead of addClient/removeClient
315ff537fb65 refactor: Add ProxyContext EventLoop* member
9aaeec3678d3 proxy-io.h: Add EventLoopRef RAII class handle addClient/removeClient refcounting
f58c8d8ba2f0 proxy-io.h: Add more detailed EventLoop comment
5108445e5d16 test: Add test coverage for client & server disconnections
59030c68cb5f Merge chaincodelabs/libmultiprocess#181: type-function.h: Fix CustomBuildField overload
688140b1dffc test: Add coverage for type-function.h
8b96229da58e type-function.h: Fix CustomBuildField overload
fa2ff9a66842 scripted-diff: Remove copyright year (ranges)

git-subtree-dir: src/ipc/libmultiprocess
git-subtree-split: b4120d34bad2de28141c5770f6e8df8e54898987

e886c65b6b

Merge commit 'e886c65b6b37aaaf5d22ca68bc14e55d8ec78212' into pr/ipc-stop-base 5c45bc989b

ipc: Use EventLoopRef instead of addClient/removeClient

Use EventLoopRef to avoid reference counting bugs and be more exception safe
and deal with removal of addClient/removeClient methods in
https://github.com/bitcoin-core/libmultiprocess/pull/160

A test update is also required due to
https://github.com/bitcoin-core/libmultiprocess/pull/160 to deal with changed
reference count semantics. In IpcPipeTest(), it is now necessary to destroy
the client Proxy object instead of just the client Connection object to
decrease the event loop reference count and allow the loop to exit so the test
does not hang on shutdown.

9a9fb19536

test: Add unit test coverage for Init and Shutdown code

Currently this code is not called in unit tests. Calling should make it
possible to write tests for things like IPC exceptions being thrown during
shutdown.

6eb09fd614

ipc: Avoid waiting for clients to disconnect when shutting down

This fixes behavior reported by Antoine Poinsot <darosior@protonmail.com>
https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2546088852 where if
an IPC client is connected, the node will wait forever for it to disconnect
before exiting.

7f65aac78b

doc: Improve IPC interface comments

Fix some comments that were referring to previous versions of these methods and
did not make sense.

0c28068ceb

ipc: Add Ctrl-C handler for spawned subprocesses

This fixes an error reported by Antoine Poinsot <darosior@protonmail.com> in
https://github.com/bitcoin-core/libmultiprocess/issues/123 that does not happen
in master, but does happen with https://github.com/bitcoin/bitcoin/pull/10102
applied, where if Ctrl-C is pressed when `bitcoin-node` is started, it is
handled by both `bitcoin-node` and `bitcoin-wallet` processes, causing the
wallet to shutdown abruptly instead of waiting for the node and shutting down
cleanly.

This change fixes the problem by having the wallet process print to stdout when
it receives a Ctrl-C signal but not otherwise react, letting the node shut
everything down cleanly.

2160995916

ipc: Handle bitcoin-wallet disconnections

This fixes an error reported by Antoine Poinsot <darosior@protonmail.com> in
https://github.com/bitcoin-core/libmultiprocess/issues/123 that does not happen
in master, but does happen with https://github.com/bitcoin/bitcoin/pull/10102
applied, where if the child bitcoin-wallet process is killed (either by an
external signal or by Ctrl-C as reported in the issue) the bitcoin-node process
will not shutdown cleanly after that because chain client stop()
calls will fail.

This change fixes the problem by handling ipc::Exception errors thrown during
the stop() calls, and it relies on the fixes to disconnect detection
implemented in https://github.com/bitcoin-core/libmultiprocess/pull/160 to work
effectively.

2581258ec2

in src/rpc/server.h:32 in 637502894d outdated

29@@ -30,6 +30,7 @@ void RpcInterruptionPoint();
30  */
31 void SetRPCWarmupStatus(const std::string& newStatus);
32 /* Mark warmup as done.  RPC calls will be processed from now on.  */
33+void SetRPCWarmupStarting();

pinheadmz commented at 7:00 pm on August 6, 2025:

637502894db32a1d532601ecc70d18f9b8a35f19

nit: comment above should stay attached to line below

ryanofsky commented at 10:40 am on August 8, 2025:

re: #32345 (review)

6375028

nit: comment above should stay attached to line below

Good catch, moved comment

in src/test/node_init_tests.cpp:37 in 637502894d outdated

32+
33+BOOST_AUTO_TEST_CASE(init_test)
34+{
35+    // Reset logging and config file path to avoid errors in AppInitMain
36+    LogInstance().DisconnectTestLogger();
37+    m_node.args->SetConfigFilePath({});

pinheadmz commented at 7:31 pm on August 6, 2025:

637502894db32a1d532601ecc70d18f9b8a35f19

Confirmed the test failure by removing these two lines, but that kinda surprised me because I assumed the test environment gets reset between modules anyway…?

0Assertion failed: (m_buffering), function StartLogging, file logging.cpp, line 56.

ryanofsky commented at 10:40 am on August 8, 2025:

re: #32345 (review)

6375028

Confirmed the test failure by removing these two lines, but that kinda surprised me because I assumed the test environment gets reset between modules anyway…?

This isn’t about resetting the test environment between tests, it’s more about conflicts between AppInitMain and BasicTestingSetup. AppInitMain was never called in unit tests before this PR, and it expects some variables to be unset which BasicTestingSetup sets. I expanded the comment to help clarify. Thanks for testing to confirm the code.

in src/ipc/capnp/protocol.cpp:79 in ff1c9278e7 outdated

74+        if (!m_loop) return;
75+        // Delete incoming connections, except the connection to a parent
76+        // process (if there is one), since a parent process should be able to
77+        // monitor and control this process, even during shutdown.
78+        m_loop->sync([&] {
79+            m_loop->m_incoming_connections.remove_if([this](mp::Connection& c) { return &c != m_parent_connection; });

pinheadmz commented at 7:43 pm on August 6, 2025:

ff1c9278e7aee8be094c63e90ca37b644f55cc7a

If there is no parent connection does that mean for sure there are no other incoming connections? The implication on line 68 is that the first incoming connection is always the parent process which skips its removal.

ryanofsky commented at 10:41 am on August 8, 2025:

re: #32345 (review)

ff1c927

If there is no parent connection does that mean for sure there are no other incoming connections?

No and in the typical case where bitcoin-node is run with -ipcbind, there will be no parent connection but incoming connections will be allowed. A parent connection only exists when a process is spawned by another, such as the GUI launching a node, the node launching a wallet, or one node spawning another during the echoipc test.

The implication on line 68 is that the first incoming connection is always the parent process which skips its removal.

It practice the parent connection, if there is one, will always be the last connection in the m_incoming_connections list. But the code is trying not to assume anything about ordering, because the list is a std::list and connections can be inserted and removed in any order. They are currently inserted at the front, but this was an arbitrary choice.

Line 68 m_parent_connection = &m_loop->m_incoming_connections.back(); is saving a pointer to the parent connection using back() method, but it could also use the front() method since at that point the parent connection should be the only connection in the list.

Line 79 goes through the list deleting every connection that’s not the parent connection, regardless of the order of connections.

I documented m_parent_connection member now to provide more context, but let me know if you think anything else should be cleared up.

pinheadmz approved

pinheadmz commented at 8:11 pm on August 6, 2025: member

ACK 0a9a194078

Built and tested on macos/arm64, reviewed each commit. Adds a unit test for node initialization, refactors IPC on some upstream changes in libmultiprocess, and ensures a clean shutdown when IPC clients are connected. I’m going to locally rebase #32297 on this and try to break it again but I suspect it’ll hold up a lot better.

DrahtBot requested review from ismaelsadeeq on Aug 6, 2025

fanquake commented at 9:07 am on August 7, 2025: member

node_init_tests will fail, if there is a bitcoind running. Not sure that’s expected, or it’s documented, that it will no-longer be possible to run the unit tests, if you’re also running bitcoind:

 0../../../src/test/node_init_tests.cpp:14: Entering test suite "node_init_tests"
 1../../../src/test/node_init_tests.cpp:33: Entering test case "init_test"
 22025-08-07T08:49:40.080515Z [unknown] [../../../../src/test/util/random.cpp:48] [void SeedRandomStateForTest(SeedRand)] Setting random seed for current tests to RANDOM_CTX_SEED=ae37739d7589272b8f3a4e318a3c7a0b54555b9c17617f8ca2f0626710060c04
 32025-08-07T08:49:40.080968Z [test] [../../src/init/common.cpp:152] [void init::LogPackageVersion()] Bitcoin Core version v29.99.0-fcb3fe8e95f7 (release build)
 42025-08-07T08:49:40.081144Z [test] [../../src/kernel/context.cpp:20] [auto kernel::Context::Context()::(anonymous class)::operator()() const] Using the 'arm_shani(1way,2way)' SHA256 implementation
 5Warning: Disk space for "/var/folders/sq/z88fhjzj0b19ftsd2_bjrmjm0000gn/T/test_common bitcoin/node_init_tests/init_test/73da3751c4c629eaf48a/blocks" may not accommodate the block files. Approximately 720 GB of data will be stored in this directory.
 6Error: Unable to bind to 127.0.0.1:8334 on this computer. Bitcoin Core is probably already running.
 7Error: Failed to listen on any port. Use -listen=0 if you want this.
 8../../../src/test/node_init_tests.cpp:42: error: in "node_init_tests/init_test": check AppInitMain(m_node) has failed
 9../../../src/test/node_init_tests.cpp:33: Leaving test case "init_test"; testing time: 3241883us
10../../../src/test/node_init_tests.cpp:14: Leaving test suite "node_init_tests"; testing time: 3241893us
11<snip>
12147/148 Test   [#8](/bitcoin-bitcoin/8/): bench_sanity_check ...................   Passed   21.36 sec
13148/148 Test   [#5](/bitcoin-bitcoin/5/): secp256k1_tests ......................   Passed   31.95 sec
14
1599% tests passed, 1 tests failed out of 148
16
17Total Test time (real) =  31.96 sec
18
19The following tests did not run:
20	 89 - script_assets_tests (Skipped)
21
22The following tests FAILED:
23	 69 - node_init_tests (Failed)
24Errors while running CTest

ryanofsky force-pushed on Aug 8, 2025

ryanofsky commented at 12:41 pm on August 8, 2025: contributor

Rebased 0a9a1940782fad8dc3494c4aacee0a06c030eb5c -> 2581258ec200efb173ea6449ad09b2e7f1cc02e0 (pr/ipc-stop.16 -> pr/ipc-stop.17, compare) fixing test/bitcoind conflict, improving documentation and pulling in upstream change https://github.com/bitcoin-core/libmultiprocess/pull/192

re: #32345 (comment)

node_init_tests will fail, if there is a bitcoind running. Not sure that’s expected, or it’s documented, that it will no-longer be possible to run the unit tests, if you’re also running bitcoind

Thanks, this was unexpected and latest push fixes it by force-setting -listen=0.

pinheadmz approved

pinheadmz commented at 5:29 pm on August 8, 2025: member

re-ACK 2581258ec200efb173ea6449ad09b2e7f1cc02e0

Minimal changes since last ack include addressing nits and fixing a conflict between bitcoind and the unit test (which I confirmed locally, was an issue before fix in latest push). Tested again with #32297.

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA256
 2
 3ACK 2581258ec200efb173ea6449ad09b2e7f1cc02e0
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmiWM0UACgkQ5+KYS2KJ
 7yTos7A//YBDIR7W6ms2s0uQpSLF1Z2J1yt/+qVUx0AAU0z171yeZZr/vaB55hDL6
 8Et1Vib1hfbtapeJN86JpRTGXS+dvwkWSVAsLB/W8xnxQuGBxcZdg06nPpJOSlZdh
 94iDCz29AUMpN4DCgz4/Q1C1rGQdGVoAq4ivlbzoHehPwrN4f8dU//DQyQUG9pQEr
10NJP6UlpJ62hEKctoc4jqp5eiFLOYhNICJgy2n0Hx51+od2NMBrHGEOH+2JWxIN34
11K8n4tENkrpeazFcv1+Q5r6q205N6IctYJUTKW4LXmftiEh1iH1lgknVF45veNDLw
121/UJ9t1mw+mE2026SO4bKspoDDHYV29ncDtQh6Z4viAcIeS63gJkV+jQhuPrtUbH
13knlMQvcOarncv981vDsGO2Ny1YwbOdqgmpIHdq/J8f4+FoEym8FPgNvSdy1LnG5K
14lmY0r6jnY45+1PNkVInPVtPLMpQB0OvypzIqqdlnLZmRD9t+EB4H92DCBi0bUJfP
15PZg0lCNpPub1o1SNyVwKUoy1dsbFf81jaEyHkW5tPT7w1WpzJrea4AaP1GGCO79V
16GZejEMIGt2dWY43gg7bC/D8AVnMEC/eG7MVjKJdZP6NDd8KjoyevgDfF4Jb77JzK
17tkBHnplucV1GrMikD3eoU3T4c8/4mE8ESvsTj9V76dQZIEHOu+0=
18=E1lO
19-----END PGP SIGNATURE-----

pinheadmz’s public key is on openpgp.org

DrahtBot requested review from Sjors on Aug 8, 2025

Sjors commented at 3:04 pm on August 12, 2025: member

re-utACK 2581258ec200efb173ea6449ad09b2e7f1cc02e0

josibake commented at 2:26 pm on August 15, 2025: member

code review ACK https://github.com/bitcoin/bitcoin/commit/2581258ec200efb173ea6449ad09b2e7f1cc02e0

Code reviewed and have done some minimal testing, mostly to try and reproduce unclean shutdowns and then verify that they are fixed in this PR.

As a general comment, I am a bit surprised that most of the issues people have been finding seem to be through manual testing and are not being caught in CI. Not sure what a concrete next step is, but I do plan to spend some time looking at the the issues reviewers have been finding during the multiprocess testing to see if there is a need for some more general tests in the CI that would have helped surface these issues much earlier.

Sjors commented at 4:22 pm on August 15, 2025: member

@josibake this project didn’t have CI until recently. It’s been introduced and expanded as the bugs started popping up, as well as by things the Bitcoin Core CI sanitiser found as I kept rebasing #31802.

fanquake merged this on Aug 18, 2025

fanquake closed this on Aug 18, 2025

hebasto commented at 6:01 pm on August 19, 2025: member

native_libmultiprocess fails to build on OpenBSD.

in src/init.cpp:306 in 2581258ec2

298@@ -298,6 +299,14 @@ void Shutdown(NodeContext& node)
299     StopREST();
300     StopRPC();
301     StopHTTPServer();
302+    for (auto& client : node.chain_clients) {
303+        try {
304+            client->stop();
305+        } catch (const ipc::Exception& e) {
306+            LogDebug(BCLog::IPC, "Chain client did not disconnect cleanly: %s", e.what());

maflcko commented at 9:27 am on August 21, 2025:

should this really be hidden by default? It seems like it should only happen unexpectedly, in which case a LogWarning may be better, or at least LogInfo?

alexanderwiederin referenced this in commit 67b164b083 on Aug 21, 2025

in src/test/node_init_tests.cpp:34 in 2581258ec2

29+    }
30+    NodeContext& m_node;
31+};
32+
33+BOOST_AUTO_TEST_CASE(init_test)
34+{

maflcko commented at 3:47 pm on August 22, 2025:

This seems to fail the integer sanitizer: https://cirrus-ci.com/task/6018134475276288?logs=check#L211:

 0test/node_init_tests.cpp(14): Entering test suite "node_init_tests"
 1test/node_init_tests.cpp(33): Entering test case "init_test"
 22025-08-21T19:53:08.170490Z [unknown] [test/util/random.cpp:48] [void SeedRandomStateForTest(SeedRand)] Setting random seed for current tests to RANDOM_CTX_SEED=ad2ae1d1b241a8a415874d45b2e104b01c396db4c21ec0899e66d0c85db1d978
 32025-08-21T19:53:08.174281Z [test] [init/common.cpp:153] [void init::LogPackageVersion()] Bitcoin Core version v29.99.0-8333aa530290 (release build)
 42025-08-21T19:53:08.174793Z [test] [kernel/context.cpp:20] [auto kernel::Context::Context()::(anonymous class)::operator()() const] Using the 'x86_shani(1way,2way)' SHA256 implementation
 52025-08-21T19:53:08.174846Z [test] [random.cpp:110] [void (anonymous namespace)::ReportHardwareRand()] Using RdSeed as an additional entropy source
 62025-08-21T19:53:08.174872Z [test] [random.cpp:113] [void (anonymous namespace)::ReportHardwareRand()] Using RdRand as an additional entropy source
 7Warning: Disk space for "/tmp/test_common bitcoin/node_init_tests/init_test/96e0d4146db9047f2d08/blocks" may not accommodate the block files. Approximately 720 GB of data will be stored in this directory.
 8/tmp/cirrus-ci-build/bitcoin-core/src/common/netif.cpp:127:29: runtime error: unsigned integer overflow: 20 - 28 cannot be represented in type 'unsigned long'
 9    [#0](/bitcoin-bitcoin/0/) 0x56171608ff27 in (anonymous namespace)::QueryDefaultGatewayImpl(unsigned short) /tmp/cirrus-ci-build/bitcoin-core/bld/src/./common/netif.cpp:127:29
10    [#1](/bitcoin-bitcoin/1/) 0x56171608ff27 in QueryDefaultGateway(Network) /tmp/cirrus-ci-build/bitcoin-core/bld/src/./common/netif.cpp:276:35
11    [#2](/bitcoin-bitcoin/2/) 0x561714e4104d in ProcessPCP() /tmp/cirrus-ci-build/bitcoin-core/bld/src/./mapport.cpp:87:44
12    [#3](/bitcoin-bitcoin/3/) 0x561714e4104d in ThreadMapPort() /tmp/cirrus-ci-build/bitcoin-core/bld/src/./mapport.cpp:125:9
13    [#4](/bitcoin-bitcoin/4/) 0x561716341286 in std::function<void ()>::operator()() const /usr/lib/gcc/x86_64-linux-gnu/15/../../../../include/c++/15/bits/std_function.h:593:9
14    [#5](/bitcoin-bitcoin/5/) 0x561716341286 in util::TraceThread(std::basic_string_view<char, std::char_traits<char>>, std::function<void ()>) /tmp/cirrus-ci-build/bitcoin-core/bld/src/util/./util/thread.cpp:21:9
15    [#6](/bitcoin-bitcoin/6/) 0x561714e42a68 in void std::__invoke_impl<void, void (*)(std::basic_string_view<char, std::char_traits<char>>, std::function<void ()>), char const*, void (*)()>(std::__invoke_other, void (*&&)(std::basic_string_view<char, std::char_traits<char>>, std::function<void ()>), char const*&&, void (*&&)()) /usr/lib/gcc/x86_64-linux-gnu/15/../../../../include/c++/15/bits/invoke.h:63:14
16    [#7](/bitcoin-bitcoin/7/) 0x7c9ad8b6c583  (/lib/x86_64-linux-gnu/libstdc++.so.6+0xf2583) (BuildId: b5a12b1c2e09b4a53c7c2c63ce3cbe2ee0b45eb6)
17    [#8](/bitcoin-bitcoin/8/) 0x5617135ed2fa in asan_thread_start(void*) asan_interceptors.cpp.o
18    [#9](/bitcoin-bitcoin/9/) 0x7c9ad87b9da3  (/lib/x86_64-linux-gnu/libc.so.6+0xa3da3) (BuildId: 3efb08fbd4ca116597b0d0dacdd264f179e702b8)
19    [#10](/bitcoin-bitcoin/10/) 0x7c9ad884d47b  (/lib/x86_64-linux-gnu/libc.so.6+0x13747b) (BuildId: 3efb08fbd4ca116597b0d0dacdd264f179e702b8)
20SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /tmp/cirrus-ci-build/bitcoin-core/src/common/netif.cpp:127:29

However, I can’t reproduce locally, so it may be related to the Linux version, which is https://cirrus-ci.com/task/6018134475276288?logs=info#L6:

0Linux 6.6.93+

Would be nice to check if anyone can reproduce this.

ryanofsky commented at 4:03 pm on August 22, 2025:

re: #32345 (review)

Probably the node_init test (or maybe the unit test framework) should be disabling -natpmp after #33004 to prevent this code from running. But maybe there is a real bug on this line if it is subtracting 20 - 28:

https://github.com/bitcoin/bitcoin/blob/73220fc0f958f9b65f66cf0cf042af220b312fc6/src/common/netif.cpp#L127

ryanofsky commented at 4:39 pm on August 22, 2025:

FWIW chatgpt suggested the following fix for the netlink code. No idea if it is correct:

 0--- a/src/common/netif.cpp
 1+++ b/src/common/netif.cpp
 2@@ -123,6 +123,17 @@ std::optional<CNetAddr> QueryDefaultGatewayImpl(sa_family_t family)
 3     }
 4 
 5     for (nlmsghdr* hdr = (nlmsghdr*)response; NLMSG_OK(hdr, recv_result); hdr = NLMSG_NEXT(hdr, recv_result)) {
 6+        // Netlink multipart dumps include control messages; ignore them.
 7+        if (hdr->nlmsg_type == NLMSG_DONE) {
 8+            break; // end of dump
 9+        }
10+        if (hdr->nlmsg_type == NLMSG_ERROR) {
11+            continue; // optional: parse nlmsgerr if desired
12+        }
13+        if (hdr->nlmsg_type != RTM_NEWROUTE) {
14+            continue; // not a route message, don't treat payload as rtmsg
15+        }
16+
17         rtmsg* r = (rtmsg*)NLMSG_DATA(hdr);
18         int remaining_len = RTM_PAYLOAD(hdr);
19

https://chatgpt.com/share/68a89d12-1764-800a-859e-2ffdc1ab6836

maflcko commented at 4:44 pm on August 22, 2025:

#33004

Ah. I somehow assumed that the ISan warning was specific to this unit test and does not reproduce outside of it. However, I can now reproduce it outside:

 0root@ubuntu-16gb-hel1-1:~/b-c# sysctl -w net.ipv6.conf.all.disable_ipv6=1
 1net.ipv6.conf.all.disable_ipv6 = 1
 2root@ubuntu-16gb-hel1-1:~/b-c# UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1" ./bld-cmake/bin/bitcoind -regtest -noprinttoconsole=1 -natpmp=1
 3/root/b-c/src/common/netif.cpp:127:29: runtime error: unsigned integer overflow: 20 - 28 cannot be represented in type 'unsigned long'
 4    [#0](/bitcoin-bitcoin/0/) 0x5f8c692ea4b7 in (anonymous namespace)::QueryDefaultGatewayImpl(unsigned short) /root/b-c/bld-cmake/src/./common/netif.cpp:127:29
 5    [#1](/bitcoin-bitcoin/1/) 0x5f8c692ea4b7 in QueryDefaultGateway(Network) /root/b-c/bld-cmake/src/./common/netif.cpp:276:35
 6    [#2](/bitcoin-bitcoin/2/) 0x5f8c68c1b32a in ProcessPCP() /root/b-c/bld-cmake/src/./mapport.cpp:87:44
 7    [#3](/bitcoin-bitcoin/3/) 0x5f8c68c1b32a in ThreadMapPort() /root/b-c/bld-cmake/src/./mapport.cpp:125:9
 8    [#4](/bitcoin-bitcoin/4/) 0x5f8c6957772d in std::function<void ()>::operator()() const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/std_function.h:591:9
 9    [#5](/bitcoin-bitcoin/5/) 0x5f8c6957772d in util::TraceThread(std::basic_string_view<char, std::char_traits<char>>, std::function<void ()>) /root/b-c/bld-cmake/src/util/./util/thread.cpp:21:9
10    [#6](/bitcoin-bitcoin/6/) 0x5f8c68c1cd4d in void std::__invoke_impl<void, void (*)(std::basic_string_view<char, std::char_traits<char>>, std::function<void ()>), char const*, void (*)()>(std::__invoke_other, void (*&&)(std::basic_string_view<char, std::char_traits<char>>, std::function<void ()>), char const*&&, void (*&&)()) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/invoke.h:61:14
11    [#7](/bitcoin-bitcoin/7/) 0x74eec02ecdb3  (/lib/x86_64-linux-gnu/libstdc++.so.6+0xecdb3) (BuildId: ca77dae775ec87540acd7218fa990c40d1c94ab1)
12    [#8](/bitcoin-bitcoin/8/) 0x74eebfe9caa3  (/lib/x86_64-linux-gnu/libc.so.6+0x9caa3) (BuildId: 282c2c16e7b6600b0b22ea0c99010d2795752b5f)
13    [#9](/bitcoin-bitcoin/9/) 0x74eebff29c3b  (/lib/x86_64-linux-gnu/libc.so.6+0x129c3b) (BuildId: 282c2c16e7b6600b0b22ea0c99010d2795752b5f)
14
15SUMMARY: UndefinedBehaviorSanitizer: unsigned-integer-overflow /root/b-c/src/common/netif.cpp:127:29

So that makes it unrelated to this pull requests.

But maybe there is a real bug on this line if it is subtracting 20 - 28:

I haven’t looked here if this is a bug, but it would be good to either suppress this warning (if it is harmless) or change the code (if there is a bug).

ipc: Handle unclean shutdowns better #32345

Code Coverage & Benchmarks

Reviews

Conflicts