👍 hebasto approved a pull request: "build: drop obj/ subdirectory for generated build.h"
(https://github.com/bitcoin/bitcoin/pull/30856#pullrequestreview-2292424496)
ACK 820bae4708b013a1634bc62e0c6b56c9688ac645.

I wonder if we can get rid of the `HAVE_BUILD_INFO` macro simultaneously or in a follow-up.

💬 hebasto commented on pull request "build: Introduce "Kernel" installation component":
(https://github.com/bitcoin/bitcoin/pull/30835#issuecomment-2340716861)
cc @fanquake @ryanofsky

💬 naumenkogs commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#issuecomment-2340717004)
ACK f1704c021e615f740130fff7cd47093d708a3028

👍 naumenkogs approved a pull request: "net: call `Select` with reachable networks in `ThreadOpenConnections`"
(https://github.com/bitcoin/bitcoin/pull/29436#pullrequestreview-2292430025)
ACK f1704c021e615f740130fff7cd47093d708a3028

💬 naumenkogs commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1751912464)
8a82ab9840a46f6382fca92b4f6bb60e5fc5ac6d

nit
Deserves commenting what if `preferred_net` is not set (Well I guess there is already a comment "empty = all").

💬 theStack commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2340770254)
> I wonder if we can get rid of the `HAVE_BUILD_INFO` macro simultaneously or in a follow-up.

Sounds reasonable and simple enough (if I didn't miss anything), added a commit for that in this PR.

🤔 ryanofsky reviewed a pull request: "multiprocess: Add IPC wrapper for Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30510#pullrequestreview-2292507327)
Rebased 61c7ad56818888b2ee07db02134456aaf3e9aedc -> 9c98c42a0166e9e201f6e9d32a0692fad5a185f0 ([`pr/mine-types.8`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.8) -> [`pr/mine-types.9`](https://github.com/ryanofsky/bitcoin/commits/pr/mine-types.9), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine-types.8-rebase..pr/mine-types.9)) adding suggested cmake comment, fixing depends package which pointed at an old libmultiprocess version causing https://cirrus-ci.com/task/465
...

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1751966743)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1751359880

> On Intel macOS 14.6.1 this doesn't work for me (CI feels the same).
>
> ```
> git clean -dfx
> cmake -B build -DWITH_MULTIPROCESS=true
> ...
> -- Configuring done (29.3s)
> CMake Error at src/CMakeLists.txt:335 (add_dependencies):
> The dependency target "bitcoin_ipc_headers" of target "bitcoin_ipc_test"
> does not exist.
> ```

Thanks, good catch. The `bitcoin_ipc_headers` target didn't exist because
...

💬 ryanofsky commented on pull request "multiprocess: Add IPC wrapper for Mining interface":
(https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1751951659)
re: https://github.com/bitcoin/bitcoin/pull/30510#discussion_r1751353778

> [988dace](https://github.com/bitcoin/bitcoin/commit/988dacead4a9a6850b767a8ced0c08b47fece56d): can you add a comment for why this isn't in `src/test/CMakeFile.txt`? A tl&dr of https://github.com/bitcoin/bitcoin/pull/30510/files#r1747656292

Thanks, added comment

💬 maflcko commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1751986876)
There is also an overhead that we probably don't want a separate fuzz target for every trivial function, because each fuzz target may be put into a separate binary and is allocated a separate folder. So in very trivial cases, I think it is fine to combine several trivial function calls into one fuzz target.

Encode/Decode of a base seems to be trivial enough. I think the previous CI timeout may be unrelated https://github.com/bitcoin/bitcoin/pull/30746#issuecomment-2323871915 , because there a
...

💬 ryanofsky commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#discussion_r1752002276)
> For new code

Thanks, this code was written 2018-08-23

💬 maflcko commented on pull request "net: call `Select` with reachable networks in `ThreadOpenConnections`":
(https://github.com/bitcoin/bitcoin/pull/29436#discussion_r1752007850)
Is there a reason why the entries are shuffled before inserting them into an unordered set? if there is a reason, it seems that shuffling of a handful of values can be done more efficiently than consuming a full 256 bits.

💬 maflcko commented on pull request "build: Use correct variable name":
(https://github.com/bitcoin/bitcoin/pull/30791#issuecomment-2340860619)
This uncovers a bug in the build system: Building with a fuzz engine fails, because the ctime tests are auto-detected in cmake, based on whether or not valgrind-devel is installed or not.

Steps to reproduce:

* Ensure valgrind devel package is installed
* Call `rm -rf ./bld-cmake/ && cmake -B ./bld-cmake -DCMAKE_C_COMPILER='clang' -DCMAKE_CXX_COMPILER='clang++' -DBUILD_FOR_FUZZING=ON -DSANITIZERS=fuzzer` and observe that the output says " ctime_tests ......................... ON"
* Call
...

👍 ryanofsky approved a pull request: "build: Introduce "Kernel" installation component"
(https://github.com/bitcoin/bitcoin/pull/30835#pullrequestreview-2292601777)
Code review ACK 7b04fabe2d95f05a295b1ef30c9aeab7f753ed46

>```
>$ rm -rf build && cmake -B build -DBUILD_KERNEL_LIB=ON
>$ cmake --build build --target bitcoinkernel
>$ cmake --install build --component Kernel --prefix /home/hebasto/INSTALL
>```

Might be nice to document the steps for building and installing one component somewhere. I'm not sure if there is kernel library documentation where these steps could go.

🤔 hodlinator reviewed a pull request: "util: Use consteval checked format string in FatalErrorf"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2292377447)
Reviewed faa32adbcf4c04f0a426eaba4a43b29a293de72b

`git range-diff master fa72ce6 faa32ad`

- Implemented positional args after me finally communicating clearly enough.
- Added `ConstevalFormatString` overload directly into the `tinyformat` namespace to ease future adoption.
- Uses more terse lambdas instead of explicit `struct`s for checking exceptions. :+1:

Does not implement support for '*', but not currently used anyway:
https://github.com/bitcoin/bitcoin/blob/c3af4b1ec3fdb308404
...

💬 hodlinator commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1751893100)
nit:
```suggestion
unsigned count_normal{0}; // Number of "normal" specifiers, like %s
```
"nor" collides with the conjunction word "nor" and with https://en.wikipedia.org/wiki/Logical_NOR

💬 hodlinator commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1752026952)
Tried adding
```
ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%02d");
```
here and it fails.

Ran
`git grep -E '"[^"]*%[0-9]+[aAcdfeEfFgGinopsuxX*][^"]*' src/ ':(exclude)src/leveldb' ':(exclude)src/secp256k1' ':(exclude)*.ts'`
and it feels like a common enough case to want to have fixed width formatting (*optionally* `0`-padded). Examples:
```
src/netbase.cpp: LogError("Proxy requested wrong authentication method %02x\n", pchRet1[1]);
src/flatfile.cpp:
...

💬 hodlinator commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1751902511)
nit: Possibly slightly more optimal to encourage compile time length calculation for the string literal on the right. But maybe compilers already do the `const char[]` -> `string_view` conversion for the literal on the right at compile time to later match `string_view::operator==` at runtime.
```suggestion
auto check_mix{[](const ErrType& str) { return str == std::string_view{"Format specifiers must be all positional or all non-positional!"}; }};
```

👍 ryanofsky approved a pull request: "build: Improve `ccache` performance for different build directories"
(https://github.com/bitcoin/bitcoin/pull/30861#pullrequestreview-2292625730)
Code review ACK 66b73bbf375ff96bf4a77539c9cf3fe52fbc15d5. This seems like a really helpful improvement, but did have some questions about it below.

💬 ryanofsky commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1752076990)
In commit "build: Improve `ccache` performance for different build directories" (66b73bbf375ff96bf4a77539c9cf3fe52fbc15d5)

Reading https://ccache.dev/manual/latest.html#_configuration_options it seems like it would make sense in most cases to set base_dir to CMAKE_SOURCE_DIR not CMAKE_BINARY_DIR.

For example if CMAKE_SOURCE_DIR is /home/bitcoin and CMAKE_BINARY_DIR is /home/bitcoin/build, it sounds like ccmake will *not* rewrite the include path `-I/home/bitcoin/src` to `-I../src` for grea
...