💬 fanquake commented on issue "index: ThreadSanitizer: data race on vptr ":
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1507100965)
> Idk. Locally this fails for me when building bdb:

Yea. You'll have to use `NO_BDB=1`, or fix BDB.

💬 fanquake commented on pull request "ci: use Debian Bookworm and Valgrind 3.19 in Valgrind jobs":
(https://github.com/bitcoin/bitcoin/pull/27444#discussion_r1165654729)
> I am not a fan of starting to build our own package manager,

Fair enough. I agree that having to maintain too many things oursevles is not idea, but I think I'm starting to move the other way, where, I'd be happy to maintain the 3 lines of bash (in the CI system) required to compile and use a newer version of Valgrind, and keep compatibility with both of the compilers we use to build releases, as opposed to dropping the ability to test under both, for the sake of conviniently being able to
...

👍 pinheadmz approved a pull request: "Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs"
(https://github.com/bitcoin/bitcoin/pull/27231#pullrequestreview-1383617680)
ACK 5991eddb98ce59b883ae695752d4e90f32d43960
<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 5991eddb98ce59b883ae695752d4e90f32d43960
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmQ4FuQACgkQ5+KYS2KJ
yTqVKQ//TIOVSamur/OBGsHB1lgx9C3wyqQFSg+RzImVrDvquEhTDdwfjWI56DzL
sYWxaSDjHLP+4jYK7x3O4QoZJW1iJDlz56PhakxtDSzWMIr0WIfHGpTV6oH1p4Uh
Kaz57R6Y71G4ADMqeR48MqLK/B7vMuEKhzUUmGk14O/8bPUHk7HJE70b5F3zOTb9
EBu/NtFd
...

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