💬 hebasto commented on pull request "tracepoints: Disables `-Wgnu-zero-variadic-macro-arguments` to compile without warnings":
(https://github.com/bitcoin/bitcoin/pull/27401#issuecomment-1507142439)
> > Maybe add a check to configure.ac whether ignoring of -Wgnu-zero-variadic-macro-arguments is [#26916 (comment)](https://github.com/bitcoin/bitcoin/issues/26916#issuecomment-1400156218) required?
>
> Sounds good, but I have no idea how to do this with `configure.ac`...

Something like in this [branch](https://github.com/hebasto/bitcoin/commits/230413-variadic):
```diff
--- a/configure.ac
+++ b/configure.ac
@@ -470,6 +470,19 @@ if test "$CXXFLAGS_overridden" = "no"; then
if test "
...

💬 pinheadmz commented on pull request "Add wallet method to detect if a key is "active"":
(https://github.com/bitcoin/bitcoin/pull/27216#issuecomment-1507152541)
rebase to: 58fba35e25c2b9dcdd5b3dd0aa8f412801a98146
- added `nodiscard`
- cleaned up help message in `getaddressinfo`
- added release note file

🤔 vasild reviewed a pull request: "net: support unix domain sockets for -proxy and -onion"
(https://github.com/bitcoin/bitcoin/pull/27375#pullrequestreview-1382786000)
Concept ACK, this would be nice to have.

I do not like that the current approach expands `enum Network` and `CNetAddr` with the UNIX type, making it first-class-citizen network that has to be handled all over the networking code, whereas it is only needed to connect to the proxy. It is not like this PR adds support for connecting to other Bitcoin nodes over UNIX sockets (using the P2P protocol). This creates meaningless situations like having a `CService` on a UNIX socket (it is meaningless b
...

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1165198178)
This looks wrong as `paddrun` is a pointer (`sizeof(paddrun)` will be always 8 bytes on 64 bit computers). According to https://www.man7.org/linux/man-pages/man7/unix.7.html it should be:

```cpp
offsetof(struct sockaddr_un, sun_path)+strlen(addr.sun_path)+1
```

FreeBSD has this, defined in `sys/un.h`:

```cpp
#define SUN_LEN(su) \
(sizeof(*(su)) - sizeof((su)->sun_path) + strlen((su)->sun_path))
```

and Linux has:

```cpp
/* Evaluate to actual length of the `sockaddr_u
...

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1165186109)
This is dangerous. Better check the size before copying (untested):

```suggestion
const std::string path{fs::PathToString(m_path)};
if (sizeof(paddrun->sun_path) < path.length() + 1) {
return false;
}
memcpy(paddrun->sun_path, path.c_str(), path.length() + 1);
```

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1165266423)
It is a bit odd to support `unix:/path/to/socket` in `Lookup()` because there is nothing to lookup in that and it is not like the other inputs supported by `Lookup()` begin with the socket type, e.g. `ipv4://1.2.3.4`.

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1165249878)
Better check the actual `sun_path` on the system, and also `sun_path` is supposed to contain the terminating `'\0'`:

```suggestion
if (str.size() + 1 > sizeof(sockaddr_un::sun_path)) {
return false;
}
```

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1165255786)
style: from `doc/developer-notes.md`:

- If an `if` only has a single-statement `then`-clause, it can appear
on the same line as the `if`, without braces. In every other case,
braces are required, and the `then` and `else` clauses must appear
correctly indented on a new line.

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1165311055)
It is better to stay inside the individual test directory. This allows running multiple tests in parallel. With the above code multiple tests will collide on the common `/tmp/sockpath`.

`/tmp/bitcoin_func_test_99opr6sq/socks5_proxy` is just 44 chars, should be below the limit on any platform, even if TMPDIR expands to something longer than `/tmp`.

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

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1165281204)
It is odd to have `CreateSockTCP()` create a non-TCP socket.

Also, the code below sets `TCP_NODELAY` on the socket which does not apply to UNIX sockets.

💬 vasild commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1165330624)
The include is introduced in the first commit and surrounded by `#if HAVE_SOCKADDR_UN` in the second commit. This means that the first commit is not self-contained (would not compile on some platforms). Better introduce the configure check first.

💬 pinheadmz commented on issue "RPC wont bind without an IP address on a non-localhost interface":
(https://github.com/bitcoin/bitcoin/issues/13155#issuecomment-1507215557)
Thanks Matt! Closing for now, lets reopen if it comes up again

✅ pinheadmz closed an issue: "RPC wont bind without an IP address on a non-localhost interface"
(https://github.com/bitcoin/bitcoin/issues/13155)

💬 MarcoFalke commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1165741318)
unrelated: Could make sense to remove unused includes, according to iwyu

💬 pinheadmz commented on issue "Add test vectors in BIP143 into tx_valid.json / sighash.json":
(https://github.com/bitcoin/bitcoin/issues/14308#issuecomment-1507218113)
This isn't a bad idea, and we could include the witnessV0 test vectors in the JSON file, but since `sighash_tests.cpp` only executes `SignatureHash()` and not `SignatureHashSchnorr()`, I don't think those test/data files should be considered complete anymore.

To test [taproot validation on bcoin](https://github.com/bcoin-org/bcoin/pull/1063) for example I created a [fork of bitcoin](https://github.com/pinheadmz/bitcoin/tree/taproottest-0.21.1) that generated a taproot test vectors json file
...

💬 MarcoFalke commented on issue "Log X-Forwarded-For for rpc ":
(https://github.com/bitcoin/bitcoin/issues/9397#issuecomment-1507222169)
Closing for now due to lack of progress and direction. Pull requests with improvements are welcome, and it is possible to re-open this issue or create a new one if this feature is requested again.

✅ MarcoFalke closed an issue: "Log X-Forwarded-For for rpc "
(https://github.com/bitcoin/bitcoin/issues/9397)

💬 MarcoFalke commented on issue "index: ThreadSanitizer: data race on vptr ":
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1507231532)
Ok, it is passing for me with NO_BDB. Could be a hardware issue on your side.

```
# lscpu
Architecture: aarch64
CPU op-mode(s): 32-bit, 64-bit
Byte Order: Little Endian
CPU(s): 4
On-line CPU(s) list: 0-3
Vendor ID: ARM
BIOS Vendor ID: QEMU
Model name: Neoverse-N1
BIOS Model name: NotSpecified CPU @ 2.0GHz
BIOS CPU family: 1
Model: 1
Thread(s) per core:
...

💬 MarcoFalke commented on issue "index: ThreadSanitizer: data race on vptr ":
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1507234555)
I am using podman, but that shouldn't matter