💬 hebasto commented on pull request "Fixes compile errors in MSVC build #27332":
(https://github.com/bitcoin/bitcoin/pull/27335#issuecomment-1490246908)
> The fact that the version of libevent was being pinned to the earlier version explains why testing didn't catch this.

To be precise, the vcpkg version is pinned now. Maybe actual pinning `libevent` version, similar to https://github.com/bitcoin/bitcoin/commit/20b6c871178f20661b849ad5677bd8ecae55cf19, could provide a better user experience of native building on Windows. Otherwise, with the current suggested approach, Windows users will be forced to update their vcpkg installations.

cc @si
...

💬 RandyMcMillan commented on pull request "build: produce a .zip for macOS distribution":
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1490249383)
note:
a bundled README should be in README.txt format - we can't assume the user can open .md files by default.

👋 jonatack's pull request is ready for review: "Add "warnings", deprecate "warning" in {create,load,unload,restore}wallet"
(https://github.com/bitcoin/bitcoin/pull/27279)

💬 ismaelsadeeq commented on pull request "test: refactor: dedup mempool_package_limits.py subtests via decorator":
(https://github.com/bitcoin/bitcoin/pull/27350#discussion_r1153162959)
Nice :100:
Just a couple of nit minor suggestions to improve the readability of the code:

Add a blank line after the first assertion assert_equal(0, node.getmempoolinfo()["size"])
Add a blank line after the line where package_hex is assigned to func(self, *args, **kwargs)

Separating the operations as written in the decorator comment, also goes well with PEP9 guidelines.

💬 dergoegge commented on pull request "refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer":
(https://github.com/bitcoin/bitcoin/pull/26140#issuecomment-1490261057)
Rebased

💬 jonatack commented on pull request "rpc, test: remove newline escape sequence from wallet warning fields":
(https://github.com/bitcoin/bitcoin/pull/27138#issuecomment-1490264044)
Based on the discussion above, https://github.com/bitcoin/bitcoin/pull/27138 is now ready. It adds a `warnings` field to these four RPCs and deprecates the `warning` fields. Its first commit implements the doc suggestion in https://github.com/bitcoin/bitcoin/pull/27138#issuecomment-1474789198 as an alternative to this pull to see what reviewers prefer.

👋 fanquake's pull request is ready for review: "test: remove `GetRNGState` lsan suppression"
(https://github.com/bitcoin/bitcoin/pull/27362)

💬 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).