💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2420892943)
Good call, this value is setting a boolean option to `true` so I made the variable name more explicit, in the "modernize" commit.

💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2429400209)
I don't think we could reliably test these paths in ci because such socket operations depend so much on the platform.

We *could* create additional child classes of `DynSock` for the unit tests, so we have mock socket variants that fail each specific option, but I'm not sure we'd get enough value from those tests to be worth the work. Could be a follow up.

💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2429627125)
yes thanks

💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2429404525)
that's nice, thanks

💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2421914543)
Good call, will use `util::Result` here and remove the error string as an "out" parameter.

💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2429694223)
good call ill set a `static constexpr` for all the times we do this in the module, overriding https://github.com/bitcoin/bitcoin/pull/32747/files#r2420892943

💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2421943022)
I'll add coverage for as many as I can. But I think all I can do from this level is try to bind to an address with an invalid family (i.e. "NET_ONION").

💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2421300798)
This is a reasonable note but I think it's out of scope for this PR, since `GetSockAddr()` is used so many other places in the code. Wrapping `sockaddr_storage` for C++ style could also be applied to `Accept()`, `Connect()`, `Bind()` and probably a few others. I'll open a new PR for that cleanup, similar to https://github.com/bitcoin/bitcoin/pull/33378

💬 waketraindev commented on pull request "rpc, test: fix "internal" label check in importdescriptors and extend test":
(https://github.com/bitcoin/bitcoin/pull/33614#issuecomment-3402968735)
Fixes #32376, same underlying issue as #31514
Happy to defer to #31514 if it's preferred.

💬 sipa commented on pull request "txgraph: randomize order of same-feerate distinct-cluster transactions":
(https://github.com/bitcoin/bitcoin/pull/33335#discussion_r2430034830)
Fixed (again).

💬 sipa commented on pull request "TxGraph: change m_excluded_clusters":
(https://github.com/bitcoin/bitcoin/pull/33469#issuecomment-3403060956)
ACK 9b43428c96872f0fbbbab4c066c6010fc18c6cc4

🤔 stratospher reviewed a pull request: "p2p: Allow block downloads from peers without snapshot block after assumeutxo validation"
(https://github.com/bitcoin/bitcoin/pull/33604#pullrequestreview-3336930055)
ACK 9f946a7.

nice! I think this aligns with the original intention to restrict the peer only when background sync isn't over in https://github.com/bitcoin/bitcoin/pull/29519#discussion_r1635456871 where this code was introduced.

looks like we might incorrectly set `pindexLastCommonBlock` in [L1401](https://github.com/bitcoin/bitcoin/blob/9f946a79ca894741547c5892f9dc60d023ae1508/src/net_processing.cpp#L1401) to the previous best known block tip again in this scenario. but it's not a problem
...

💬 sdaftuar commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2430088556)
Yes I think so!

⚠️ glozow opened an issue: "Minor Release 29.3"
(https://github.com/bitcoin/bitcoin/issues/33628)
Backports
- https://github.com/bitcoin/bitcoin/pull/33611

<!--
RC
- [] final changes
- [] tag
- [] upload binaries
- [] announcements
-->

See [release process docs](https://github.com/bitcoin/bitcoin/blob/master/doc/release-process.md) for more details. This issue can be used to track status updates. Please feel free to suggest backports in the comments.

💬 brunoerg commented on pull request "test: P2SH sig ops are only counted with `SCRIPT_VERIFY_P2SH`":
(https://github.com/bitcoin/bitcoin/pull/33624#discussion_r2430105092)
Done.

💬 brunoerg commented on pull request "test: P2SH sig ops are only counted with `SCRIPT_VERIFY_P2SH`":
(https://github.com/bitcoin/bitcoin/pull/33624#discussion_r2430105307)
Done.

💬 brunoerg commented on pull request "test: P2SH sig ops are only counted with `SCRIPT_VERIFY_P2SH`":
(https://github.com/bitcoin/bitcoin/pull/33624#issuecomment-3403144907)
Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/33624#discussion_r2429811753 and https://github.com/bitcoin/bitcoin/pull/33624#discussion_r2429816763.

👍 instagibbs approved a pull request: "test: P2SH sig ops are only counted with `SCRIPT_VERIFY_P2SH`"
(https://github.com/bitcoin/bitcoin/pull/33624#pullrequestreview-3336988018)
ACK 3a10d700bc1889b3690097efc935c5a4ba5966bb

💬 brunoerg commented on pull request "docs: add doc comment for SRD selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/33622#discussion_r2430117604)
e6c439dcb23e1a0d03261734fd89412a15b1932a: I don't this it needs this "Full Description:". It seems obvious that is the description of the algorithm.

```suggestion
This algorithm works by selecting inputs randomly until the target amount is
```

💬 brunoerg commented on pull request "docs: add doc comment for SRD selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/33622#discussion_r2430157941)
Aren't there already documentation about the parameters in `coinselection.h`? If so, we should just add the description there instead.