💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2392462021)
Done

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2392462361)
It's no longer relevant, removed.

💬 achow101 commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#discussion_r2392462540)
Done

💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2392492431)
Was supposed to be for the test to work in non wallet environments, but since the test now always uses MiniWallet internally by default, this is no longer needed

Removed the line of code

📝 ryanofsky opened a pull request: "init: Signal m_tip_block_cv on Ctrl-C"
(https://github.com/bitcoin/bitcoin/pull/33511)
Signal `m_tip_block_cv` when Ctrl-C is pressed or `SIGTERM` is received, the same way it is currently signaled when the `stop` RPC is called. This lets RPC calls like `waitforblockheight` and IPC calls like `waitTipChanged` be interrupted, instead of waiting for their original timeouts and delaying shutdown.

This issue was reported by plebhash in #33463

🤔 l0rinc reviewed a pull request: "Introduce SockMan ("lite"): low-level socket handling for HTTP"
(https://github.com/bitcoin/bitcoin/pull/32747#pullrequestreview-3282387933)
I'm not immersed in this to give a meaningful ack yet, but please see a few of my comments.

My main observation is that we're introducing tests and new code at the same time and I find it hard to follow what the relation between the two is - how do I tell if they're doing what they're supposed to.
Given that this is meant to replace libevent, I wonder if we could add either a wrapper to libevent and cover it with tests as a first step (could be done in a separate standalone PR as well) and slow
...

💬 l0rinc commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2389700626)
nit: I know this was the original name as well, but variables (by their name) should reflect the purpose of their usage, not their value. `ipv6_only{1}` or `tcp_nodelay{1}` might make the context more obvious (if that's indeed what they're goal is.

💬 l0rinc commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2389703932)
if error message is tied to a `false` return value, can we return `std::optional<bilingual_str>` instead of having both a return value and an output parameter?
```C++
std::optional<bilingual_str> BindAndStartListening(const CService& to)
{
sockaddr_storage storage;
socklen_t len{sizeof(storage)};
if (!to.GetSockAddr(reinterpret_cast<sockaddr*>(&storage), &len)) {
return Untranslated(strprintf("Bind address family for %s not supported", to.ToStringAddrPort()));
}
...
```

💬 l0rinc commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2389728358)
are we testing this code in any way?
And can we enable compile and be type-checking and make it `if constexpr` instead of using macros?

💬 l0rinc commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2389732096)
seems to me we're not testing any of these unhappy paths - could we do that or is it too much work?

💬 l0rinc commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2389716205)
when`reinterpret_cast<sockaddr*>(&storage)` as in `SockMan::BindAndStartListening` and when c-style cast?

💬 l0rinc commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2389737802)
what's the difference between this an the `one` (without const) above? Can we unify their naming so that they represent the meaning of what they're storing instead of the value?

💬 l0rinc commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2389738877)
nit: most logs don't need a trailing newline anymore

💬 l0rinc commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2389762328)
I understand that we need different sizes for ipv4 and ipv6, hence these ugly polimorphism-mimicking structures, but I'm wondering if there's a way to hide this ugliness in the C++ code with either decicated structures or something lke `std::variant<sockaddr_in, sockaddr_in6>`

💬 l0rinc commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2389740440)
We don't actually need the value here, just that it exists, right?

```suggestion
Assume(std::ranges::any_of(m_listen, [&](const auto& sock) { return sock.get() == &listen_sock; }));
```

💬 ryanofsky commented on issue "`bitcoin-node` is unkillable after mining IPC connection is established":
(https://github.com/bitcoin/bitcoin/issues/33463#issuecomment-3353488242)
#33511 contains the patch above and should fix this issue

💬 polespinasa commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2392615787)
Agree it doesn't really fit. Implemented there because of similarity with `sendrawtransaction`.
Any suggestions? Maybe `rpc/rawtransaction.cpp` or `rpc/net.cpp`?

💬 m3dwards commented on pull request "depends: Update URL for `qrencode` package source tarball":
(https://github.com/bitcoin/bitcoin/pull/33494#issuecomment-3353508042)
ACK 93a70a42d30fa2f9404b76d5bbdb5ea316fc1032

Also checked archive contents are the same as OP:
<details>
<summary>diff -r $dir1/libqrencode-4.1.1 $dir2/qrencode-4.1.1</summary>

```shell
Only in /var/folders/q9/65c9dvyj3lbdsv5zrhp_8qk80000gn/T/tmp.g0dIABqWhw/libqrencode-4.1.1: .github
Only in /var/folders/q9/65c9dvyj3lbdsv5zrhp_8qk80000gn/T/tmp.g0dIABqWhw/libqrencode-4.1.1: .gitignore
Only in /var/folders/q9/65c9dvyj3lbdsv5zrhp_8qk80000gn/T/tmp.g0dIABqWhw/libqrencode-4.1.1: .travis.yml
...

💬 davidgumberg commented on pull request "test: sock: Enable socket pair tests on Windows":
(https://github.com/bitcoin/bitcoin/pull/33506#issuecomment-3353532500)
Rebased to split into two commits, a mostly refactor commit https://github.com/bitcoin/bitcoin/pull/33506/commits/06a5140e96a75948cd2e490b414419a5485f2cfd and the commit that adds the new `CreateSocketPair()` for windows and enables Windows socket tests https://github.com/bitcoin/bitcoin/pull/33506/commits/7586205c4a5e94c7e2357d8928c68dfab95dc47a

💬 achow101 commented on pull request "[28.x] backports + 28.3rc1":
(https://github.com/bitcoin/bitcoin/pull/33476#issuecomment-3353541552)
Can you also do `gen-bitcoin-conf.sh` since that will need to update for the new defaults as well.