💬 dergoegge commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1751908222)
It might be less obvious (and maybe not even a problem) for this simple harness but the decode/encode and encode/decode round-trips are different tests. Their input spaces are not the same, which means that reusing the same fuzz input for both leads to less efficient fuzzing (e.g. the corpus will include inputs that are only relevant to one of the tests but both will be executed).

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