Bitcoin Core Github
44 subscribers
121K links
Download Telegram
💬 MarcoFalke commented on pull request "test: remove `GetRNGState` lsan suppression":
(https://github.com/bitcoin/bitcoin/pull/27362#issuecomment-1491412437)
lgtm ACK 71b3e9b0ade9680f6847e93785225c5927929336
💬 MarcoFalke commented on pull request "ci: set docker run --ulimit to workaround Valgrind assertion":
(https://github.com/bitcoin/bitcoin/pull/27364#issuecomment-1491421754)
Have you tried podman? The `podman-docker` on Ubuntu/Debian might help (haven't tried it), but not sure if it exists on your Fedora.
💬 willcl-ark commented on issue "Adding interprocess onion access instead local ip":
(https://github.com/bitcoin/bitcoin/issues/27252#issuecomment-1491446913)
Oh nice work @pinheadmz! I will help test #27375 too.

I have a small bash script which uses `socat` to pipe between an inet and unix socket, but native support is much better:

```bash
#!/usr/bin/env bash

# default tor sock on debian
socat -v tcp-l:9050,reuseaddr,fork unix:/run/tor/socks 2>&1/dev/null & disown
socat_pid=$!

while true; do
sleep 5 # check every 5 seconds
if ! pgrep -x "bitcoind" >/dev/null; then
# bitcoind not found, terminate socat and exit loop

...
💬 MarcoFalke commented on issue "test: `wallet_importdescriptors.py --descriptors` failure":
(https://github.com/bitcoin/bitcoin/issues/27282#issuecomment-1491468284)
Ok, this still happens: https://cirrus-ci.com/task/5541464030052352?logs=ci#L2622

However, it looks like there is another unrelated bug in the rpc proxy to dispatch the same RPC twice? There really shouldn't be any reason to blindly run the same RPC twice without the user (test developer) asking for it.

I presume running the RPC twice will also discard the earlier result, so still no luck debugging this issue.
⚠️ MarcoFalke reopened an issue: "test: `wallet_importdescriptors.py --descriptors` failure"
(https://github.com/bitcoin/bitcoin/issues/27282)
Seen on a aarch64 Alpine box. Master @ https://github.com/bitcoin/bitcoin/commit/40e1c4d4024b8ad35f2511b2e10bf80c5531dfde. Binaries compiled with Clang 15.0.7. Valgrind `valgrind-3.21.0.GIT`.

We saw some issues with this test recently (#27229), but this looks like a different issue:

```bash
261/262 - wallet_importdescriptors.py --descriptors failed, Duration: 411 s

stdout:
2023-03-20T10:39:03.422000Z TestFramework (INFO): PRNG seed is: 4441145092714460381
2023-03-20T10:39:03.423000Z
...
💬 MarcoFalke commented on pull request "ci: Remove second user account":
(https://github.com/bitcoin/bitcoin/pull/27376#issuecomment-1491475352)
red CI can be ignored
📝 MarcoFalke opened a pull request: "test: Remove python3.5 workaround"
(https://github.com/bitcoin/bitcoin/pull/27378)
Remove workaround for a bug that is long fixed in a EOL python version, that isn't used by us.

If the workaround is still needed, it should at least log the exception before silently discarding it, so that debugging is possible/easier.
💬 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).
💬 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
...
💬 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?
💬 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
...
💬 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`...
💬 fanquake commented on pull request "test: Remove python3.5 workaround":
(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.
💬 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.
📝 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
^
...
💬 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)
💬 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.
👍 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.