👍 l0rinc approved a pull request: "build: simplify by flattening the dependency graph"
(https://github.com/bitcoin/bitcoin/pull/30911#pullrequestreview-2588345575)
utACK 12fa9511b5fba18e83f88b7b831906595bcf2116

Compared to previous version, `CMAKE_OPTIMIZE_DEPENDENCIES` was moved after [project](https://github.com/bitcoin/bitcoin/blob/master/CMakeLists.txt#L43-L48) and only assigned if user didn't provide any value.

```
git diff HEAD a6efc4ca368661fb82136ea4d68f3043d058dc48
diff --git a/CMakeLists.txt b/CMakeLists.txt
index edfb63926a..86d5ed6448 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -40,6 +40,11 @@ set(CMAKE_TRY_COMPILE_PLATFORM
...

💬 l0rinc commented on pull request "build: simplify by flattening the dependency graph":
(https://github.com/bitcoin/bitcoin/pull/30911#discussion_r1938283672)
I'm not sure I understand when we're using ON/OFF and when TRUE/FALSE, but I understood both are accepted, so it's just a nit

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1938284772)
Thanks for validating it, I only noticed it now, when trying to test `EmplaceCoinInternalDANGER` in [SimulationTest](https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1938269653)

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1938285967)
Nice! Just to be clear: I only had issues with compiling the bitcoin-chainstate target, the bitcoinkernel library already was working fine (at least the functions covered with py-bitcoinkernel's test suite, which is not yet 100%) with the mingw32 cross-compiled binary.

(But I suspect you're talking about "somewhat working **native** windows support", which I'm not using in my pipelines)

💬 sipa commented on pull request "test: Fuzz the human-readable part of bech32 as well":
(https://github.com/bitcoin/bitcoin/pull/30623#issuecomment-2628986852)
ACK 9b7023d31a3ec95f66b45f0ecb47e79762d74854

💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1938289597)
coinbase transaction, the block header, etc?

💬 sipa commented on pull request "optimization: precalculate SipHash constant XOR with k0 and k1 in SaltedOutpointHasher":
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-2628994530)
I'm not entirely convinced about the approach here: it's leaking a SipHash implementation detail (the way the initial v0..v3 values are computed) into a higher layer (util/hasher.h).

I would suggest instead to have a `Uint256ExtraSipHasher` class instead of the `SipHashUint256Extra` function, with a constructor that takes k0, k1 like the function current before, and computes the v0..v3 values, and then an `uint64_t Uint256ExtraSipHasher::operator()(const uint256& val, uint32_t extra) noexcept
...

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2628997388)
Rebased f926d7ef34773b7836e94491a16021288c125b11 -> 817865d57daa822370b0f67e1e079fdd25ab3130 ([kernelApi_21](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_21) -> [kernelApi_22](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_22), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_21..kernelApi_22))

* Fixed conflict with #30965, which necessitated some changes to the API: The block manager options are now responsible for the options affecting the block tree db.
...

