💬 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?
(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.
(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.
(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
...
(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)
```
(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; })) {
```
(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.
(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.
(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.
(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
...
(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
(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?
(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.
(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?
(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
...
(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.
(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.
(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.
(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;
```
(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;
```
📝 polespinasa opened a pull request: "RPC: add sendrawtransactiontopeer"
(https://github.com/bitcoin/bitcoin/pull/33507)
Adds a new RPC `sendrawtransactiontopeer`, which sends a single tx to a specific peer.
After the transaction is sent to the peer it is forgotten and not stored in the local mempool.
This rpc can serve several purposes. For example, it allows you to “spoof” an initial retransmission from a different "trusted" peer, making it appear as if the original participant was never aware of the transaction. It can also be useful in testing and simulation environments.
Solves #28636 and #21876 (P
...
(https://github.com/bitcoin/bitcoin/pull/33507)
Adds a new RPC `sendrawtransactiontopeer`, which sends a single tx to a specific peer.
After the transaction is sent to the peer it is forgotten and not stored in the local mempool.
This rpc can serve several purposes. For example, it allows you to “spoof” an initial retransmission from a different "trusted" peer, making it appear as if the original participant was never aware of the transaction. It can also be useful in testing and simulation environments.
Solves #28636 and #21876 (P
...