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

💬 hodlinator commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1934755815)
nit: listen to -> listen on?
```suggestion
errmsg = strprintf(_("Cannot listen on %s: %s"), to.ToStringAddrPort(), NetworkErrorString(WSAGetLastError()));
```

💬 hodlinator commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1934748506)
Problem: " (<CLIENT_NAME> already running?)"-`string` is untranslated.

Would suggest bringing back old strings to reduce translation churn, or ensuring the new version is fully translated. Old strings:
```suggestion
if (err == WSAEADDRINUSE) {
err_msg = strprintf(_("Unable to bind to %s on this computer. %s is probably already running."), to.ToStringAddrPort(), CLIENT_NAME);
} else {
err_msg = strprintf(_("Unable to bind to %s on this computer (bin
...

💬 hodlinator commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1934722181)
nit: The comment adds no information.
```suggestion
std::vector<std::shared_ptr<Sock>> m_listen;
```
Same for `m_unused_i2p_sessions_mutex`.

💬 hebasto commented on issue "build: use UCRT runtime for Windows (release) binaries":
(https://github.com/bitcoin/bitcoin/issues/30210#issuecomment-2629109656)
> - [ ] Add CI to cover new runtime.

I've [added](https://github.com/hebasto/bitcoin-core-nightly/pull/39) a nightly build with UCRT to https://github.com/hebasto/bitcoin-core-nightly.

> Inconsistency in the binaries leading to more workarounds/disabling test code: [#31410](https://github.com/bitcoin/bitcoin/pull/31410).

It seems that the mentioned issue requires further investigation, as the fix is still needed.

💬 hebasto commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#issuecomment-2629110703)
FWIW, the Windows UCRT-linked cross-compiled builds still [need](https://github.com/hebasto/bitcoin-core-nightly/pull/39) this fix.