💬 fanquake commented on pull request "test: remove `GetRNGState` lsan suppression":
(https://github.com/bitcoin/bitcoin/pull/27362#issuecomment-1490280408)
Rebased past #27363.

💬 Sjors commented on pull request "ci: use LLVM/clang-16 in native_asan job":
(https://github.com/bitcoin/bitcoin/pull/27360#issuecomment-1490298280)
I haven't tried to reproduce the original issue, since I don't have an aarch64 machine, but this works for me on Ubuntu 22.10 with AMD Ryzen 7950X:

```sh
MAKEJOBS="-j32" CCACHE_SIZE=1G FILE_ENV="ci/test/00_setup_env_native_asan.sh" ./ci/test_run_all.sh
```

💬 Sjors commented on pull request "ci: set docker run --ulimit to workaround Valgrind assertion":
(https://github.com/bitcoin/bitcoin/pull/27364#issuecomment-1490352360)
I was able to run this, at least to get past `addr_info_deserialize`, on x86_64 Ubuntu 22.10 with Docker version 23.0.2.

Potential caveat: if you're running Docker in rootless mode this override then `—ulimit` seems to be ignored by default. See https://docs.docker.com/engine/security/rootless/#limiting-resources (the instructions there worked for me)

👍 hebasto approved a pull request: "refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer"
(https://github.com/bitcoin/bitcoin/pull/26140)
ACK 3a060ae7b67cc28fc60cf28cbc518fa1df24f545

💬 akifgrape commented on pull request "ci: set docker run --ulimit to workaround Valgrind assertion":
(https://github.com/bitcoin/bitcoin/pull/27364#issuecomment-1490368397)
> aarch64'te (Fedora 37) , çalıştırırken `native_fuzz_with_valgrind_job`aşağıdakileri gördüm:
>
> ```shell
> Run addr_info_deserialize with args ['valgrind', '--quiet', '--error-exitcode=1', '/home/fedora/ci_scratch/ci/scratch/build/bitcoin-aarch64-unknown-linux-gnu/src/test/fuzz/fuzz', '-runs=1', '/home/fedora/ci_scratch/ci/scratch/qa-assets/fuzz_seed_corpus/addr_info_deserialize']
> valgrind: m_libcfile.c:66 (vgPlain_safe_fd): Assertion 'newfd >= VG_(fd_hard_limit)' failed.
>
>
> val
...

💬 hebasto commented on pull request "refactor: Continue moving application data from CNode to Peer":
(https://github.com/bitcoin/bitcoin/pull/26621#issuecomment-1490375127)
Concept ACK.

💬 mzumsande commented on issue "Fix net grouping of non-IP networks":
(https://github.com/bitcoin/bitcoin/issues/27369#issuecomment-1490395263)
fyi @amitiuttarwar @naumenkogs @sdaftuar

I think there are two aspects to this that don't necessarily need be addressed at the same time:
1) The rule not to make more than one outbound connection to a netgroup
2) Netgroup-based bucketing in addrman

I think 1) is more urgent right now, because the scenario outlined in https://github.com/bitcoin/bitcoin/pull/27264#issuecomment-1481322628 - it's likely that there are nodes out there that are `-onlynet=onion` and try to compensate for the ad
...

💬 sdaftuar commented on issue "Fix net grouping of non-IP networks":
(https://github.com/bitcoin/bitcoin/issues/27369#issuecomment-1490435928)
> [#27264 (comment)](https://github.com/bitcoin/bitcoin/pull/27264#issuecomment-1490154556) - workaround it by not inserting tor/i2p/cjdns address in `setConnected` for now? so `GetGroup` wouldn't be called on these networks.

This seems clearly correct to me -- I think it was an oversight that `setConnected` picks up netgroups for non-IPV4/IPV6 networks in the first place.

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