💬 hodlinator commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2071517155)
Nice!

💬 jesterhodl commented on pull request "[29.x] qt: 29.1 translations update":
(https://github.com/bitcoin/bitcoin/pull/32352#issuecomment-2847057846)
> > * do I have to do anything when I just maintain new or updated labels? eg. when I align wording across the app
>
>
>
> Mind clarifying please?
Sure. I translate lots of apps
on Transifex, Weblate, etc.
I routinely revisit to check for new things to update or correct existing translations based on my experience or feedback.
So I update the transifex translations.

When I do this, is it sufficient and github pulls the .ts files automatically, or would it be wise to ping someone?

💬 hebasto commented on pull request "doc: Add hint about avoiding spaces in paths when building on Windows":
(https://github.com/bitcoin/bitcoin/pull/32397#issuecomment-2847086592)
> It's not clear what is failing to build? If it is libiconv, how is that related to our build; is this something we can just avoid?

`libiconv` is listed as a direct dependency of the `qt` package (see the output of `vcpkg depend-info qt`).

💬 kevkevinpal commented on pull request "tests: Added progress tracker when running fuzz test_runner.py":
(https://github.com/bitcoin/bitcoin/pull/32354#issuecomment-2847093955)
> 5d39a39

Thank you for the review, I agree that looks much better and makes sense to use the current output we get from libfuzzer

I pushed [5d39a39](https://github.com/bitcoin/bitcoin/pull/32354/commits/5d39a3995dbf23e78f17a46e4793b28322d95ebe) with similar changes to what you suggested

💬 rkrux commented on pull request "wallet: `addhdkey` RPC to add just keys to wallets via new `void(KEY)` descriptor":
(https://github.com/bitcoin/bitcoin/pull/29136#issuecomment-2847115949)
I have not gone into the details yet.

Quick comment - the PR title and description would now need to be updated to use `unused` instead of `void`. Also, some documentation in [descriptors.md file](https://github.com/bitcoin/bitcoin/blob/master/doc/descriptors.md) would be nice.

👍 hodlinator approved a pull request: "net: improve the interface around FindNode() and avoid a recursive mutex lock"
(https://github.com/bitcoin/bitcoin/pull/32326#pullrequestreview-2811932998)
re-ACK 4f635d100dd1ab5e83bb392d8fbee4a3f4400315

Concept unchanged since [first ACK](https://github.com/bitcoin/bitcoin/pull/32326#pullrequestreview-2790161149).

Changes since then:
* Includes now merged #32338 which simplified `AlreadyConnectedToAddress()` (spawned from this PR: https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2058384454)
* Changed from `OutboundConnectedTo(Str|Service)` -> `IsConnectedToAddr(|Port) const` and `str_addr` -> `addr_port`, nice!
* `AlreadyConnecte
...

💬 0xB10C commented on issue "test: interface_usdt_net.py failure under --valgrind":
(https://github.com/bitcoin/bitcoin/issues/32374#issuecomment-2847168570)
> still saw SIGTRAP raised.

Ah, every time the tracepoint is reached a SIGTRAP is fired. No matter the tracepoint contents, even with an empty one. Valgrind intercepts SIGTRAP and aborts.

> ==1145996== Process terminating with default action of signal 5 (SIGTRAP)

It might be possible to add a valgrind suppression for this this, but I haven't tired it yet.

💬 hebasto commented on pull request "doc: Add hint about avoiding spaces in paths when building on Windows":
(https://github.com/bitcoin/bitcoin/pull/32397#issuecomment-2847187033)
According to the vcpkg [docs](https://github.com/microsoft/vcpkg-docs/blob/main/vcpkg/users/buildsystems/cmake-integration.md), the default install directory (`${CMAKE_BINARY_DIR}\vcpkg_installed`) can be overridden by setting the [`VCPKG_INSTALLED_DIR`](https://github.com/microsoft/vcpkg-docs/blob/main/vcpkg/users/buildsystems/cmake-integration.md#vcpkg_installed_dir) variable to a path that contains no spaces. This resolves the issue.

💬 hebasto commented on pull request "[29.x] qt: 29.1 translations update":
(https://github.com/bitcoin/bitcoin/pull/32352#issuecomment-2847200274)
> > > * do I have to do anything when I just maintain new or updated labels? eg. when I align wording across the app
> >
> >
> > Mind clarifying please?
>
> Sure. I translate lots of apps on Transifex, Weblate, etc. I routinely revisit to check for new things to update or correct existing translations based on my experience or feedback. So I update the transifex translations.
>
> When I do this, is it sufficient and github pulls the .ts files automatically, or would it be wise to ping
...

💬 hebasto commented on pull request "random: Use modern Windows randomness functions":
(https://github.com/bitcoin/bitcoin/pull/32400#issuecomment-2847214954)
Concept ACK.

🤔 sipa reviewed a pull request: "random: Use modern Windows randomness functions"
(https://github.com/bitcoin/bitcoin/pull/32400#pullrequestreview-2812038985)
Code review ACK 9dcf2105a9fa60b600145e3fe032a296d47b28e3. I checked this matches the API documentation, but did not test anything. I'll leave build/linter issues to others.

💬 hebasto commented on pull request "random: Use modern Windows randomness functions":
(https://github.com/bitcoin/bitcoin/pull/32400#discussion_r2071632347)
Why not narrow the scope of this dependency to the `bitcoin_util` and `bitcoinkernel` targets?

👍 hebasto approved a pull request: "common: Close non-std fds before exec in RunCommandJSON"
(https://github.com/bitcoin/bitcoin/pull/32343#pullrequestreview-2812111754)
ACK a0eed55398f882d9390e50582b10272d18f2b836, tested on Ubuntu 25.04:

- On the master branch @ 68ac9f116c0228a277f18f60ba2278b56356e6ac:
```
$ cat /tmp/list-fd.sh
#!/usr/bin/env bash
ls -al /proc/self/fd > /tmp/open-files
$ ./build/bin/bitcoind -regtest -daemon -signer=/tmp/list-fd.sh
$ ./build/bin/bitcoin-cli -regtest enumeratesigners # Ignore errors
$ ./build/bin/bitcoin-cli -regtest stop
$ cat /tmp/open-files
total 0
dr-x------ 2 hebasto hebasto 10 May 2 15:05 .
dr-xr-xr-x 9
...

💬 willcl-ark commented on issue "test: interface_usdt_net.py failure under --valgrind":
(https://github.com/bitcoin/bitcoin/issues/32374#issuecomment-2847310135)
I think it would make most sense here to probably skip these tests when valgrind is enabled.

I reckon that any benefits we'd get by taking the time to fix eBPF/Valgrind incompatibility (if its even fixable) wouldn't be worth the investment.

They are already being tested via `ASan + LSan + UBSan + integer, no depends, USDT`, but appear to be being skipped in `Msan, depends`, even though USDT is enabled. Perhaps we enable that instead to cover memory sanitization?

👍 hebasto approved a pull request: "test: remove Boost SIGCHLD workaround."
(https://github.com/bitcoin/bitcoin/pull/32403#pullrequestreview-2812134260)
ACK 3add6ab9adcd722d57c6d488581358ae9b377f1a, I have reviewed the code and it looks OK.

📝 l0rinc opened a pull request: "fuzz: assert FastRandomContext range boundaries"
(https://github.com/bitcoin/bitcoin/pull/32404)
This PR addresses a few leftover nits found while reviewing https://github.com/bitcoin/bitcoin/pull/30611.
This was also needed to validate its behavior properly, because currently there's no way to visualize how often (and why) we're flushing.

💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#issuecomment-2847346556)
`c52b673bf8...c9cd7d3aa9`: rebase and address suggestions

💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2071700058)
There is even a bigger problem with `if (!file.Commit() || file.fclose() != 0) {` -- if the commit fails when `fclose()` will not be called, it will return and the file destructor will be upset that the file has been written to and is not closed. Fixed.

💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2071702348)
Changed to `LogError()`, but `__func__` is redundant with the config option `-logsourcelocations`, so I omitted it.

💬 vasild commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2071702885)
Right. Removed the scope.