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

💬 ryanofsky commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1752039301)
In commit "build: Propagate `-fdebug-prefix-map` flags to secp256k1 subtree" (b9fb3d1d99ae0b178b31c654b03a3b4f4a449fe7)

Is there some reason why only secp256k1 is specified here, not other subprojects like leveldb and crc32c? Would be good to expand comment to say whether secp256k1, or if this also makes sense to apply other projects.

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

Would be good to add a comment explaining `if(CMAKE_GENERATOR MATCHES "Ninja|Makefiles")` saying that COMPILER_LAUNCHER variable only works for these backends not other backends. Otherwise the check seems completely arbitrary and doesn't make sense.

💬 mzumsande commented on issue "28.0rc1 synchronizes much slower on Windows":
(https://github.com/bitcoin/bitcoin/issues/30833#issuecomment-2340998016)
I managed to get my windows installation compiled (Windows 10 Pro 22H2), didn't observe a notable difference in the benchmarks.
I'm still in the process of trying to test the sync speed, but if it's due to the undo data, then a reindex-chainstate should be sufficient to see the difference?! Did anyone try that already?

💬 hebasto commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1752106537)
For now, only the `secp256k1` subtree uses its own build system. All other subtrees' targets consume the `core_interface`, which propagates these flags to them.

💬 l0rinc commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1752114277)
Thanks @maflcko.
@dergoegge, would a `CallOneOf` based solution satisfy your needs?

💬 hebasto commented on issue "build: reproducibility issue with macOS Guix builds":
(https://github.com/bitcoin/bitcoin/issues/30815#issuecomment-2341060674)
> Any idea why this happens?
>
> Is it something else from the context outside GUIX that leaks in? Umasks? File system differences?

I can confirm that the problem is caused by different `umask` values for the Linux users running the `./contrib/guix/guix-build` script`.

> We can likely work around this at the cmake level but ideally the guix build shell would prevent this from happening in the first place.
>
> Edit: oh i think i see, guix mounts the git directory into its build enviro
...

💬 ryanofsky commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1752126150)
re: https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1752106537

> For now, only the `secp256k1` subtree uses its own build system. All other subtrees' targets consume the `core_interface`, which propagates these flags to them.

Thanks! Might be good to add "because secp256k1, unlike other subprojects, does not use core_interface"

💬 fanquake commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1752127638)
I don't think "which apply to both C++ and C, " is relevant here. At least, mentioning C++ isn't relevant for secp256k1.

👋 hebasto's pull request is ready for review: "build: Use CMake's default permissions in macOS `deploy` target"
(https://github.com/bitcoin/bitcoin/pull/30838)