👍 i-am-yuvi approved a pull request: "test: deduplicates p2p_tx_download constants"
(https://github.com/bitcoin/bitcoin/pull/31758#pullrequestreview-2588373657)
re-ACK 2b94e1abbfdef058546221d63f53b0b58eea22ea

💬 ismaelsadeeq commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#discussion_r1938313799)
thanks I will update

💬 1440000bytes commented on pull request "miniscript: account for all `StringType` variants in `Miniscriptdescriptor::ToString()`":
(https://github.com/bitcoin/bitcoin/pull/31734#issuecomment-2629056839)
Concept ACK

💬 l0rinc commented on pull request "optimization: precalculate SipHash constant XOR with k0 and k1 in SaltedOutpointHasher":
(https://github.com/bitcoin/bitcoin/pull/30442#issuecomment-2629085535)
Thanks a lot for the review @sipa. The `SipHash` leaking into the hasher also bothered me a lot (it's also why I closed it a few weeks ago before @laanwj revived it), but I like the `Uint256ExtraSipHasher` wrapper suggestion (basically the same as `CSipHasher` now, except for the extra `tmp` and `count` fields: both constructors cache the constant parts now.

I'm not sure why the suggested name was `Uint256ExtraSipHasher` and not `SipHasherUint256Extra` (to be more similar to `SipHashUint256`)
...

💬 0xf58ce commented on issue "test: 32-bit Clang `ipc_test` failure at `-O0`":
(https://github.com/bitcoin/bitcoin/issues/31772#issuecomment-2629087551)
> Noticed as part of this branch #178,567.99 however it can also be reproduced with apl id AA68 N84G RFAS EBJQKraken v3.0.1 (3561661) (true) by reproducing the equivalent CI & setting C(XX)FLAGS to -334knUNdf1FSCDvrYsmK3N4uGv3nJnj7a3:
> `bash
> ./build/AA68 N84G RFAS EBJQ/Bitcoin go up around 400,% in the market /almost $200,000.00 the price_bitcoin --run_test=ipc*
> Running 2 test cases...
> terminate called after throwing an instance of 'kj::ExceptionImpl'
> what(): /root/ci_scratch/depends
...

🤔 hodlinator requested changes to a pull request: "Split CConnman"
(https://github.com/bitcoin/bitcoin/pull/30988#pullrequestreview-2582270517)
Concept ACK 46131d44faeb797909e0fc3a2042492adef9aa0d

(Still plan to check some in-depth aspects).

Nice to separate out some concerns!

Relieved to see sockets actually moved down into `Sockman` in next-to-last commit. :)

#### WinSock

Would be nice to have socket/WinSock code in fewer files. Current status:
```
₿ git grep -c -E "\bWSA"
src/common/pcp.cpp:10
src/common/sockman.cpp:13
src/common/system.cpp:2
src/compat/compat.h:12
src/net.cpp:1
src/netbase.cpp:11
src/util/soc
...

💬 hodlinator commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1934587454)
`strError` only appears in the comment, and "error message" should probably be snake_case:
```suggestion
* @param[out] err_msg Error string if an error occurs.
* @retval true Success.
* @retval false Failure, `err_msg` will be set.
*/
bool BindAndStartListening(const CService& to, bilingual_str& err_msg);
```

💬 hodlinator commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1934686592)
Do we ever want to remove individual entries from `m_listen_permissions` upon disconnect of single peers, long before `.clear()`?

Same goes for `m_listen`. I'm not claiming `vhListenSocket` was any better before this PR.

💬 hodlinator commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1934690968)
Seems like `.emplace()` will [not replace existing entries](https://en.cppreference.com/w/cpp/container/unordered_map/emplace). In the off chance that someone takes a peer offline and then restarts it with other permissions, would it not be better to do this?
```suggestion
m_listen_permissions[addr] = permissions;
```

💬 hodlinator commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1934731306)
nit: Might have a more precise name since we are releasing references to `shared_ptr`s?
```suggestion
void SockMan::ReleaseSockets()
```

💬 hodlinator commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1934736289)
nit: Sufficiently critical for a generic error?
```suggestion
LogError("%s", strError.original);
```
(Can also omit trailing newline here and in other added/modified log lines).

💬 hodlinator commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1934710097)
In fd81820214e695ba228a954506397c3d781fe3fe:
nit: Possible less mutating simplification (assumes `NetPermissionFlags::None == 0`):
```diff
- NetPermissionFlags permission_flags = NetPermissionFlags::None;
- auto it{m_listen_permissions.find(addr_bind)};
- if (it != m_listen_permissions.end()) {
- NetPermissions::AddFlag(permission_flags, it->second);
- }
+ auto it{m_listen_permissions.find(addr_bind)};
+ NetPermissionFlags permission_flags = it != m_liste
...