💬 fanquake commented on pull request "doc: Add hint about avoiding spaces in paths when building on Windows":
(https://github.com/bitcoin/bitcoin/pull/32397#issuecomment-2847050625)
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? In either case, please adjust the hint to be explcit about spaces being unsupported, and remove vaugeries like "they have been known to cause build issues.". Either spaces are supported, in which case we can test the behaviour, or they are not supported.

💬 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.