node: Add --debug_runs/-waitfordebugger + --debug

hodlinator commented at 1:18 pm on January 23, 2025: contributor

Facilitates debugging specific instances of bitcoind in the context of Python tests, inside of an editor/IDE.

Makes bitcoind spin during startup, waiting for a debugger to be attached, so that breakpoints during process init can be debugged.

Example usage

Generate build files with -DWAIT_FOR_DEBUGGER=ON & build.
Set breakpoints in editor/IDE.
Start Python test with debugger command: $ ./build/test/functional/feature_abortnode.py --debug_runs 1 --debug_cmd "kgx -- $(which lldb) -o continue -p \$PID\$" (LLDB inside a Gnome Console window) or $ ./build/test/functional/feature_abortnode.py --debug_runs 1 --debug_cmd "subl --command \"debugger {\\\"action\\\": \\\"start\\\", \\\"configuration\\\": {\\\"name\\\": \\\"foo\\\", \\\"type\\\": \\\"lldb\\\", \\\"request\\\": \\\"attach\\\", \\\"program\\\": \\\"dummyprogram\\\", \\\"pid\\\": \\\"\$PID\$\\\"}}\"" (Sublime Text Debugger)
Experience how debugger hits breakpoints.

Needed before merge

Testing on Mac to confirm -waitfordebugger on the C++ side behaves as expected.

DrahtBot commented at 1:18 pm on January 23, 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/31723.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	l0rinc

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:

#33142 (test: Run bench sanity checks in parallel with functional tests by maflcko)
#32380 (Modernize use of UTF-8 in Windows code by hebasto)

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 typos and grammar issues:

“it would result no TracerPid being detected,” -> “it would result in no TracerPid being detected,” [missing preposition “in” makes the clause ungrammatical]
“so the node would not leave that function and start responding to RPCs\n# this would mean wait_for_rpc_connection() during the test would not\n# complete.” -> “so the node would not leave that function and start responding to RPCs. This would mean wait_for_rpc_connection() during the test would not complete.” [run-on / missing sentence boundary makes the flow confusing]

drahtbot_id_5_m

hodlinator marked this as a draft on Jan 23, 2025

in src/bitcoind.cpp:267 in 7d181da293 outdated

