💬 willcl-ark commented on pull request "net: support unix domain sockets for -proxy":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1154121825)
I think I'd go for `unix` here (but only because it's shorter).
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1154121825)
I think I'd go for `unix` here (but only because it's shorter).
💬 willcl-ark commented on pull request "net: support unix domain sockets for -proxy":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1154147617)
This needs to be shorter (< 92 chars?). From `man 7 unix`:
```
Pathname sockets
When binding a socket to a pathname, a few rules should be observed for maximum portability and ease of coding:
* The pathname in sun_path should be null-terminated.
* The length of the pathname, including the terminating null byte, should not exceed the size of sun_path.
* The addrlen argument that describes the enclosing sockaddr_un structure should have a value of at
...
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1154147617)
This needs to be shorter (< 92 chars?). From `man 7 unix`:
```
Pathname sockets
When binding a socket to a pathname, a few rules should be observed for maximum portability and ease of coding:
* The pathname in sun_path should be null-terminated.
* The length of the pathname, including the terminating null byte, should not exceed the size of sun_path.
* The addrlen argument that describes the enclosing sockaddr_un structure should have a value of at
...
💬 willcl-ark commented on pull request "net: support unix domain sockets for -proxy":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1154156655)
I wonder if this should be a `Warning` or at least an `Info` level message?
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1154156655)
I wonder if this should be a `Warning` or at least an `Info` level message?
💬 willcl-ark commented on pull request "net: support unix domain sockets for -proxy":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1154205583)
Im curious why you didn't enable it for `-onion` here, as it seems a compartiviely small addtion:
```diff
diff --git a/src/init.cpp b/src/init.cpp
index c0a320136..53b6b2a4b 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1283,8 +1283,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
std::string host_out;
uint16_t port_out{0};
if (!SplitHostPort(socket_addr, port_out, host_out)) {
- // Allow un
...
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1154205583)
Im curious why you didn't enable it for `-onion` here, as it seems a compartiviely small addtion:
```diff
diff --git a/src/init.cpp b/src/init.cpp
index c0a320136..53b6b2a4b 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1283,8 +1283,8 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
std::string host_out;
uint16_t port_out{0};
if (!SplitHostPort(socket_addr, port_out, host_out)) {
- // Allow un
...
💬 willcl-ark commented on pull request "net: support unix domain sockets for -proxy":
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1154148265)
On the test this is trying to use a path `/private/var/folders/v7/fs2b0v3s0lz1n57gj9y4xb5m0000gn/T/cirrus-ci-build/ci/scratch/test_runner/test_runner_₿_🏃_20230331_010828/feature_proxy_156`...
(https://github.com/bitcoin/bitcoin/pull/27375#discussion_r1154148265)
On the test this is trying to use a path `/private/var/folders/v7/fs2b0v3s0lz1n57gj9y4xb5m0000gn/T/cirrus-ci-build/ci/scratch/test_runner/test_runner_₿_🏃_20230331_010828/feature_proxy_156`...
💬 fanquake commented on pull request "test: Remove python3.5 workaround":
(https://github.com/bitcoin/bitcoin/pull/27378#issuecomment-1491599097)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/27378#issuecomment-1491599097)
Concept ACK
💬 MarcoFalke commented on pull request "ci: use LLVM/clang-16 in native_asan job":
(https://github.com/bitcoin/bitcoin/pull/27360#discussion_r1154231425)
I guess google will push the image on April 25th or so. It should be fine for you to make all changes now and then click re-run in April, or close and re-open.
(https://github.com/bitcoin/bitcoin/pull/27360#discussion_r1154231425)
I guess google will push the image on April 25th or so. It should be fine for you to make all changes now and then click re-run in April, or close and re-open.
💬 fanquake commented on pull request "ci: use LLVM/clang-16 in native_asan job":
(https://github.com/bitcoin/bitcoin/pull/27360#discussion_r1154237235)
I wonder if we should split the tracepoints stuff into it's own CI, so it's not a blocker for other unrelated tests.
(https://github.com/bitcoin/bitcoin/pull/27360#discussion_r1154237235)
I wonder if we should split the tracepoints stuff into it's own CI, so it's not a blocker for other unrelated tests.
📝 fanquake converted_to_draft a pull request: "ci: use LLVM/clang-16 in native_asan job"
(https://github.com/bitcoin/bitcoin/pull/27360)
Similar to #27298. Working for me on `x86_64` and solves the issue I currently see with TSAN on `aarch64` with master (68828288e5d35c17848ab3da8cd231d1b69651c0):
```bash
crc32c/src/crc32c_arm64.cc:101:26: runtime error: load of misaligned address 0xffff84400406 for type 'uint64_t' (aka 'unsigned long'), which requires 8 byte alignment
0xffff84400406: note: pointer points here
b9 c5 22 00 01 01 1a 6c 65 76 65 6c 64 62 2e 42 79 74 65 77 69 73 65 43 6f 6d 70 61 72 61 74 6f
^
...
(https://github.com/bitcoin/bitcoin/pull/27360)
Similar to #27298. Working for me on `x86_64` and solves the issue I currently see with TSAN on `aarch64` with master (68828288e5d35c17848ab3da8cd231d1b69651c0):
```bash
crc32c/src/crc32c_arm64.cc:101:26: runtime error: load of misaligned address 0xffff84400406 for type 'uint64_t' (aka 'unsigned long'), which requires 8 byte alignment
0xffff84400406: note: pointer points here
b9 c5 22 00 01 01 1a 6c 65 76 65 6c 64 62 2e 42 79 74 65 77 69 73 65 43 6f 6d 70 61 72 61 74 6f
^
...
💬 MarcoFalke commented on pull request "ci: use LLVM/clang-16 in native_asan job":
(https://github.com/bitcoin/bitcoin/pull/27360#discussion_r1154242414)
Not sure if this is worth it. I am not aware of anyone running the CI on aarch64 other than you. So it should be fine for you to locally apply this patch in the meantime (for less than a month)
(https://github.com/bitcoin/bitcoin/pull/27360#discussion_r1154242414)
Not sure if this is worth it. I am not aware of anyone running the CI on aarch64 other than you. So it should be fine for you to locally apply this patch in the meantime (for less than a month)
💬 fanquake commented on pull request "build: produce a .zip for macOS distribution":
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1491619772)
> we can't assume the user can open .md files by default.
I'm pretty sure we can assume that on macOS.
(https://github.com/bitcoin/bitcoin/pull/27099#issuecomment-1491619772)
> we can't assume the user can open .md files by default.
I'm pretty sure we can assume that on macOS.
👍 hebasto approved a pull request: "test: remove `GetRNGState` lsan suppression"
(https://github.com/bitcoin/bitcoin/pull/27362)
ACK 71b3e9b0ade9680f6847e93785225c5927929336, tested on Ubuntu 22.04 x86_64.
(https://github.com/bitcoin/bitcoin/pull/27362)
ACK 71b3e9b0ade9680f6847e93785225c5927929336, tested on Ubuntu 22.04 x86_64.
🚀 fanquake merged a pull request: "test: remove `GetRNGState` lsan suppression"
(https://github.com/bitcoin/bitcoin/pull/27362)
(https://github.com/bitcoin/bitcoin/pull/27362)
💬 stratospher commented on pull request "p2p: skip netgroup diversity of new connections for tor/i2p/cjdns":
(https://github.com/bitcoin/bitcoin/pull/27374#issuecomment-1491700077)
> It doesn't seem to me like it matters much which way we do this, since I believe netgroups for different networks cannot collide, so this looks fine to me.
> I think the same logic needs to be applied to the anchors in
i didn't add tor/i2p/cjdns peer addresses in `setConnected` to skip the netgroup diversity check when making both outbound and anchor connections. (now understood from the above comments why it won't work)
> As far as I can see, this logic should equally be applied to
...
(https://github.com/bitcoin/bitcoin/pull/27374#issuecomment-1491700077)
> It doesn't seem to me like it matters much which way we do this, since I believe netgroups for different networks cannot collide, so this looks fine to me.
> I think the same logic needs to be applied to the anchors in
i didn't add tor/i2p/cjdns peer addresses in `setConnected` to skip the netgroup diversity check when making both outbound and anchor connections. (now understood from the above comments why it won't work)
> As far as I can see, this logic should equally be applied to
...
💬 MarcoFalke commented on pull request "ci: use LLVM/clang-16 in native_asan job":
(https://github.com/bitcoin/bitcoin/pull/27360#issuecomment-1491715912)
test comment (ignore)
(https://github.com/bitcoin/bitcoin/pull/27360#issuecomment-1491715912)
test comment (ignore)
💬 fanquake commented on pull request "test: Remove python3.5 workaround":
(https://github.com/bitcoin/bitcoin/pull/27378#discussion_r1154334637)
Can drop `-rpcservertimeout=900` from feature_dbcrash given we are setting the default here?
(https://github.com/bitcoin/bitcoin/pull/27378#discussion_r1154334637)
Can drop `-rpcservertimeout=900` from feature_dbcrash given we are setting the default here?
💬 dergoegge commented on pull request "refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer":
(https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1154339822)
Actually this doesn't work because `mapRelay` is guraded by `cs_main`😭😭😭
(https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1154339822)
Actually this doesn't work because `mapRelay` is guraded by `cs_main`😭😭😭
💬 dergoegge commented on pull request "refactor: Move CNodeState members guarded by g_msgproc_mutex to Peer":
(https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1154345076)
But... `mapRelay` is actually only accessed from the msg proc thread, so we can simply mark it as guarded by `g_msgproc_mutex` and still do this!
(https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1154345076)
But... `mapRelay` is actually only accessed from the msg proc thread, so we can simply mark it as guarded by `g_msgproc_mutex` and still do this!
📝 dergoegge opened a pull request: "#26140 follow-ups"
(https://github.com/bitcoin/bitcoin/pull/27379)
Addresses left over feedback from #26140.
* https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1153498543
* https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1153499627
(https://github.com/bitcoin/bitcoin/pull/27379)
Addresses left over feedback from #26140.
* https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1153498543
* https://github.com/bitcoin/bitcoin/pull/26140#discussion_r1153499627
💬 fanquake commented on pull request "contrib: allow multi-sig binary verification v2":
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1154356697)
[I mentioned in the original PR, that I think this can just be dropped](https://github.com/bitcoin/bitcoin/pull/23020#discussion_r1117049036). We know we are downloading bitcoin-core, so I'm not sure why we need to support anything other than just passing in a version number.
(https://github.com/bitcoin/bitcoin/pull/27358#discussion_r1154356697)
[I mentioned in the original PR, that I think this can just be dropped](https://github.com/bitcoin/bitcoin/pull/23020#discussion_r1117049036). We know we are downloading bitcoin-core, so I'm not sure why we need to support anything other than just passing in a version number.