💬 stickies-v commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1153170805)
This docstring is updated in ab4257be34209fd61efb409593dc7aa21dba0fb7, but there it still returns a `bool`. The signature is only updated to return `std::optional<CNetAddr>` in 30efd6d36e24c83f5c094e30f8cb25e66e913d73

Moreover, the function never returns `addresses`, it's only ever singular. I also don't like the phrasing of 'empty', so would prefer updating this to:
```suggestion
* @returns The resulting network address to which the specified host
* string resolved or std::null
...

💬 stickies-v commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1153157545)
A bit more readable and avoids default initializing CServices:

```suggestion
services.reserve(addresses.size());
for (const auto& addr : addresses)
services.emplace_back(addr, port);
```

💬 stickies-v commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1153142035)
The comment was on an older commit, so apologies if I'm missing something, but I'm not sure it's fully addressed? In `if (i->first.empty() || (addr.has_value() && addr->IsBindAny()))`, I think the first `i->first.empty()` is indeed still superfluous as theStack pointed out.

🚀 fanquake merged a pull request: "ci: use LLVM/clang-16 in native_fuzz (ASAN) job"
(https://github.com/bitcoin/bitcoin/pull/27363)

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