💬 fanquake commented on pull request "Mempool: Do not enforce TRUC checks on reorg":
(https://github.com/bitcoin/bitcoin/pull/33504#issuecomment-3348792877)
https://github.com/bitcoin/bitcoin/actions/runs/18108538923/job/51529011305?pr=33504#step:8:1778:
```bash
Error processing input "/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_corpora/tx_pool/93f59991917f0dc980deabf80d553cf4573ebdd5"

test/util/txmempool.cpp:191 CheckMempoolTRUCInvariants: Assertion `entry.GetCountWithDescendants() <= TRUC_DESCENDANT_LIMIT' failed.
Error processing input "/Users/runner/work/bitcoin/bitcoin/ci/scratch/qa-assets/fuzz_corpora/tx_pool/93f59991917
...

🤔 mzumsande reviewed a pull request: "net: support overriding the proxy selection in ConnectNode()"
(https://github.com/bitcoin/bitcoin/pull/33454#pullrequestreview-3281486955)
Concept ACK

💬 mzumsande commented on pull request "net: support overriding the proxy selection in ConnectNode()":
(https://github.com/bitcoin/bitcoin/pull/33454#discussion_r2389093456)
Is this an oversight? Before, we'd use the network of `target_addr` instead of `addrConnect`, which makes more sense to me.

💬 mzumsande commented on pull request "net: support overriding the proxy selection in ConnectNode()":
(https://github.com/bitcoin/bitcoin/pull/33454#discussion_r2389142994)
should be `proxy_arg` to be consistent with the definition (although maybe `proxy_override` would be better?)

💬 instagibbs commented on pull request "Mempool: Do not enforce TRUC checks on reorg":
(https://github.com/bitcoin/bitcoin/pull/33504#issuecomment-3348879737)
@fanquake thanks, TRUC topology is no longer actually being stopped when limits are bypassed(when a reorg happens), so invariants checks for TRUC may fail now. I now removed the limits check flags because they're not actually being used to model reorgs. If we want to fuzz reorgs, we should do that instead.

🤔 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?