💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853379963)
Given that currently the CI checks every commit, do we have a way to specifying to the CI that the test commit is meant to fail, i.e. demonstrates the need for the upcoming fix? Or would we add an ignored test before the fix and just unignore it with the fix? I still think that in the current setup it makes more sense to commit them as a singe work-unit - fixes and testing and benching and documenting them are not extras, they're part of the same work unit.

💬 maflcko commented on pull request "build: Fix coverage builds":
(https://github.com/bitcoin/bitcoin/pull/31337#issuecomment-2493082250)
Fixed the docs in https://github.com/llvm/llvm-project/commit/994c544c18c86cbdb6536aae5d27ef7e2f592486

💬 Sjors commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#issuecomment-2493170407)
re-utACK 73db95c65c1d372822166045ca8b9f173d5fd883

I also checked that `submitblock` in the new test fails with `duplicate` if you revert 1f7fc738255205a64374686aca9a4c53089360f1.

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