👍 brunoerg approved a pull request: "test: Add expected result assertions"
(https://github.com/bitcoin/bitcoin/pull/31656#pullrequestreview-2552630267)
code review ACK dd4f894c58a8f220da224bd220cdad8cd810099a

This is just a simple addition to the test case "Test that two less valuable UTXOs with a combined lower weight are preferred over a more valuable heavier UTXO" and does not increase any test coverage.

For this case, we currently only check whether coin grinder was able to return a result. Now, it checks if the result is the expected one.

🤔 Missjones2829 reviewed a pull request: "[28.x] 28.1 backports and final changes"
(https://github.com/bitcoin/bitcoin/pull/31594#pullrequestreview-2552722992)
Merge

💬 laanwj commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1916637752)
i would really prefer not to bring back use of `strtol` in C++ code; it has some known issues with locale-dependence (especially on Linux). what about:

```c++
#include <charconv>
...
std::vector<unsigned char> hex_string_to_char_vec(const std::string& hex)
{
std::vector<unsigned char> bytes;

for (size_t i{0}; i < hex.length(); i += 2) {
unsigned int val{0};
auto [p, ec] = std::from_chars(hex.data() + i, hex.data() + i + 2, val, 16);
if (ec == std::er
...

💬 laanwj commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1916639299)
missing `#include <vector>`

💬 maflcko commented on issue "MSVC 17.12.0 internal compiler error ":
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2592864064)
Could create a new issue? Maybe it hit an automated filter?

💬 fanquake commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2592867952)
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?

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1916636569)
14fcef6b0d1d1fa9395f9af2bafbf3de63d14ac2: I find this confusing.

I think the problem starts with the strange naming of `ThreadI2PAccept`, instead of `ThreadI2PHandler` akin to `ThreadSocketHandler`.

Anyway, what that thread seems to do is, in a loop, try to `Listen` on the socket and once listening, `Accept(` new connections.

Whenever listening fails or succeeds it calls `EventI2PListen` which then calls `AddLocal` / `RemoveLocal` to keep the announced public addresses up to date.

Wi
...

💬 brunoerg commented on pull request "fuzz: Expand script verification flag testing to segwit v0 and tapscript":
(https://github.com/bitcoin/bitcoin/pull/31460#issuecomment-2592880804)
> SigVersion::WITNESS_V0 and SigVersion::TAPSCRIPT scripts are currently not covered by this test, as fuzzers are blocked from e.g. creating a valid taproot script path spend commitment.

The same applies to `signature_checker` and `eval_script` harnesses?

💬 naiyoma commented on pull request "test: Add test for rpcwhitelistdefault":
(https://github.com/bitcoin/bitcoin/pull/29858#discussion_r1916676445)
I agree that renaming and calling` test_users_permissions` twice is cleaner and less confusing. For consistency, I'll delete the following redundant code from `run_test()` since whitelisted permissions will now be tested in `test_users_permissions()`:


```diff
index b6477323b3..cea558300e 100755
--- a/test/functional/rpc_whitelist.py
+++ b/test/functional/rpc_whitelist.py
@@ -73,10 +73,6 @@ class RPCWhitelistTest(BitcoinTestFramework):
self.restart_node(0)

for user
...

👍 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