📝 stratospher opened a pull request: "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns"
(https://github.com/bitcoin/bitcoin/pull/27374)
Follow up for #27264.

In order to make sure that our persistent outbound slots belong to different netgroups, distinct net groups of our peers are added to `setConnected`. We’d only open a persistent outbound connection to peers which have a different netgroup compared to those netgroups present in `setConnected`.

Current `GetGroup()` logic assumes route-based diversification behaviour for tor/i2p/cjdns addresses (addresses are public key based and not route-based). Distinct netgroups poss
...

💬 stratospher commented on issue "Fix net grouping of non-IP networks":
(https://github.com/bitcoin/bitcoin/issues/27369#issuecomment-1490464682)
opened https://github.com/bitcoin/bitcoin/pull/27374 to skip tor/i2p/cjdns addresses in `setConnected` for now.

💬 hebasto commented on pull request "refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer":
(https://github.com/bitcoin/bitcoin/pull/26140#issuecomment-1490501853)
FWIW, I have no issues with TSan-instrumented builds locally using both clang-14 and clang-15. Also the "TSan, depends, gui" CI task passed for me locally.

👍 sdaftuar approved a pull request: "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns"
(https://github.com/bitcoin/bitcoin/pull/27374)
utACK.

I think the other way we could write this would be to leave `setConnected` unchanged, and change the logic where we look up in `setConnected` to only do the lookup for non-IPV4/IPV6 peers. It doesn't seem to me like it matters much which way we do this, since I believe netgroups for different networks cannot collide, so this looks fine to me.

💬 sdaftuar commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#discussion_r1153449034)
nit: If you touch this again, I think it would be worth adding a comment explaining why this code is here (and probably also add a comment to where the lookup in `setConnected` happens down below, near line 1806, so that if someone is touching the code in just one place they are aware of this issue).

💬 EthanHeilman commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#issuecomment-1490540612)
@hebasto Thats a great point. I had only been thinking of users that are in the same boat as me, doing a fresh MSVC build, but you are right, most users are likely building against pre-installed packages. I don't think my PR should be merged until it either:

* Adding the ability to define the function signature based on the actual version of libevent installed
OR
* Pinning the version of libevent in vcpkg.json so that users with old dependencies upgrade when they run build

I'm fairly al
...

💬 vasild commented on pull request "addrman: Enable selecting addresses by network":
(https://github.com/bitcoin/bitcoin/pull/27214#discussion_r1153482086)
I ran this on a mainnet `peers.dat`:
```
addrman size total: 72947
addrman size IPv4: 47252
addrman size IPv6: 11491
addrman size Tor: 13305
addrman size I2P: 893
addrman size CJDNS: 6
```

<details>
<summary>[patch] How to load /tmp/peers.dat from bench_bitcoin</summary>

```diff
diff --git i/src/bench/addrman.cpp w/src/bench/addrman.cpp
index 8a5cab443f..b8ec4a4071 100644
--- i/src/bench/addrman.cpp
+++ w/src/bench/addrman.cpp
@@ -1,17 +1,22 @@
// Copyright (c) 2020-2
...

🚀 glozow merged a pull request: "refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer"
(https://github.com/bitcoin/bitcoin/pull/26140)

💬 jnewbery commented on pull request "refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer":
(https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1153498543)
Would it make sense to pass in a `const TxRelay&` here instead of a `const Peer&`?

💬 jnewbery commented on pull request "refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer":
(https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1153499627)
Can you remove the `LOCK(cs_main)` now (and the `LOCKS_EXCLUDED(cs_main)` annotation from the function declaration)?

💬 MarcoFalke commented on pull request "move-only: IsDeprecatedRPCEnabled to rpc/util to fix CI builds when called from wallet":
(https://github.com/bitcoin/bitcoin/pull/27322#issuecomment-1490625756)
Actually, on a second though. Any reason why you can't just call `rpcEnableDeprecated`?

💬 pablomartin4btc commented on pull request "httpserver, rest: fix segmentation fault on evhttp_uri_get_query":
(https://github.com/bitcoin/bitcoin/pull/27253#issuecomment-1490626840)
> I think that is fine because if the input is such that `HTTPRequest` constructor throws, then we need not to test those because they will never happen in the real code either.

Thanks @vasild, it makes sense what you are saying, I'm still playing with some options and I'll get back to you soon.

💬 MarcoFalke commented on pull request "ci: cleanup of CI_EXEC & CI_EXEC_ROOT":
(https://github.com/bitcoin/bitcoin/pull/27333#issuecomment-1490646824)
Meta note: The author needs to reply to the open discussions. Any reply is fine, but maintainers are likely waiting for it.

🚀 glozow merged a pull request: "test: refactor: dedup mempool_package_limits.py subtests via decorator"
(https://github.com/bitcoin/bitcoin/pull/27350)

✅ fanquake closed an issue: "ci: Cleanup ci/test/01_base_install.sh"
(https://github.com/bitcoin/bitcoin/issues/27321)

🚀 fanquake merged a pull request: "ci: cleanup of CI_EXEC & CI_EXEC_ROOT"
(https://github.com/bitcoin/bitcoin/pull/27333)

🚀 fanquake merged a pull request: "guix: use python-minimal (3.9)"
(https://github.com/bitcoin/bitcoin/pull/27361)

💬 dergoegge commented on pull request "refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer":
(https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1153606022)
Yes, seems like it. Will do this in a follow up (including your other suggestion), thanks!

💬 mzumsande commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#discussion_r1153586371)
How about putting this into a seperate function (something like `bool LimitOutboundByNetgroup(Network n)`), and using a `switch` statement there, similar to the existing `IsAddrV1Compatible()`? That way, the compiler would prevent us from forgetting about this if we ever add another network.

💬 hebasto commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#issuecomment-1490737465)
> I'm fairly allergic to the idea of build scripts inspecting function signatures and then defining macros based on that.

FWIW, the [coming](https://github.com/bitcoin/bitcoin/pull/25797) CMake-based build system will do it :)