💬 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

💬 pinheadmz commented on pull request "net: support unix domain sockets for -proxy and -onion":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1165801803)
See this ci failure: https://cirrus-ci.com/task/5039125594636288?logs=ci#L2638-L2640

This is why I used a shorter path, sometimes TMPDIR is > 50 characters! You have a good point about getting max `sun_path` from the system though, maybe it will balance out

```
File "/private/var/folders/v7/fs2b0v3s0lz1n57gj9y4xb5m0000gn/T/cirrus-ci-build/ci/scratch/build/bitcoin-arm64-apple-darwin/test/functional/test_framework/socks5.py", line 131, in __init__
self.s.bind(conf.addr)
OSError: AF_UNIX p
...

💬 mzumsande commented on pull request "refactor: Introduce EvictionManager and use it for the inbound eviction logic":
(https://github.com/bitcoin/bitcoin/pull/25572#discussion_r1165808152)
This removes the check `node->fDisconnect`, which I think could be dangerous:
We set `fDisconnect` in `AttemptToEvictConnection()`, but don't immediately disconnect. The actual cleanup happens at some later point, when `ThreadSocketHandler` calls `DisconnectNodes()`.
So if we have an attacker that makes new connections to us rapidly, there would be a race between `ThreadSocketHandler` and the attacker making the next connection to us. If the attacker wins this race, we could accept two inbound
...

💬 mzumsande commented on pull request "refactor: Introduce EvictionManager and use it for the inbound eviction logic":
(https://github.com/bitcoin/bitcoin/pull/25572#discussion_r1165793761)
Could use `Assume` and continue in order to avoid the `assert`.

💬 mzumsande commented on pull request "refactor: Introduce EvictionManager and use it for the inbound eviction logic":
(https://github.com/bitcoin/bitcoin/pull/25572#discussion_r1165796262)
00230350cd32d5a1d2421b588a7f493afa7c69dd: I think that `prefer_evict` could also be removed from `CNodeOptions`.

💬 1440000bytes commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#issuecomment-1507338618)
Hi @MarcoFalke

More than 5 tests are failing