💬 Sjors commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#discussion_r1853497933)
Good catch, pushed...

💬 Sjors commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2493230777)
I re-enabled multiprocess for two more depends build machines: macOS cross and centos. Let's see how those go.

I also enabled it for the native fuzz with valgrind job and the native valgrind job, let's see how that goes.

For all non-depends builds I now explicitly set `-DWITH_MULTIPROCESS` to either `ON` or `OFF` for clarity.

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853544511)
"being opened to the ".bitcoin"-directory (not a file)). " - double parenthesis close.

And if we're here aleady the other one is missing an `of` in `to track initialization *of* HTTP RPC authentication`

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853551532)
The part I'm not sure about is checking `gArgs.GetBoolArg("-version", false)` twice - since the inner check has an else, which is a bit confusing, since that can only happen if `argc < 2 || HelpRequested(gArgs)` are true - seems like the condition can be simplified, especially since `if (argc < 2) {` is rechecked later. (please verify for other such conditions, not necessarily this one)

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853552851)
it's quite noisy in this case, but don't have string feelings about it :)

🤔 vasild reviewed a pull request: "test: avoid internet traffic in rpc_net.py"
(https://github.com/bitcoin/bitcoin/pull/31343#pullrequestreview-2453797824)
Concept ACK

💬 vasild commented on pull request "test: avoid internet traffic in rpc_net.py":
(https://github.com/bitcoin/bitcoin/pull/31343#discussion_r1853553795)
I checked on my network and packets to `169.254.33.44` still leave the machine and are received by the default gateway.

What about using `-connect=0` or `-proxy=127.0.0.1:1` for this particular test? One of the nodes it adds is available and it can connect to it (node2), but by reading the test I get the impression that this is not necessary. I.e. the test would still work and fulfill its purpose if it cannot connect to node2. So a setup like `-proxy=127.0.0.1:1` where it cannot connect to an
...

👍 TheCharlatan approved a pull request: "guix: swap `moreutils` for just `sponge`"
(https://github.com/bitcoin/bitcoin/pull/31323#pullrequestreview-2454001885)
ACK c4ee9b8aa07824610e1de9c70a4b938844c5ff53

💬 hebasto commented on pull request "guix: swap `moreutils` for just `sponge`":
(https://github.com/bitcoin/bitcoin/pull/31323#discussion_r1853651388)
Why are these lines needed?

👍 TheCharlatan approved a pull request: "rpc: add optional blockhash to waitfornewblock, unhide in help"
(https://github.com/bitcoin/bitcoin/pull/30635#pullrequestreview-2454014018)
ACK 1f8808d411fc0ed2b91f185e08ad8c1ee8f8432f

Tested in the GUI too.

💬 fanquake commented on pull request "build: Fix coverage builds":
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2493367729)
> which inadvertently broke coverage builds for both [Clang's source-based code coverage](https://issues.oss-fuzz.com/issues/379122777)

Can you clarify that this is only for oss-fuzz, due to its use of other options and a third party script, otherwise, source based code coverage using Clang currently still works fine with master (see: https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2490620693). Similarly, in the inline comment you've added, can you be a bit more specific than "can
...

💬 Sjors commented on issue "TSAN/MSAN fails with vm.mmap_rnd_bits=32 even with llvm 18.1.3":
(https://github.com/bitcoin/bitcoin/issues/30674#issuecomment-2493384001)
When I install the ARM version of Ubuntu in VM on an M4 macOS machine using UTM, the default of `vm.mmap_rnd_bits` is 33. Though I haven't checked if the native ARM job cares.

💬 hebasto commented on pull request "build: Fix coverage builds":
(https://github.com/bitcoin/bitcoin/pull/31337#discussion_r1853671315)
[Reworked](https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2490932956).

💬 hebasto commented on pull request "build: Fix coverage builds":
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2493416095)
> > which inadvertently broke coverage builds for both [Clang's source-based code coverage](https://issues.oss-fuzz.com/issues/379122777)
>
> Can you clarify that this is only for oss-fuzz, due to its use of other options and a third party script, otherwise, source based code coverage using Clang currently still works fine with master (see: [#31337 (comment)](https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2490620693)). Similarly, in the inline comment you've added, can you be a bi
...

💬 hebasto commented on pull request "build: Fix coverage builds":
(https://github.com/bitcoin/bitcoin/pull/31337#discussion_r1853686757)
The comment has been added.

💬 Sjors commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2493428765)
Added a commit to enable the `native_asan` and `native_valgrind` jobs, to get a more update to date log.

💬 willcl-ark commented on pull request "dbwrapper: Bump LevelDB max file size to 128 MiB to avoid system slowdown from high disk cache flush rate":
(https://github.com/bitcoin/bitcoin/pull/30039#issuecomment-2493431874)
I would also support slightly reducing the value in this PR, my preference though would be 32MB, for these reasons:

- The benchmark data shows the biggest gains come from the initial increases (2MB → 4MB → 8MB)
- There are diminishing returns after 8MB, with 128MB actually performing slightly worse than 64MB in total time in some of the benchmarks above
- Most of the performance gains are captured by the 32MB size, esp. when including HDDs
- Smaller files will also:
- Be more manageable
...

💬 maflcko commented on pull request "ci: Skip broken Wine64 tests by default":
(https://github.com/bitcoin/bitcoin/pull/31284#issuecomment-2493435481)
The false positives keep coming in, so it would be good to make progress here:

https://cirrus-ci.com/task/4830737755537408?logs=ci#L2355

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853700315)
given that checking the args is not for free (triggers locks and merges from multiple sources), maybe we could introduce a `IsArgEnabled` which gets rid of this confusing combination (where `-noconf=0` would be set and not negated, I think)

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853683566)
What's the reason for not wanting an `about` window regardless of the `-version` flag's value? Doesn't `gArgs.IsArgSet("-version")` make more sense here?