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

💬 polespinasa commented on issue "Mempool Expiry eviction might remove txs that could be mined in the next block":
(https://github.com/bitcoin/bitcoin/issues/33510#issuecomment-3353606181)
How about we make the expiration time dynamic based on the position (computer by fee rate) in the mempool?

Txs more likely to be mined have a bigger expiry time. If the mempool clears a bit, txs with low feerate will be on top of the mempool and their expiry time can be re-calculated to increase and avoid evicting txs that will be mined.

This way transactions more likely to be mined will not be evicted while the txs less likely to be mined will be evicted and only kept if the mempool clears a
...

💬 mzumsande commented on pull request "test: fix p2p_leak_tx.py":
(https://github.com/bitcoin/bitcoin/pull/33121#discussion_r2392697933)
done with the latest push. I also adjusted the third commit with the new test in a similar way.

💬 mzumsande commented on pull request "test: fix p2p_leak_tx.py":
(https://github.com/bitcoin/bitcoin/pull/33121#issuecomment-3353612781)
[a52c148](https://github.com/bitcoin/bitcoin/commit/a52c148ece2296f22e2fe1b7e9584d49e23f03f2) to [14ae71f](https://github.com/bitcoin/bitcoin/commit/14ae71f323dd011c6d51470ea15cf00750970f65):
- Used mocktime everywhere to speed up the test.
- corrected `on_inv` override variable name in 1st commit (my IDE complained about that)

💬 achow101 commented on pull request "test: Use extra_port() helper in feature_bind_extra.py":
(https://github.com/bitcoin/bitcoin/pull/33260#issuecomment-3353680023)
ACK fabc2615af26c61a503f23ae4fd0353f90602bbe

💬 polespinasa commented on pull request "RPC: add sendrawtransactiontopeer":
(https://github.com/bitcoin/bitcoin/pull/33507#discussion_r2392760028)
You're right! Done :)