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

🤔 fjahr reviewed a pull request: "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`"
(https://github.com/bitcoin/bitcoin/pull/31243#pullrequestreview-2588404383)
Everything besides my `GetPrivKey` question looks good to me. Will do a final pass once I get an answer there.

💬 fjahr commented on pull request "descriptor: Move filling of keys from `DescriptorImpl::MakeScripts` to `PubkeyProvider::GetPubKey`":
(https://github.com/bitcoin/bitcoin/pull/31243#discussion_r1938345053)
Hm, I am not seeing why having two `GetPrivKey`s is better than one. The description of the commit only talks about the other one, not this one. For this one the caller still cares about the return value it seems. Can't they be unified under the `std::optional` returning variant? It seems like the variant using `FlatSigningProvider` is just used in one place in `ExpandPrivate` and it just saves us one line there.

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2629115868)
@hodlinator

Thank you for your feedback!


> ### Ninja
>
> PR summary:
>
> > _IMPORTANT._ Don't forget to install Ninja.
>
> I was able to build on Linux without ninja in my path. Guix/Windows install it automatically, so maybe the wording is a bit drastic?

Please see [this](https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2399781582) comment.

💬 yancyribbens commented on pull request "test: Add expected result assertions":
(https://github.com/bitcoin/bitcoin/pull/31656#issuecomment-2629144602)
@achow101 @sipa @sr-gi I noticed you acked the [PR](https://github.com/bitcoin/bitcoin/pull/27877) that added this test, so I think you can probably quickly review this since you understand the code. friendly ping to review.

The test claims to be testing that a "good solution" is found, although in actuality, there's only an assertion that _any_ solution is found. Coin-grinder is looking for a solution with the lowest weight and all UTXOs in the test have the same weight. Therefore, the sol
...