💬 stickies-v commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1153145784)
```suggestion
const CService ui_proxy{ui_proxy_netaddr.value_or(CNetAddr{}), ui->proxyPort->text().toUShort()};
```

💬 stickies-v commented on pull request "p2p: cleanup `LookupIntern`, `Lookup` and `LookupHost`":
(https://github.com/bitcoin/bitcoin/pull/26261#discussion_r1153136412)
Largely unrelated, so just leaving this here for follow-up. I think this can be cleaned up quite a bit:

<details>
<summary>git diff</summary>

```diff
diff --git a/src/httpserver.cpp b/src/httpserver.cpp
index 49c2a85e1..46d095633 100644
--- a/src/httpserver.cpp
+++ b/src/httpserver.cpp
@@ -311,17 +311,17 @@ static bool HTTPBindAddresses(struct evhttp* http)
}

// Bind addresses
- for (std::vector<std::pair<std::string, uint16_t> >::iterator i = endpoints.begin(); i
...

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