👍 stickies-v approved a pull request: "kernel: Move kernel-related cache constants to kernel cache"
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2552509159)
ACK 430d5feee9b8be3a85e85696c449753783301c2b

💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1916557354)
in 7993f26173d911d3773cb2580938ac9aad8ad31b

You could make this test platform independent with:

<details>
<summary>git diff on 7993f26173</summary>

```diff
diff --git a/src/test/util_tests.cpp b/src/test/util_tests.cpp
index 9991b1dda5..5bde29d0bb 100644
--- a/src/test/util_tests.cpp
+++ b/src/test/util_tests.cpp
@@ -1970,7 +1970,9 @@ BOOST_AUTO_TEST_CASE(mib_string_literal_test)
{
BOOST_CHECK_EQUAL(0_MiB, 0);
BOOST_CHECK_EQUAL(1_MiB, 1024 * 1024);
- BOOST_CHECK_
...

💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1916686135)
I think these should be `size_t` too?

💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1916694318)
Alright, I'm on board too. I liked how the dual-type implementation made using the functions without casting and clamping easier (and as such perhaps a bit safer), but I agree that from a single-responsibility principle the current approach makes more sense, and it is consistent.

💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1916527403)
in 7993f26173d911d3773cb2580938ac9aad8ad31b:

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

```suggestion
if (input > (std::numeric_limits<T>::max() >> shift) ||
input < (std::numeric_limits<T>::min() >> shift)) {
return std::nullopt;
}
```

💬 instagibbs commented on pull request "test: add coverage for immediate orphanage eviction case":
(https://github.com/bitcoin/bitcoin/pull/31628#issuecomment-2592931960)
@rkrux If you can suggest alternative code that deterministically picks the right orphan to auto-evict, I would appreciate it.

💬 rkrux commented on pull request "test: add coverage for immediate orphanage eviction case":
(https://github.com/bitcoin/bitcoin/pull/31628#issuecomment-2592949092)
Oh yeah I recall that a randomly chosen orphan is evicted, I see the issue with adding this portion in the `test_max_orphan_amount` test now.

💬 0xB10C commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2592963244)
> Prevent startup when user provided a -blockreservedweight lower than 2000 WU

Maybe @wangchun from F2Pool can confirm if this would work for F2Pool and how many weigh units they currently reserve. And if they do it with a custom patch or `-maxblockweight`.

💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1916720450)
Must have dropped this by mistake during one of the rebases.

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1916714492)
dc6393cb93c4851a363b69fd474656cac1ae3b3b: `EventReadyToSend` doesn't exist yet in this commit, so maybe the commit message can announce it.

🤔 Sjors reviewed a pull request: "Split CConnman"
(https://github.com/bitcoin/bitcoin/pull/30988#pullrequestreview-2552832660)
Concept ACK

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