🤔 hodlinator reviewed a pull request: "node: Add --debug_runs/-waitfordebugger + --debug_cmd"
(https://github.com/bitcoin/bitcoin/pull/31723#pullrequestreview-3274687166)
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 `#ifdef`s and default-`OFF` CMake variables are pretty cut & dry. Maybe there's some scenario I haven't thought of.

💬 hodlinator commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2387855746)
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?

💬 hodlinator commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2387995922)
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.

💬 hodlinator commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2387333141)
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.

💬 hodlinator commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2387108889)
Could do potentially do something like...
```C++
#if defined(WAIT_FOR_DEBUGGER) && defined(NDEBUG)
#error "Should only enable WAIT_FOR_DEBUGGER in debug builds"
#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* mod
...

💬 hodlinator commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2387963933)
Changed to comment explaining why we add a long timeout:
```python
# Allow human to context switch away, take time to attach debugger and/or debug init phase for 2 hours
self.rpc_timeout = max(self.rpc_timeout, 2 * 60 * 60)
```

💬 hodlinator commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2386939634)
This code was not fully baked between me soliciting conceptual feedback and putting it up for formal review, thanks for catching that!

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

💬 hodlinator commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2388939051)
Might be some lingering bad habit to include windows.h early. CI seems okay with sorting - fixed.

💬 hodlinator commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2387353926)
That piece of C++26 wasn't on my radar, nice! Added comment.

💬 hodlinator commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2388927844)
Happy to take contributions for them, but focusing on the main ones for now.

📝 JACOBO10S opened a pull request: "Create leading new hihgs"
(https://github.com/bitcoin/bitcoin/pull/33505)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 JACOBO10S commented on pull request "Create leading new hihgs":
(https://github.com/bitcoin/bitcoin/pull/33505#issuecomment-3349009108)
next one

🤔 fjahr reviewed a pull request: "ci: detect outbound internet traffic generated while running tests"
(https://github.com/bitcoin/bitcoin/pull/31349#pullrequestreview-3281444082)
Hm, I was hoping I could easily test this by uncommenting the change in the first commit and then running this in the CI, which I did by pushing to this branch: https://github.com/fjahr/bitcoin/commits/pr31349/ While not everything has finished, some job including unit tests succeeded and the only failing job [ran out of space](https://github.com/fjahr/bitcoin/actions/runs/18109731912/job/51532955001) which seems unrelated. Am I missing something here?

💬 fjahr commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2389021877)
nit: Would be nice to add a comment here so people don't have to hunt down the commit description.

💬 fjahr commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#discussion_r2389363270)
nit: here I would also like a bit of documention, maybe adding a small section in `ci/readme.md` would be the right place for this?

📝 davidgumberg opened a pull request: "test: sock: Enable socket pair tests on Windows"
(https://github.com/bitcoin/bitcoin/pull/33506)
Some `class Sock` tests were previously disabled because Windows lacks [`socketpair(2)`](https://man7.org/linux/man-pages/man2/socketpair.2.html). This PR adds a `CreateSocketPair()` helper for Windows which imitates [`socketpair()`](https://github.com/torvalds/linux/blob/1896ce8eb6c61824f6c1125d69d8fda1f44a22f8/net/unix/af_unix.c#L1830-L1859), and enables these test cases for Windows.

This change is generally an improvement, but is also broken out of a [branch](github.com/davidgumberg/bitc
...

💬 l0rinc commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2389620468)
"support" doesn't help me understand this:
> support
To bear the weight of, especially from below; keep from falling, sinking, or slipping.

💬 l0rinc commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2389636212)
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.

💬 l0rinc commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2389639009)
> 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.

💬 l0rinc commented on pull request "node: Add --debug_runs/-waitfordebugger + --debug_cmd":
(https://github.com/bitcoin/bitcoin/pull/31723#discussion_r2389640672)
Ah, so it's not the negative that we're guarding, in that case maybe:
```suggestion
attached = pid != 0;
```