💬 hebasto commented on pull request "Renamed UniValue::__pushKV to UniValue::pushKVEnd.":
(https://github.com/bitcoin/bitcoin/pull/27822#issuecomment-1576402224)
Does it make sense to force such changes with the Clang's [`-Wreserved-identifier`](https://clang.llvm.org/docs/DiagnosticsReference.html#wreserved-identifier) diagnostic flag?

💬 pablomartin4btc commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1217755620)
Those are needed, I've checked the test code line by line. I'll do another check while I fix the current issue/ failure. Thanks!

💬 willcl-ark commented on pull request "contrib: verify torrents with verify-binary.py":
(https://github.com/bitcoin/bitcoin/pull/27762#discussion_r1217758935)
Thanks fixed in next push

💬 willcl-ark commented on pull request "contrib: verify torrents with verify-binary.py":
(https://github.com/bitcoin/bitcoin/pull/27762#discussion_r1217759061)
Thanks fixed in next push

💬 MarcoFalke commented on issue "Build fails on Fedora 36 - configure failed for src/secp256k1":
(https://github.com/bitcoin/bitcoin/issues/27808#issuecomment-1576412348)
Does it happen after a `make clean && make distclean` ?

💬 willcl-ark commented on pull request "contrib: verify torrents with verify-binary.py":
(https://github.com/bitcoin/bitcoin/pull/27762#issuecomment-1576414179)
> Do we want to use the .asc file from the torrent, or should we just get it from the Guix signature repo? The latter may have more signatures, as they keep coming in after release.

I agree that the guix sig repo will likely have more signatures, I only worry that for privacy-concious folks, for whom either bitcoincore.org is blocked or they wish to remain hidden, automatically pinging a (different) specific website might not be ideal behaviour... What do you think?

💬 fanquake commented on pull request "guix: Update `python-lief` package to 0.13.1":
(https://github.com/bitcoin/bitcoin/pull/27813#issuecomment-1576423104)
> The usage of LIEF 0.13.1 in the developer environment has been assumed since https://github.com/bitcoin/bitcoin/pull/27507.

The only requirement to have LIEF 0.13.1 installed, is if you want to run the Python linter, which is not a reason to bump pacakges in our release environment. Outside of that, I don't think LIEF is used for anything in developer environments.

💬 hebasto commented on pull request "guix: Update `python-lief` package to 0.13.1":
(https://github.com/bitcoin/bitcoin/pull/27813#issuecomment-1576453032)
> ... which is not a reason to bump packages in our release environment.

"and drops the patch that was upstreamed."

💬 fanquake commented on pull request "test, init: perturb file to ensure failure instead of only deleting them":
(https://github.com/bitcoin/bitcoin/pull/26653#discussion_r1217816163)
Note that it looks like this didn't work, and is being fixed in #27823.

🤔 vasild reviewed a pull request: "net: support unix domain sockets for -proxy and -onion"
(https://github.com/bitcoin/bitcoin/pull/27375#pullrequestreview-1461645134)
Approach ACK 1a2095baf12b8f8995bed513a23d5faf16917fba

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1217688663)
```suggestion
return AF_UNSPEC;
```

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1217689114)
Unnecessary empty line.

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1217685437)
TCP is a specific protocol ([at layer 4 in the OSI model](https://en.wikipedia.org/wiki/OSI_model)) while this refers to layer 3. Maybe rename it to something like `GetProtocolFamily()`, `GetAddressFamily()`, `GetFamily()` or `GetSAFamily()`.

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1217700884)
`sa_family_t` is cheap to copy. No need to pass it as a const reference:

```cpp
std::unique_ptr<Sock> CreateSockOS(sa_family_t address_family);
```

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1217692250)
`CService::GetSockAddr()` treats CJDNS as IPv6. So:
```suggestion
case NET_IPV6:
case NET_CJDNS:
return AF_INET6;
```

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1217480535)
The comment gives the impression that `sa_family_t` is defined on Windows (as `u_short`), but it is not defined at all (otherwise this `typedef u_short sa_family_t;` would not compile because it redefines it, right?). Maybe reword the comment to something like:

```cpp
// Windows does not have `sa_family_t` - it defines `sockaddr::sa_family` as `u_short`.
// Thus define `sa_family_t` on Windows too so that the rest of the code can use `sa_family_t`.
```

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1217719465)
All tuning that happens to the socket below in this function seems relevant to UNIX sockets too, except `TCP_NODELAY`.

```diff
--- i/src/netbase.cpp
+++ w/src/netbase.cpp
@@ -484,23 +484,21 @@ bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* a

std::unique_ptr<Sock> CreateSockOS(const sa_family_t& address_family)
{
// Not IPv4, IPv6 or UNIX
if (address_family == AF_UNSPEC) return nullptr;

+ int protocol{IPPROTO_TCP};
#if HAVE_SOCKADD
...

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1217741453)
Pass `path` by const reference, strings are expensive to copy.

```suggestion
explicit Proxy(const std::string& path, bool _randomize_credentials=false): unix_socket_path(path), is_unix_socket(true), randomize_credentials(_randomize_credentials) {}
```

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1217743952)
Members should start with `m_`. Maybe also rename the existent `randomize_credentials` to `m_randomize_credentials` (in a separate commit).

🚀 fanquake merged a pull request: "wallet: Add tracing for sqlite statements"
(https://github.com/bitcoin/bitcoin/pull/27801)