262@@ -254,6 +263,51 @@ static bool AppInit(NodeContext& node)
263 
264 MAIN_FUNCTION
265 {
266+    for (int i = 0; i < argc; ++i) {
267+        if (strcmp(argv[i], "-waitfordebugger") == 0) {

maflcko commented at 1:46 pm on January 23, 2025:

Wouldn’t it be easier to just start with a debugger instead of first starting without one and then attaching one?

hodlinator commented at 1:58 pm on January 23, 2025:

Updated PR description to clarify intended usage.

One could possibly launch bitcoind as a parameter to lldb/gdb, but I don’t know if there is a way of doing that if one also wants to have the debugging view with stepping etc inside of the editor/IDE.

hodlinator commented at 9:57 am on September 25, 2025:

I’ve explored launching the debugger instead of attaching in a separate branch, see #31723 (comment)

fanquake commented at 1:47 pm on January 23, 2025: member

Makes bitcoind spin during startup, waiting for a debugger to be attached.

Can you explain more? If you want to run under a debugger, then why not just start bitcoind under a debugger; why do we need to add this option?

hodlinator commented at 1:55 pm on January 23, 2025: contributor

Can you explain more? If you want to run under a debugger, then why not just start bitcoind under a debugger; why do we need to add this option?

Clarified PR description text and added example usage. But I’m curious to hear other ways of doing the same thing if there are any.

maflcko commented at 2:00 pm on January 23, 2025: member

I think if you want to debug the start-up sequence specifically, it may be easier to do it outside the tests? Then anything else, if needed, can happen without patching Bitcoin Core source code itself.

hodlinator commented at 2:19 pm on January 23, 2025: contributor

I think if you want to debug the start-up sequence specifically, it may be easier to do it outside the tests? Then anything else, if needed, can happen without patching Bitcoin Core source code itself.

This is not about debugging the start-up sequence specifically, but rather debugging from an early stage. Maybe one has set 5 breakpoints, some of which are hit during bitcoind init, and some which are hit by re-running a Python test which calls a set of RPCs.

fanquake commented at 2:24 pm on January 23, 2025: member

Ideally we wouldn’t be adding debug code, to production binaries, to facilitate using an IDE.

Can you configure your IDE to do something like the equivalent of using gdb/lldb, and process attach --name bitcoind --waitfor? So it just grabs bitcoind once it spins up?

hodlinator commented at 2:37 pm on January 23, 2025: contributor

Ideally we wouldn’t be adding debug code, to production binaries, to facilitate using an IDE.

Agree it should not be in production binaries, should be #ifdefed away. Added as another reason for draft status.

Can you configure your IDE to do something like the equivalent of using gdb/lldb, and process attach --name bitcoind --waitfor? So it just grabs bitcoind once it spins up?

I’ve never seen something like that in an IDE/editor, but I’ll have a look, might exist a debugger console which allows that kind of thing.

This PR does however allow for attaching to a specific node instance of bitcoind if a Python tests runs several nodes at once. (I’m considering changing the index to refer to how many bitcoind-launches one has done, some tests just relaunch node 0 a bunch of times, and one might be interested in debugging it on the 3rd execution Edit: this is now implemented).

yuvicc commented at 1:46 pm on January 27, 2025: contributor

Agree with @maflcko @fanquake, not sure if we want to add that to the production code.

hodlinator force-pushed on Jan 27, 2025

hodlinator commented at 7:15 pm on January 27, 2025: contributor

Added tentative #ifdef solution in latest push. CMake part could probably be improved since now I think the #define leaks into the CMake project’s public interface, which should not be necessary.

DrahtBot added the label Needs rebase on Feb 12, 2025

hodlinator force-pushed on Feb 12, 2025

DrahtBot removed the label Needs rebase on Feb 12, 2025

hodlinator commented at 8:05 am on February 13, 2025: contributor

Added CMake option WAIT_FOR_DEBUGGER.
Changed the Python logic from debugging specific nodes, to debugging specific node runs/executions, which is more specific.
Use prctl() on Linux to allow any process to attach to bitcoind. This removes the necessity to disable kernel.yama.ptrace_scope on some systems, which may be seen as decreasing security, but is compiled out unless WAIT_FOR_DEBUGGER=ON.
(Confirmed PR works on Windows).
(Rebased).

hodlinator renamed this:
~~qa debug: Add --debugnode/-waitfordebugger [DRAFT]~~
qa debug: Add --debug_runs/-waitfordebugger [DRAFT]
on Feb 13, 2025

maflcko commented at 11:45 am on February 13, 2025: member

I think if you want to debug the start-up sequence specifically, it may be easier to do it outside the tests? Then anything else, if needed, can happen without patching Bitcoin Core source code itself.

This is not about debugging the start-up sequence specifically, but rather debugging from an early stage. Maybe one has set 5 breakpoints, some of which are hit during bitcoind init, and some which are hit by re-running a Python test which calls a set of RPCs.

I’d still say it is easier to debug the init sequence separately than to start a new build with a new build flag for the edge case where someone wants to debug both in an IDE and also the init sequence , but no strong opinion.

hodlinator commented at 3:49 pm on February 13, 2025: contributor

fanquake:

Can you configure your IDE to do something like the equivalent of using gdb/lldb, and process attach --name bitcoind --waitfor? So it just grabs bitcoind once it spins up?

I’ve never seen something like that in an IDE/editor, but I’ll have a look, might exist a debugger console which allows that kind of thing.

The only debugging console I get with the IDE states “No Active Debug Session” when I try to input commands. :(

maflcko:

This is not about debugging the start-up sequence specifically, but rather debugging from an early stage. Maybe one has set 5 breakpoints, some of which are hit during bitcoind init, and some which are hit by re-running a Python test which calls a set of RPCs.

I’d still say it is easier to debug the init sequence separately than to start a new build with a new build flag for the edge case where someone wants to debug both in an IDE and also the init sequence , but no strong opinion.

Maybe part of this is down to me not getting used to using a Python debugger, breaking in Python at appropriate times and then attaching a C++ debugger to the new process. Being able to specify which bitcoind-executions one wants to debug still seems like a smoother workflow when testing the same scenario repeatedly.

I recently used a version of this PR when digging into #31734. Being able to step through the code (with a panel for inspecting local variables and watches), as the Python test framework is issuing RPCs, in the safety of one’s IDE, is nice. Knowing that bitcoind has not passed through one’s breakpoints (unless there’s some rare static initialization going on) helps build certainty about what unfamiliar code is involved in.

Granted, using --waitfor straight in gdb/lldb would give definite certainty what code runs. But the IDE GUI would not be there. I know gdb has some kind of TUI, and there are probably commands for saving and restoring state. It just feels like operating the computer with my feet. Happy to be proven wrong though. Please shill your setups. :)

DrahtBot added the label Needs rebase on Mar 20, 2025

hodlinator force-pushed on Mar 20, 2025

DrahtBot removed the label Needs rebase on Mar 20, 2025

DrahtBot added the label Needs rebase on Apr 25, 2025

hodlinator force-pushed on Jun 12, 2025

DrahtBot removed the label Needs rebase on Jun 12, 2025

DrahtBot added the label Needs rebase on Sep 5, 2025

hodlinator force-pushed on Sep 23, 2025

hodlinator renamed this:
~~qa debug: Add --debug_runs/-waitfordebugger [DRAFT]~~
node: Add --debug_runs/-waitfordebugger
on Sep 23, 2025

hodlinator force-pushed on Sep 23, 2025

DrahtBot removed the label Needs rebase on Sep 23, 2025

hodlinator renamed this:
~~node: Add --debug_runs/-waitfordebugger~~
node: Add --debug_runs/-waitfordebugger [DRAFT, rework incoming]
on Sep 24, 2025

hodlinator renamed this:
~~node: Add --debug_runs/-waitfordebugger [DRAFT, rework incoming]~~
node: Add --debug_runs/-waitfordebugger
on Sep 25, 2025

hodlinator force-pushed on Sep 25, 2025

hodlinator commented at 9:53 am on September 25, 2025: contributor

Realized one could start a debugger session from the CLI: $ subl --command "debugger {\"action\": \"start\", \"configuration\": {\"name\": \"foo\", \"type\": \"lldb\", \"request\": \"launch\", \"program\": \"\${project_path}/build/bin/bitcoind\", \"args\": [\"-noconnect\"]}}" or $ kgx -- $(which lldb) -o run -- /home/hodlinator/bc/3/build/bin/bitcoind -noconnect

So I’ve explored a variation of this which does not change the C++ side at all - https://github.com/hodlinator/bitcoin/tree/refs/heads/2025/09/launch_debugger. The approach is to launch the debugger from inside TestNode with the bitcoind command line as parameters. A stumbling block was that both debugger commands above exit immediately instead of running until bitcoind exits. Both document supporting --wait but Sublime Text requires passing in a file to be opened (--wait dummyfile) and the kgx Gnome Console has it marked as “TODO”.

This lead me to create the launch_and_wait.py shim which simulates bitcoind process lifetime behavior. The shim has the following drawbacks:

Requires manually sending a HTTP GET request for it to exit. - Maybe there’s some way for it to monitor PID-files and exit automatically based off that for most tests which don’t mess around with that file.
While it could pass on environment variables (or in the case of Sublime Text have them specified through configuration.env), I don’t think it’s possible to pass through stderr/stdout since the debug launch commands exit quickly. - Maybe there’s some way for it to monitor the node’s stdout/stderr files and copy their contents to its own stdout/stderr, but it seems a bit messy.

Attaching to the process seems less error prone. After having worked on the above I realized attaching the debugger could be automated, so added that as a second commit in this PR.

If reviewers insist I could continue exploring the launch_debugger route to see how monitoring PID-files (or just require something like --wait dummyfile) and replicating stdout/stderr contents pans out.

hodlinator marked this as ready for review on Sep 25, 2025

hodlinator renamed this:
~~node: Add --debug_runs/-waitfordebugger~~
node: Add --debug_runs/-waitfordebugger + --debug_cmd
on Sep 25, 2025

in test/functional/test_framework/test_framework.py:261 in eca366ba08 outdated

256@@ -257,6 +257,8 @@ def parse_args(self, test_file):
257                             help="Explicitly use v1 transport (can be used to overwrite global --v2transport option)")
258         parser.add_argument("--test_methods", dest="test_methods", nargs='*',
259                             help="Run specified test methods sequentially instead of the full test. Use only for methods that do not depend on any context set up in run_test or other methods.")
260+        parser.add_argument("--debug_runs", dest="debug_runs", nargs="*", type=int, default=[],
261+                            help="Node executions in the test to stall and wait for a debugger, 0-based.")

l0rinc commented at 8:07 pm on September 26, 2025:

the 0-based part may need some explanation

hodlinator commented at 12:54 pm on September 29, 2025:

If a test executes bitcoind twice, there will be run 0 and run 1. I thought calling that “0-based” was enough.

Changed to: “Indexes for node executions in the test to stall and wait for a native debugger, 0-based.” - maybe that’s clearer?

in test/functional/test_framework/test_node.py:292 in eca366ba08 outdated

285@@ -283,10 +286,19 @@ def start(self, extra_args=None, *, cwd=None, stdout=None, stderr=None, env=None
286         if env is not None:
287             subp_env.update(env)
288 
289+        wait_for_debugger = TestNode.debug_runs is not None and TestNode.run in TestNode.debug_runs
290+        if wait_for_debugger:
291+            extra_args.append("-waitfordebugger")
292+            self.rpc_timeout = max(self.rpc_timeout, 2 * 60 * 60)  # 2h timeout

l0rinc commented at 8:08 pm on September 26, 2025:

nit: comment is redundant

hodlinator commented at 1:28 pm on September 29, 2025:

Changed to comment explaining why we add a long timeout:

0# Allow human to context switch away, take time to attach debugger and/or debug init phase for 2 hours
1self.rpc_timeout = max(self.rpc_timeout, 2 * 60 * 60)

in test/functional/test_framework/test_node.py:298 in eca366ba08 outdated

294         self.process = subprocess.Popen(self.args + extra_args, env=subp_env, stdout=stdout, stderr=stderr, cwd=cwd, **kwargs)
295 
296         self.running = True
297-        self.log.debug("bitcoind started, waiting for RPC to come up")
298+        if wait_for_debugger:
299+            self.log.info(f"bitcoind started (run #{TestNode.run}, node #{self.index}), waiting for debugger, PID: {self.process.pid}")

l0rinc commented at 8:09 pm on September 26, 2025:

nice - it might be overkill, but how difficult do you think it would be to add a test for whether we can start a test with debugging? Would help with checking it on other platforms as well

hodlinator commented at 1:33 pm on September 29, 2025:

I’m concerned with adding an extra build permutation for CI. Maybe could turn it on in some debug build. Hm..

Changed C++ code to use constexpr over #ifdef so that the code at least gets regularly compiled.

in src/bitcoind.cpp:181 in eca366ba08 outdated

176+{
177+    for (int i = 0; i < argc; ++i) {
178+        if (strcmp(argv[i], "-waitfordebugger") == 0) {
179+            while (true) {
180+                bool attached{false};
181+#   if defined(__linux__)

l0rinc commented at 8:10 pm on September 26, 2025:

learning from #33435 we may want to add support for BSD as well (not sure anyone will use it, but at least we should be aware that it’s an option)

hodlinator commented at 6:48 pm on September 29, 2025:

Happy to take contributions for them, but focusing on the main ones for now.

in src/bitcoind.cpp:211 in eca366ba08 outdated

206+                if (ret != EXIT_SUCCESS) {
207+                    return ret;
208+                }
209+                attached = info.kp_proc.p_flag & P_TRACED;
210+#   elif defined(WIN32)
211+                attached = IsDebuggerPresent();

l0rinc commented at 8:12 pm on September 26, 2025:

This is funny, I wonder why other platforms don’t have this.

Seems C++26 will have this standardized: https://en.cppreference.com/w/cpp/utility/is_debugger_present.html, maybe we can add it as a comment (and adjust the naming to match that, if needed)

hodlinator commented at 9:51 am on September 29, 2025:

That piece of C++26 wasn’t on my radar, nice! Added comment.

in src/bitcoind.cpp:183 in eca366ba08 outdated

178+        if (strcmp(argv[i], "-waitfordebugger") == 0) {
179+            while (true) {
180+                bool attached{false};
181+#   if defined(__linux__)
182+                // Allow any process to attach to us.
183+                prctl(PR_SET_PTRACER, PR_SET_PTRACER_ANY);

l0rinc commented at 8:25 pm on September 26, 2025:

is it possible to add an assert somewhere that this isn’t enabled when we’re not in debug mode, just to be sure?

hodlinator commented at 8:24 am on September 29, 2025:

Could do potentially do something like…

0#if defined(WAIT_FOR_DEBUGGER) && defined(NDEBUG)
1#error "Should only enable WAIT_FOR_DEBUGGER in debug builds"
2#endif

…though there could be scenarios where one wants to debug release builds as well.

/contrib/guix/symbol-check.py could also potentially analyze the binary after the fact. LLM says that the lief module can find calls to prctl when dynamically linking and otherwise one could add a dependency on the capstone module and use that to iterate through the assembly instructions to find a direct syscall.

Could you please elaborate a bit what you’re after?

l0rinc commented at 1:40 am on September 30, 2025:

I was just referring to the concerns of other reviewers about modifying production code - maybe a test could make sure we don’t accidentally include this in the future. We have tests that start up perf tool, it shouldn’t be too difficult to check if a debugger can attach to us in a test (though I wouldn’t guard, I would want that test to fail if run with a debugger, that’s how I would trust it) - but if you don’t think it’s useful, I understand.

hodlinator commented at 7:54 pm on September 30, 2025:

Added a test for Linux (the only OS where we explicitly enable attaching) in the latest push.

in src/bitcoind.cpp:179 in eca366ba08 outdated

170@@ -159,6 +171,60 @@ static bool ProcessInitCommands(ArgsManager& args)
171     return false;
172 }
173 
174+#ifdef WAIT_FOR_DEBUGGER
175+static int HandleWaitForDebugger(int argc, char* argv[])
176+{
177+    for (int i = 0; i < argc; ++i) {
178+        if (strcmp(argv[i], "-waitfordebugger") == 0) {
179+            while (true) {

l0rinc commented at 8:31 pm on September 26, 2025:

Seems a bit weird to me that we’re doing all this logic during iteration, adds extra context that isn’t needed. Can we separate the iteration and arg finding from doing the processing?

0bool wait_for_debugger{false};
1for (int i = 1; i < argc; ++i) {
2    if (strcmp(argv[i], "-waitfordebugger") == 0) {
3        wait_for_debugger = true;
4        break;
5    }
6}

Notes:

I would personally prefer C++ method instead of pure C, e.g. std::string_view(argv[i]) == "-waitfordebugger")
std::find_if or std::any_of might reduce this to a one-liner

0if (!std::ranges::any_of(args, [](std::string_view s) { return s == "-waitfordebugger"; })) return EXIT_SUCCESS;

hodlinator commented at 7:17 am on September 29, 2025:

This code was not fully baked between me soliciting conceptual feedback and putting it up for formal review, thanks for catching that!

Switched to:

0    if (std::any_of(argv, argv + argc, [] (char* arg) { return strcmp(arg, "-waitfordebugger") == 0; })) {

in src/bitcoind.cpp:32 in eca366ba08 outdated

27@@ -28,7 +28,19 @@
28 #include <util/tokenpipe.h>
29 #include <util/translation.h>
30 
31+#ifdef WIN32
32+#include <windows.h>
33+#include <debugapi.h>

l0rinc commented at 8:32 pm on September 26, 2025:

nit: any reason for not sorting these?

hodlinator commented at 6:53 pm on September 29, 2025:

Might be some lingering bad habit to include windows.h early. CI seems okay with sorting - fixed.

in src/bitcoind.cpp:186 in eca366ba08 outdated

181+#   if defined(__linux__)
182+                // Allow any process to attach to us.
183+                prctl(PR_SET_PTRACER, PR_SET_PTRACER_ANY);
184+
185+                std::ifstream sf{"/proc/self/status", std::ios::in};
186+                if (!sf.good()) {

l0rinc commented at 8:36 pm on September 26, 2025:

it seems to the ifstream has operator bool() so

0if (!sf) return EXIT_FAILURE;

should be more compact

hodlinator commented at 8:35 am on September 29, 2025:

.good() seems to be a better option since it also covers EOF, see the table at: https://en.cppreference.com/w/cpp/io/basic_ios/operator_bool.html#See_also

in src/bitcoind.cpp:218 in eca366ba08 outdated

213+#       error "Platform doesn't support WAIT_FOR_DEBUGGER/-waitfordebugger.";
214+#   endif // platform
215+                if (attached) {
216+                    break;
217+                } else {
218+                    std::this_thread::sleep_for(100ms);

l0rinc commented at 8:47 pm on September 26, 2025:

I wonder if there’s a better way than sleep. The internets indicates that raise(SIGSTOP) + SIGTRAP would also pause and revive the process safely - I don’t have Linux locally, but maybe we could give it a try

hodlinator commented at 9:43 am on September 29, 2025:

Using signals would be cool, but I’d rather use the current approach that matches IsDebuggerPresent (and the future std::is_debugger_present()) and also has a pattern that works on Mac (if it indeed works :) ). I don’t expect sleep_for() to busy-wait when given such a “long” duration.

in src/init.cpp:1131 in eca366ba08 outdated

1125@@ -1125,6 +1126,12 @@ bool AppInitParameterInteraction(const ArgsManager& args)
1126         }
1127     }
1128 
1129+#ifndef WAIT_FOR_DEBUGGER
1130+    if (args.GetBoolArg("-waitfordebugger", false)) {
1131+        return InitError(Untranslated("-waitfordebugger support was not included as WAIT_FOR_DEBUGGER is not #defined."));

l0rinc commented at 8:50 pm on September 26, 2025:

Most of this is hard to test, but maybe we can test this safely

nit: had to read the sentence a few times, maybe we can change it to:

0        return InitError(Untranslated("-waitfordebugger is unavailable in this build. Rebuild with -DWAIT_FOR_DEBUGGER=ON."));

hodlinator commented at 9:47 am on September 29, 2025:

You want to add a functional test that runs bitcoind to confirm that -waitfordebugger triggers this error message? Or maybe a unit test that could depend on the #define? Not sure, anything I can come up with feels performative.

Took nit.

l0rinc commented at 1:42 am on September 30, 2025:

add a functional test that runs bitcoind to confirm that -waitfordebugger triggers this error message

Yes, it would be reassuring to me that it’s not on by default.

hodlinator commented at 7:56 pm on September 30, 2025:

New test includes assert_start_raises_init_error() at the end for this precisely.

in test/functional/test_framework/test_framework.py:262 in eca366ba08 outdated

256@@ -257,6 +257,8 @@ def parse_args(self, test_file):
257                             help="Explicitly use v1 transport (can be used to overwrite global --v2transport option)")
258         parser.add_argument("--test_methods", dest="test_methods", nargs='*',
259                             help="Run specified test methods sequentially instead of the full test. Use only for methods that do not depend on any context set up in run_test or other methods.")
260+        parser.add_argument("--debug_runs", dest="debug_runs", nargs="*", type=int, default=[],

l0rinc commented at 8:52 pm on September 26, 2025:

is --debug_runs 1,3 the same as --debug_runs 1 --debug_runs 3?

And I find the name confusing, it sounds like how many rounds of debugging we’re doing.

hodlinator commented at 1:54 pm on September 29, 2025:

Correction for the first multi-value form: --debug_runs 1 3 (Edit: Made indexes the same)

If --debug_runs is provided multiple times as in your second form, only the last value(s) are respected. The behavior is a feature of the Python argparser. One could switch to the append type which would make the second form work, but then I think the first form would stop working.

in src/bitcoind.cpp:210 in eca366ba08 outdated

195+                        break;
196+                    }
197+
198+                    std::getline(sf, s);
199+                }
200+                attached = pid > 0;

l0rinc commented at 8:58 pm on September 26, 2025:

do we need any pid validation? If it’s set can it be a non-number or a negative one? Can it be a different process’ id?

hodlinator commented at 9:14 am on September 29, 2025:

0₿ cat /proc/self/status |grep TracerPid
1TracerPid:	0

0 is what we get when nothing is attached or the input stream somehow failed to read. When it is non-zero, it will be the debugger’s PID.

l0rinc commented at 1:44 am on September 30, 2025:

Ah, so it’s not the negative that we’re guarding, in that case maybe:

0                attached = pid != 0;

hodlinator commented at 6:46 am on September 30, 2025:

Did something similar to #31723 (review)

in src/bitcoind.cpp:199 in eca366ba08 outdated

194+                        sf >> pid;
195+                        break;
196+                    }
197+
198+                    std::getline(sf, s);
199+                }

l0rinc commented at 9:13 pm on September 26, 2025:

this seems very manual, if it’s really the only way to get the pid, maybe we can do a precompiled regex instead:

0const std::string s{std::istreambuf_iterator<char>{sf}, {}};
1static const std::regex re(R"(TracerPid:\s*(\d+))", std::regex::optimize);
2if (std::smatch m; std::regex_search(s, m, re)) attached = std::stoul(m[1]) > 0;

or

0static const std::regex re(R"(TracerPid:\s*(\d+))", std::regex::optimize);
1for (std::string line; !attached && std::getline(sf, line);) {
2    if (std::smatch m; std::regex_search(line, m, re)) attached = std::stoul(m[1]) > 0;
3}

hodlinator commented at 9:10 am on September 29, 2025:

I’d rather not go as heavy as regex here, but limited the scope of name & pid:

 0for (std::string name; status.good();
 1    // Skips to the beginning of next line using name as scratch buffer.
 2    std::getline(status, name)) {
 3    status >> name;
 4    if (name == "TracerPid:") {
 5        uint pid{0};
 6        status >> pid;
 7        attached = pid > 0;
 8        break;
 9    }
10}

l0rinc commented at 2:04 am on September 30, 2025:

Precompiled regexes are optimized to NFAs, but this one doesn’t even backtrack so it’s likely really fast and very easy to read.

But looking at https://github.com/catchorg/Catch2/blob/devel/src/catch2/internal/catch_debugger.cpp#L95 it seems to me that we don’t even need to parse it, just check if it starts with zero or not, maybe something like:

0for (std::string line; !attached && std::getline(status, line);) {
1    if (line.starts_with("TracerPid:")) {
2        attached = line.find_first_of("123456789") != std::string::npos;
3    }
4}

hodlinator commented at 7:05 am on September 30, 2025:

That’s a clever way of doing it, did something very close to it now (hopefully it’s small enough that source code license issues don’t apply).

in src/init.cpp:707 in eca366ba08 outdated

703@@ -704,6 +704,7 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
704     hidden_args.emplace_back("-daemon");
705     hidden_args.emplace_back("-daemonwait");
706 #endif
707+    argsman.AddArg("-waitfordebugger", "Spin until a debugger is attached", ArgsManager::ALLOW_ANY, OptionsCategory::DEBUG_TEST);

l0rinc commented at 9:14 pm on September 26, 2025:

can we document this in https://github.com/bitcoin/bitcoin/blob/ca38cf701dc635d39db987105c5b2ccc87fd9815/doc/developer-notes.md?plain=1#L297 as well?

hodlinator commented at 1:41 pm on September 29, 2025:

Good point, added.

in src/CMakeLists.txt:30 in eca366ba08 outdated

25@@ -26,6 +26,10 @@ endif()
26 include(../cmake/secp256k1.cmake)
27 add_secp256k1(secp256k1)
28 
29+if(WAIT_FOR_DEBUGGER)
30+  add_compile_definitions(WAIT_FOR_DEBUGGER=1)

l0rinc commented at 9:21 pm on September 26, 2025:

nit: given that the default is OFF, wouldn’t =ON be more cmake and user friendly?

hodlinator commented at 7:32 am on September 27, 2025:

This is the C++ compiler #define which are usually set to 1, not the CMake variable.

in src/bitcoind.cpp:206 in eca366ba08 outdated

201+#   elif defined(__APPLE__)
202+                const int mib[] = { CTL_KERN, KERN_PROC, KERN_PROC_PID, getpid() };
203+                kinfo_proc info;
204+                size_t size{sizeof(info)};
205+                const int ret{sysctl(mib, sizeof(mib) / sizeof(*mib), &info, &size, nullptr, 0)};
206+                if (ret != EXIT_SUCCESS) {

l0rinc commented at 9:29 pm on September 26, 2025:

we can likely inline this and if we used an std::array the complexity would be reduced slighly:

0std::array mib{CTL_KERN, KERN_PROC, KERN_PROC_PID, getpid()};
1kinfo_proc info{};
2size_t size{sizeof(info)};
3if (int ret{sysctl(mib.data(), mib.size(), &info, &size, nullptr, 0)}) return ret;
4attached = info.kp_proc.p_flag & P_TRACED;

nit: consider type name hints like in https://github.com/bitcoin/bitcoin/blob/2562fe1b2b63c3a510735ba417935340d533a844/src/common/netif.cpp#L237

hodlinator commented at 9:24 am on September 29, 2025:

Taken with minor adjustments.

in src/bitcoind.cpp:217 in eca366ba08 outdated

212+#   else
213+#       error "Platform doesn't support WAIT_FOR_DEBUGGER/-waitfordebugger.";
214+#   endif // platform
215+                if (attached) {
216+                    break;
217+                } else {

l0rinc commented at 9:35 pm on September 26, 2025:

nit:

0if (attached) break;
1std::this_thread::sleep_for(100ms);

hodlinator commented at 9:31 am on September 29, 2025:

Switched to:

0        if (attached) {
1            return EXIT_SUCCESS;
2        }
3
4        std::this_thread::sleep_for(100ms);

I prefer making major control flow interruptions like return happen on their own line (also changed line calling HandleWaitForDebugger).

in src/bitcoind.cpp:175 in eca366ba08 outdated

170@@ -159,6 +171,60 @@ static bool ProcessInitCommands(ArgsManager& args)
171     return false;
172 }
173 
174+#ifdef WAIT_FOR_DEBUGGER
175+static int HandleWaitForDebugger(int argc, char* argv[])

l0rinc commented at 9:50 pm on September 26, 2025:

Can we modernize this to

0static int HandleWaitForDebugger(std::span<char*> args)

?

hodlinator commented at 7:14 am on September 29, 2025:

Made the arg check external to the function.

in src/bitcoind.cpp:324 in eca366ba08 outdated

319@@ -254,6 +320,10 @@ static bool AppInit(NodeContext& node)
320 
321 MAIN_FUNCTION
322 {
323+#ifdef WAIT_FOR_DEBUGGER
324+    if (int ret{HandleWaitForDebugger(argc, argv)}; ret != EXIT_SUCCESS) return ret;

l0rinc commented at 10:04 pm on September 26, 2025:

let’s not propagate these further:

0    if (int ret{HandleWaitForDebugger(std::span{argv + 1, size_t(argc - 1)})}; ret != EXIT_SUCCESS) return ret;

hodlinator commented at 9:44 am on September 29, 2025:

Avoided conversion to span by checking outside of the function.

in CMakeLists.txt:123 in eca366ba08 outdated

119@@ -120,6 +120,7 @@ cmake_dependent_option(BUILD_WALLET_TOOL "Build bitcoin-wallet tool." ${BUILD_TE
120 option(REDUCE_EXPORTS "Attempt to reduce exported symbols in the resulting executables." OFF)
121 option(WERROR "Treat compiler warnings as errors." OFF)
122 option(WITH_CCACHE "Attempt to use ccache for compiling." ON)
123+option(WAIT_FOR_DEBUGGER "Support for waiting during startup for debugger to be attached." OFF)

l0rinc commented at 10:11 pm on September 26, 2025:

0option(WAIT_FOR_DEBUGGER "Pause at startup until a debugger attaches (-waitfordebugger)." OFF)

hodlinator commented at 7:09 am on September 29, 2025:

Took inspiration from your suggestion:

0option(WAIT_FOR_DEBUGGER "Support for waiting at startup until a debugger attaches (-waitfordebugger)." OFF)

Still want to retain “support” so it doesn’t sound like it’s always on.
Retained “wait” over “pause” since it matches the feature’s name.

l0rinc commented at 1:25 am on September 30, 2025:

“support” doesn’t help me understand this:

support To bear the weight of, especially from below; keep from falling, sinking, or slipping.

hodlinator commented at 7:41 pm on September 30, 2025:

To me there’s a silent, implied, “Enables …” or “Provides …” at the beginning of the description for these options that would get repetitive if spelled out, which may make “support” more natural.

Note that simply enabling the option does not pause startup, one also has to start with -waitfordebugger.

Curious to hear what other reviewers think.

l0rinc commented at 10:21 pm on September 26, 2025: contributor

Concept ACK, I’m always struggling with debugging these tests with multiple nodes, I usually end up just printing their internal states.

Thanks for working on this. I went through the code, tried to find simpler solutions to minimize and modernize the code we need to add to production code. I didn’t get to trying this out yet, so I’m not sure my suggestions make sense, I just checked if it compiles on a Mac or Linux. Will try to test it properly next week.

Since we’re editing production code for this I would prefer having a test which checks that support was not included as WAIT_FOR_DEBUGGER is logged, proving that we’re correctly excluding it from code (it’s fine if the test isn’t passing in debug mode)

hodlinator force-pushed on Sep 29, 2025

hodlinator commented at 8:44 pm on September 29, 2025: contributor

Thanks for reviewing @l0rinc!

Since we’re editing production code for this I would prefer having a test which checks that support was not included as WAIT_FOR_DEBUGGER is logged, proving that we’re correctly excluding it from code (it’s fine if the test isn’t passing in debug mode)

Feel it would be a bit performative since #ifdefs and default-OFF CMake variables are pretty cut & dry. Maybe there’s some scenario I haven’t thought of.

hodlinator force-pushed on Oct 1, 2025

hodlinator commented at 7:56 am on October 1, 2025: contributor

Latest push adds a functional test and breaks apart commits somewhat.

hodlinator force-pushed on Oct 1, 2025

qa: Add feature_debugging.py

Verifies that debugging bitcoind is not allowed by default on Linux, at least in sane environments.

c43f99aecd

node: Add -waitfordebugger

Makes bitcoind spin during startup, waiting for a debugger to be attached. Once a debugger is detected, execution will continue.

52d7030b07

qa: Add --debug_runs

Useful for debugging one or several bitcoind nodes running in the context of a functional test.

025776e93a

qa: Add --debug_cmd

Enables automated attaching of debuggers.

Attaching with LLDB in Gnome Console window:
$ ./build/test/functional/feature_abortnode.py --debug_runs 1 --debug_cmd "kgx -- $(which lldb) -o continue -p \$PID\$"

Attaching with Sublime Text Debugger:
$ ./build/test/functional/feature_abortnode.py --debug_runs 1 --debug_cmd "subl --command \"debugger {\\\"action\\\": \\\"start\\\", \\\"configuration\\\": {\\\"name\\\": \\\"foo\\\", \\\"type\\\": \\\"lldb\\\", \\\"request\\\": \\\"attach\\\", \\\"program\\\": \\\"dummyprogram\\\", \\\"pid\\\": \\\"\$PID\$\\\"}}\""

d1a50b119f

hodlinator force-pushed on Oct 1, 2025

node: Add –debug_runs/-waitfordebugger + –debug_cmd #31723

Example usage

Needed before merge

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)