💬 fanquake commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#issuecomment-2060962844)
> There are no conflicts with https://github.com/bitcoin/bitcoin/pull/29865.

Looks like there are now?

💬 laanwj commented on pull request "ZMQ: Support UNIX domain sockets":
(https://github.com/bitcoin/bitcoin/pull/27679#issuecomment-2061002157)
Looks good to me now
Code review ACK 21d0e6c7b7c7af7f6e54a45829b4fbfba6923b86

💬 0xB10C commented on pull request "tracing: cast block_connected duration to µs":
(https://github.com/bitcoin/bitcoin/pull/29877#issuecomment-2061008679)
> How is is possible that this even compiled? I presume tracing just takes any type and value and passes it along as raw bytes, so `std::chrono` being type-safe does not help here?
>
> Edit: If yes, one could consider if adding a static_assert to ban std::chrono (and possibly other types) from tracing?

In this case, casting with e.g. `int64_t{time_5 - time_start}` would have failed to compile too. Maybe being explicit with the type we expect here, as we do with e.g. the utxocache tracepoin
...

🤔 tdb3 reviewed a pull request: "ZMQ: Support UNIX domain sockets"
(https://github.com/bitcoin/bitcoin/pull/27679#pullrequestreview-2005766388)
cr re ACK. Changes lgtm. Will follow up with some testing within the next few days as time allows.

💬 maflcko commented on pull request "tracing: cast block_connected duration to µs":
(https://github.com/bitcoin/bitcoin/pull/29877#issuecomment-2061024985)
lgtm ACK ca4eaa5777888292bbdef3ae8d45af6a407df5df

💬 maflcko commented on pull request "tracing: cast block_connected duration to µs":
(https://github.com/bitcoin/bitcoin/pull/29877#issuecomment-2061028339)
> In this case, casting with e.g. `int64_t{time_5 - time_start}` would have failed to compile too. Maybe being explicit with the type we expect here, as we do with e.g. the utxocache tracepoints, is worth considering.

Another benefit would be that the code is self-documenting. If a comment for each argument is appended in-line, one could even generate the doc/tracing file from the source code, with the guarantee that it is accurate.

But that can be done in a follow-up.

🚀 fanquake merged a pull request: "guix: replace GCC unaligned VMOV patch with binutils patch"
(https://github.com/bitcoin/bitcoin/pull/29846)

💬 Christewart commented on pull request "tests: fix `OP_1NEGATE` handling in `CScriptOp`":
(https://github.com/bitcoin/bitcoin/pull/29589#issuecomment-2061048370)
@dgpv @petertodd what do you think is the path forward for this?

👍 alfonsoromanz approved a pull request: "doc: Add example of mixing private and public keys in descriptors"
(https://github.com/bitcoin/bitcoin/pull/28373#pullrequestreview-2005838311)
Re ACK 24b67fa9f602cdeac0e9736256f77d048f616c48

👍 alfonsoromanz approved a pull request: "test: Fix intermittent issue in p2p_handshake.py"
(https://github.com/bitcoin/bitcoin/pull/29898#pullrequestreview-2005867060)
Tested ACK b199b323840cf4978bda3437eee36c3dd228702a

I was able to reproduce this issue by using the sleep statement.

Test execution before this fix (failed):

<img width="743" alt="Screenshot 2024-04-17 at 08 58 30" src="https://github.com/bitcoin/bitcoin/assets/19962151/c0b2cf2a-b8f3-4ce0-8fc5-39788b563774">

Test execution after this fix (passed):

<img width="1061" alt="Screenshot 2024-04-17 at 09 05 43" src="https://github.com/bitcoin/bitcoin/assets/19962151/6e389d28-ddc4-4a9b-9c
...

💬 willcl-ark commented on pull request "build: special instruction check script":
(https://github.com/bitcoin/bitcoin/pull/26693#issuecomment-2061135454)
> BTW: along with some capstone magic like https://github.com/bitcoin/bitcoin/pull/29874, and a bare-bones DWARF (or linker map) parser to get function ranges, it would be possible to make a check like this that runs in the final binary, checking that special instructions only appear in allow-listed functions.

This sounds very cool (and more practically-useful than the check here perhaps).

> i think this script makes sense, it's good to check that objects with special instructions don't co
...

📝 glozow converted_to_draft a pull request: "26.x: backport #29853 ("sign: don't assume we are parsing a sane Miniscript")"
(https://github.com/bitcoin/bitcoin/pull/29854)
Backports #29853.

💬 glozow commented on pull request "26.x: backport #29853 ("sign: don't assume we are parsing a sane Miniscript")":
(https://github.com/bitcoin/bitcoin/pull/29854#issuecomment-2061140568)
Thanks. Converted to draft until the PR for master is merged.

💬 fanquake commented on pull request "build: special instruction check script":
(https://github.com/bitcoin/bitcoin/pull/26693#issuecomment-2061152077)
> I can add it to the CI in this change if that's preferred?

I'm not sure. We ended up removing security/symbol checks from the CI, because most of them wont pass there, and even if they do pass in the CI, that doesn't really matter unless they pass in Guix too. I'm going to think about this a bit more, now that there is some renewed interest, and ideas for new checks (£29874).

📝 glozow opened a pull request: "[26.x] archive 26.1 release notes + backports"
(https://github.com/bitcoin/bitcoin/pull/29899)
Archives 26.1 release notes and backports:
- #29691
- #29869

📝 maflcko opened a pull request: "ci: Run everything in Nulgrind or Valgrind"
(https://github.com/bitcoin/bitcoin/pull/29900)
Currently segmentation faults or core dumps do not result in a stacktrace. This is problematic when debugging issues, such as https://github.com/bitcoin/bitcoin/issues/29889#issuecomment-2059515027

Fix it by running everything in Nulgrind, which has less overhead than Valgrind, unless Valgrind is already selected.

💬 maflcko commented on pull request "ci: Run everything in Nulgrind or Valgrind":
(https://github.com/bitcoin/bitcoin/pull/29900#issuecomment-2061177361)
I looked into many alternatives, such as `gdb --batch --ex ...`, or `catchsegv`, but none of them worked in the CI setting.

I think the only reasonable alternative would be to implement a signal handler in the C++ code.

Another alternative would be to wait for C++ (or compilers) to port `RUST_BACKTRACE`, but that seems unlikely, or at least far out in the feature.

💬 maflcko commented on issue "Intermittent issue in test/ipc_tests.cpp Fatal glibc error: pthread_mutex_lock.c:450 (__pthread_mutex_lock_full): assertion failed: e != ESRCH || !robust":
(https://github.com/bitcoin/bitcoin/issues/29889#issuecomment-2061178176)
> stack trace

#29900

👍 fanquake approved a pull request: "build: Fix false positive `CHECK_ATOMIC` test"
(https://github.com/bitcoin/bitcoin/pull/29859#pullrequestreview-2005982397)
ACK dd3e0fa12534c9e782dc9c24d2e30b70a0d73176

🚀 fanquake merged a pull request: "build: Fix false positive `CHECK_ATOMIC` test"
(https://github.com/bitcoin/bitcoin/pull/29859)