💬 maflcko commented on pull request "blockstorage: Avoid potential Memory Leak":
(https://github.com/bitcoin/bitcoin/pull/30932#issuecomment-2362882553)
Closing as a low-quality (and obviously wrong) LLM generated bot/spam patch, with an (obviously wrong) hallucinated explanation, without any test coverage or otherwise steps to test, and finally test failures in the existing tests.

💬 maflcko commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768048796)
Not sure about adding those. The whole point of the previous pull requests was to remove this linter (https://github.com/bitcoin/bitcoin/issues/30530). Now having follow-ups (https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1767054547) that make it harder, albeit minimally, seems a step in the wrong direction.

💬 maflcko commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768048525)
not sure about removing this. Apart from Wunused, it is also required to be detected in coverage reports (which will obviously omit consteval stuff)

💬 maflcko commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768047784)
style nit (feel free to ignore): I suspect you could reduce verbosity and having to hand-format all of this by using `tuple_cat`, see https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756495304

This should also ensure that all cases are covered and wouldn't require to change the tests vectors.

👍 vasild approved a pull request: "Fix display issues for IPv6 proxy setup in Options Dialog (UI only, no functionality impact)"
(https://github.com/bitcoin-core/gui/pull/836#pullrequestreview-2317404454)
ACK fee4cba48472239f7a426db62f45c4b6af8e5ef2

👍 zaidmstrr approved a pull request: "fix: handle invalid `-rpcbind` port earlier"
(https://github.com/bitcoin/bitcoin/pull/30679#pullrequestreview-2317472339)
Code review ACK [e6994ef](https://github.com/bitcoin/bitcoin/commit/e6994efe08b282dd9e46602bcbad69567fe91dcd)
All the things seem correct as before.

📝 maflcko opened a pull request: "ci: Approximate MAKEJOBS in image build phase"
(https://github.com/bitcoin/bitcoin/pull/30935)
The `MAKEJOBS` env var is the default in image builds, which is fine, because it is only relevant when building msan (or iwyu) and only differs when setting MAKEJOBS to something other than `nproc` (currently used as an approximation).

So the normal workflow of `MAKEJOBS="-j$(nproc)" FILE_ENV="./ci/test/00_setup_env_native_msan.sh" ./ci/test_run_all.sh` already works today.

However, `MAKEJOBS="-j1" FILE_ENV="./ci/test/00_setup_env_native_msan.sh" ./ci/test_run_all.sh` does not.

This is
...

💬 maflcko commented on pull request "doc: Change `nproc` in docs to `getconf _NPROCESSORS_ONLN`":
(https://github.com/bitcoin/bitcoin/pull/30619#issuecomment-2363234138)
Just for reference, the change would be wrong anyway, because the two are used to return different values

```
$ podman run --cpuset-cpus=0-1 --rm ubuntu:noble nproc
2
$ podman run --cpuset-cpus=0-1 --rm ubuntu:noble getconf _NPROCESSORS_ONLN
16

💬 Sjors commented on pull request "Stratum v2 Transport":
(https://github.com/bitcoin/bitcoin/pull/30315#issuecomment-2363248114)
I added a commit (again) that moves `CNetMessage`, `CSerializedNetMsg` and `Transport` to `common/transport.h`. To keep the diff small it doesn't move the implementation; that could be done in a separate PR. This avoids a circular dependency between `bitcoin-node` and `bitcoin-sv2`, which doesn't cause problems here, but leads to headaches in #29432 (e.g. tests spontaneously crashing).

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2363286272)
Rebased to incorporate latest relevant changes to the build system and `AutoFile`. This now also builds on #30901 so please ignore the first commit or leave your review of it in that PR.

💬 fjahr commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1768296965)
I am now using that function. I thought there might be issue with the missing header guards but the generated files are not listed. There is now also no more comment in the generated file.

💬 hodlinator commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768296972)
Not sure it would reduce verbosity that much, but would definitely consider switching if someone took the time to provide a diff.

Compiler warnings are triggered for missing cases.

💬 hodlinator commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768304651)
Good point about coverage! Error paths at runtime should be covered by `FailFmtWithError`, but happy path might only be run at compile time. Will bring it back.

💬 hodlinator commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768298071)
I'll be happy to remove them once the linter is no longer run on CI.

💬 fjahr commented on pull request "cmake: Revamp handling of data files for `{test,bench}_bitcoin` targets":
(https://github.com/bitcoin/bitcoin/pull/30901#issuecomment-2363304611)
tACK 0c35be69cf2a18a7a9173d0f9f88116b4417c892

I have based #28792 on this now. It works and seems like a nicer way to do what I need there than the previous approach.

Take it with a grain of salt though because I don't think I am experienced enough with CMake to tell if there are any hidden issues with this approach or if there are even better ways to achieve this.

📝 l0rinc opened a pull request: "doc: Add `nproc` support for Mac through `coreutils`"
(https://github.com/bitcoin/bitcoin/pull/30936)
See: https://www.gnu.org/software/coreutils/manual/html_node/nproc-invocation.html

Revival of a failed attempt in https://github.com/bitcoin/bitcoin/pull/30619

You can test on your mac via:
```bash
% echo $(nproc)
command not found: nproc
```
```bash
% brew install coreutils
...
% echo $(nproc)
10
```

💬 l0rinc commented on pull request "doc: Change `nproc` in docs to `getconf _NPROCESSORS_ONLN`":
(https://github.com/bitcoin/bitcoin/pull/30619#issuecomment-2363312549)
Yeah, I hated how verbose that was.
But did a search again and it seems `coreutils` adds `nproc` support, so pushed a PR adding that in the OSx docs: https://github.com/bitcoin/bitcoin/pull/30936/files

💬 maflcko commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768317863)
> I'll be happy to remove them once the linter is no longer run on CI.

That seems like extra churn and merge conflicts for no good reason. It should be trivial to avoid adding lines to this file. For example a simple `#define tfm_fmt tfm::format` in the test (and using it) should avoid merge conflicts with future changes, or at least make them one-line conflicts at most.

💬 maflcko commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1768321462)
This should also ensure that all cases are covered and wouldn't require to change the tests vectors.

If you don't want to take it, that is fine. However, it would be good to at least terminate in the default case instead of silently passing. Otherwise it is unclear that all cases are covered.

💬 fanquake commented on pull request "doc: Add `nproc` support for Mac through `coreutils`":
(https://github.com/bitcoin/bitcoin/pull/30936#issuecomment-2363327675)
Cnocept nack. This is something a user can do on their own system if they want to use nproc generally. However it's not a requirement for building bitcoind.

💬 fanquake commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2363333121)
Guix build (x86_64):
```bash
f65f7839cb8ce4b194ab55a698b9840dd335d790a5499d3303cf79885405bedb guix-build-7025942687fd/output/aarch64-linux-gnu/SHA256SUMS.part
7953c080586b3500c73edd93731b8b9bcfd79ac4f5fe7953d51dc54643899255 guix-build-7025942687fd/output/aarch64-linux-gnu/bitcoin-7025942687fd-aarch64-linux-gnu-debug.tar.gz
c82339ac1813727df996deda765087d430080a62d5fff7e16f46727be703a68c guix-build-7025942687fd/output/aarch64-linux-gnu/bitcoin-7025942687fd-aarch64-linux-gnu.tar.gz
ffe2b02
...