💬 TheCharlatan commented on pull request "depends: set AR & RANLIB for CMake":
(https://github.com/bitcoin/bitcoin/pull/30078#issuecomment-2104546249)
Guix build (aarch64):
```
cf21ea004d1f8d6d7b53d4ef725083229b18469948439f79872a2b019e8bc966 guix-build-7efd6bf03755/output/aarch64-linux-gnu/SHA256SUMS.part
1fa79040aba310b34075b11eefbdb570dd1f053d25ea432faf9bca596603cabb guix-build-7efd6bf03755/output/aarch64-linux-gnu/bitcoin-7efd6bf03755-aarch64-linux-gnu-debug.tar.gz
a97ee58be3ea463c565ec8098ed3f98a04f70b7a897c2b1961df083f5142e07b guix-build-7efd6bf03755/output/aarch64-linux-gnu/bitcoin-7efd6bf03755-aarch64-linux-gnu.tar.gz
1f4f554289
...

💬 vasild commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1596668853)
nit: replace "include the trace.h" with either "include trace.h" or "include the trace.h header"

🤔 vasild reviewed a pull request: "tracing: Only prepare tracepoint arguments when actually tracing"
(https://github.com/bitcoin/bitcoin/pull/26593#pullrequestreview-2049935876)
Approach ACK 38e24dfc5f1c54723018bf9d973b7d9cf9a2b824

Mostly nits that would be nice to address + one blocker about the `bitcoind_pid` vs `pid` mismatch.

💬 vasild commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1596671187)
nit:
```suggestion
// Setting SDT_USE_VARIADIC lets systemtap (sys/sdt.h) know that we want to use
```

💬 vasild commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1596685441)
Indentation in Bitcoin Core is 4 spaces. This file uses 2 spaces (also some of the examples in `doc/tracing.md`).

💬 vasild commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1596683957)
nit:
```suggestion
if (TRACEPOINT_ACTIVE(example, gated_expensive_argument)) {
```

💬 vasild commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1596676716)
3 nits:

```suggestion
If needed, an extra `if (TRACEPOINT_ACTIVE(context, event)) {...}` check can be
```

💬 vasild commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1596711392)
I know it would be weird usage if somebody does something like `TRACEPOINT_ACTIVE(...) + 1` but in general it is better to define macros in such a way to avoid unexpected outcomes. In that case it would translate to `context##_##event##_semaphore > (0 + 1)` whereas the expectation would be `(context##_##event##_semaphore > 0) + 1`.

```suggestion
#define TRACEPOINT_ACTIVE(context, event) (context##_##event##_semaphore > 0)
```

💬 vasild commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1596714759)
whitespace:
```suggestion
if (TRACEPOINT_ACTIVE(context, event)) { \
STAP_PROBEV(context, event __VA_OPT__(, ) __VA_ARGS__); \
} \
```

💬 vasild commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#discussion_r1596724174)
The argument is called `bitcoind_pid`, but the `print` uses `{pid}`. The neighboring `py` files use just `pid`:

```suggestion
def main(pid):
print(f"Hooking into bitcoind with pid {pid}")
bitcoind_with_usdts = USDT(pid=int(pid))
```

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2104594682)
Added some scaffolding code for an index with cut-through: `-bip352ctindex`

I'm not sure yet how to actually implement this in a way that doesn't cause too much I/O. The benefit of cut-through is that it's smaller on disk and results in less data to download for light clients. But I suspect it will involve a _lot_ more disk I/O to generate and maintain it. Having both options make it easier to compare these trade-offs.

💬 hebasto commented on pull request "test, subprocess: Improve coverage report correctness":
(https://github.com/bitcoin/bitcoin/pull/30075#discussion_r1596742711)
> Doing the reset unconditionally prior to the exec function this way seems like a reasonable alternative approach to that to me.

It looks better to me because it counts the failed `execvp()`, which returns.

💬 hebasto commented on pull request "test, subprocess: Improve coverage report correctness":
(https://github.com/bitcoin/bitcoin/pull/30075#discussion_r1596743670)
Well, reset seem unneeded here. I'm going to drop it.

🤔 vasild reviewed a pull request: "doc: removed help text saying that peers may not connect automatically"
(https://github.com/bitcoin/bitcoin/pull/29994#pullrequestreview-2050072561)
ACK 95897ff181c0757e445f0e066a2a590a0a0120d2

Why exactly was this closed?

> I'm not sure this should be removed unless it has been observed to no longer be the case

> According to https://bitnodes.io/dashboard/, ~85% of public nodes are running 23 or higher

Those are (currently) 82.3% of the listening nodes. I think "unlikely to get incoming connections" is not true anymore and this sentence can be dropped.

What about non-listening nodes? Incoming connections come also from them.
...

💬 hebasto commented on pull request "test, subprocess: Improve coverage report correctness":
(https://github.com/bitcoin/bitcoin/pull/30075#issuecomment-2104612898)
Addressed @ajtowns's feedback. Added comments. Updated the PR description.

🤔 hebasto reviewed a pull request: "build: swap cctools otool for llvm-objdump"
(https://github.com/bitcoin/bitcoin/pull/29739#pullrequestreview-2050083898)
My Guix build:
```
67f4db93b4f985590948dc344306042841ab747b40ad73bf97ba62d77eb7bfd4 guix-build-7f5ac4520d15/output/aarch64-linux-gnu/SHA256SUMS.part
62e019f164d3c8cf8a1a649b2caa500dd85b00a319910feaf97658020b8cd1e3 guix-build-7f5ac4520d15/output/aarch64-linux-gnu/bitcoin-7f5ac4520d15-aarch64-linux-gnu-debug.tar.gz
c2227a058cf91c0bc3eb244cb254c3c6fcecddc21ba619bca8839d9d79bb1961 guix-build-7f5ac4520d15/output/aarch64-linux-gnu/bitcoin-7f5ac4520d15-aarch64-linux-gnu.tar.gz
e794a50c8aeebefa6
...

💬 fanquake commented on pull request "build: use `-no_exported_symbols` on macOS":
(https://github.com/bitcoin/bitcoin/pull/29072#issuecomment-2104619042)
Based on #21778, so the flag is usable in release builds.

💬 instagibbs commented on pull request "test: add conflicting topology test case":
(https://github.com/bitcoin/bitcoin/pull/30066#discussion_r1596767307)
It's coverage for `package-not-child-with-unconfirmed-parents`, which only occurs once, semi-incidentally, in `test/functional/p2p_opportunistic_1p1c.py`. If the restriction is taken out, we know there's already coverage and can adapt the test to cover the new expected failure.

💬 alfonsoromanz commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1596777994)
Thanks for your review. Actually this test uses only one node and I used the same notation used in other tests, but I am willing to change it if others agree.

💬 alfonsoromanz commented on pull request "test: Assumeutxo: import snapshot in a node with a divergent chain":
(https://github.com/bitcoin/bitcoin/pull/29996#discussion_r1596778313)
Good point. The goal of this test is to generate a divergent chain starting from `START_HEIGHT` but having less work than the snapshot, which height equals `SNAPSHOT_BASE_HEIGHT`. Maybe I can set `nBlocks=SNAPSHOT_BASE_HEIGHT-START_HEIGHT-1`. This will result in `99` and it will adapt to any future changes to either `START_HEIGHT` or `SNAPSHOT_BASE_HEIGHT`