💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1916699333)
21c5e05619c8a6eb736bb1c61725f4b5f669ffb4: maybe use `[_, node]` in places where you don't need the `id`.

It's a bit unfortunate that you can't (?) directly loop over all `T` in `std::unordered_map<Key, T>`. Since most of the time we don't need `Key` (`id`) here.

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1916723639)
dc6393cb93c4851a363b69fd474656cac1ae3b3b: it would be good to annotate in `CConnman::SocketHandlerConnected` above `sendSet = it->second.occurred & Sock::SEND;` that `Sock::SEND` is unset when `ShouldTryToRecv` is `false`.

💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1916750160)
Mmh, yeah, I think that should work.

👍 ryanofsky approved a pull request: "kernel: Move kernel-related cache constants to kernel cache"
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2552875375)
Code review ACK 430d5feee9b8be3a85e85696c449753783301c2b. Changes since last review were adding fuzz test and changing comments as suggested and switching back from digits() to sizeof().

This all looks good, though I was curious about two things. (No need to investigate these, just curious if there is any more information)

- This is moot but I was wondering about the "I had a compiler complain about signed to unsigned integer comparison without it." https://github.com/bitcoin/bitcoin/pull/
...

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1916739868)
In commit "fuzz: Add fuzz test for checked and saturating add and left shift" (b7c8d88ec2d299d2b19f5c622ee5080cfdf8b2a0)

Could add comment to explain divide by two: "// Range needs to be at least twice as big to allow two numbers to be added without overflowing." Apparently I have goldfish memory, I wrote this code yesterday and already forgot why it was there.

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1916730308)
In commit "util: Add integer left shift helpers" (7993f26173d911d3773cb2580938ac9aad8ad31b)

intput (unless you want to coin a phrase)

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1916723745)
re: https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1916527403

> nit: I think the runtime signedness checks are unnecessary, and find `std::numeric_limits<T>::min()` a bit more readable than `-max_allowed_input - 1`, so putting it all together this can just be simplified to

Agree with this suggestion. The only reason for writing code that way was that I read "For negative a, the value of a >> b is implementation-defined" in the [c++ reference](https://en.cppreference.com/w/cpp/la
...

👍 rkrux approved a pull request: "test: add coverage for immediate orphanage eviction case"
(https://github.com/bitcoin/bitcoin/pull/31628#pullrequestreview-2552949377)
tACK 6836d428199c0c19f7034bf6ea0855b8ec0a69d0

build, functional tests pass. In agreement with the approach.

💬 rkrux commented on pull request "test: add coverage for immediate orphanage eviction case":
(https://github.com/bitcoin/bitcoin/pull/31628#discussion_r1916770664)
> normally happens randomly

Ahh sorry, I missed the randomly part.

💬 darosior commented on pull request "fuzz: Expand script verification flag testing to segwit v0 and tapscript":
(https://github.com/bitcoin/bitcoin/pull/31460#issuecomment-2593052385)
An alternative to touching consensus code to test this is to fuzz `EvalScript` directly. We already have a target for it, `eval_script`. It could be adapted to also be ran under a Tapscript context by filling dummy `ScriptExecutionData`, and the soft fork check could be added to this target. The downside of this approach is that it would not cover the `VerifyScript` logic, but in the context of making sure proposed opcodes are indeed soft forks what we really care about it `EvalScript`.

💬 marcofleon commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1916790039)
Sounds good, I kept the check and adjusted the comment in a separate commit. Let me know if it makes sense or can be improved in any way.

👍 ryanofsky approved a pull request: "refactor: Check translatable format strings at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31061#pullrequestreview-2553008614)
Code review ACK fa3efb5729091a36a0e82316e9e4b7c09115dc2e. Since last review added TranslateFn commit, clarified FormatStringCheck documentation, dropped redundant `inline` keyword

💬 theuni commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2593155279)
> Why are `bitcoin-chainstate` and `bitcoin-util` being put into libexec? `bitcoin-util` is meant to be called by users directly. Not sure why `bitcoin-chainstate` would be put there either?

Agreed. The point of libexec is (implicitly) to make it HARDER to run binaries from there, because they don't live in the user's `$PATH`. They're meant to be found by other binaries that _do_ live in `$PATH`. So long as we're recommending/relying on users running a binary by hand, it shouldn't live in lib
...

💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2593227168)
Updated 430d5feee9b8be3a85e85696c449753783301c2b -> 2a92702bafca5c78b270a9502a22cb9deac02cfc ([kernel_cache_sizes_16](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_16) -> [kernel_cache_sizes_17](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_17), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_16..kernel_cache_sizes_17))

* Took @stickies-v's [suggestion](https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1916527403) for si
...

💬 maflcko commented on pull request "[WIP] build: remove need to test for endianness":
(https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2593247012)
> MSVC

Has someone reported the request to them? If not, it seems less likely they'll fix it.

💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2593266357)
re-ACK 2a92702bafca5c78b270a9502a22cb9deac02cfc

No changes except for outstanding review comments to simplify `CheckedLeftShift()`'s implementation wrt range checks, making the `_MiB` test platform-agnostic, making `IndexCacheSizes` members `size_t` and doc/typo improvements.

📝 hebasto opened a pull request: "cmake: Do not modify `CMAKE_TRY_COMPILE_TARGET_TYPE` globally"
(https://github.com/bitcoin/bitcoin/pull/31662)
This was requested in https://github.com/bitcoin/bitcoin/pull/31359#issuecomment-2515287092.

From https://github.com/bitcoin/bitcoin/pull/31359#issuecomment-2511246212:
> (Almost?) every CMake check internally uses the [`try_compile()`](https://cmake.org/cmake/help/latest/command/try_compile.html) command, whose behaviour, in turn, depends on the [`CMAKE_TRY_COMPILE_TARGET_TYPE`](https://cmake.org/cmake/help/latest/variable/CMAKE_TRY_COMPILE_TARGET_TYPE.html) variable:
>
> 1. The defau
...

✅ fanquake closed an issue: "Nuke format linter"
(https://github.com/bitcoin/bitcoin/issues/30530)

🚀 fanquake merged a pull request: "refactor: Check translatable format strings at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31061)

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2593311499)
@fanquake @theuni

Thank you for your feedback!

> So I propose dropping the `libexec` stuff entirely for this PR.

Dropped.