💬 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`

💬 instagibbs commented on pull request "refactor prep for package rbf":
(https://github.com/bitcoin/bitcoin/pull/30072#discussion_r1596780968)
will fix if touching again

💬 glozow commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2104653494)
cc @josibake @instagibbs @naumenkogs @darosior @murchandamus since you reviewed the last time

💬 instagibbs commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2104660144)
concept ACK

strikes me as the right idea, keep it simple for now, focusing on correctness. Do we have charts anywhere tracking % of transactions that are in a cluster size of 1?

📝 remyers opened a pull request: "wallet: add coin selection parameter `add_excess_to_recipient_position` for changeless txs with excess that would be added to fees "
(https://github.com/bitcoin/bitcoin/pull/30080)
This PR replaces draft [PR 29442](https://github.com/bitcoin/bitcoin/pull/29442) as a simpler alternative that only adds new coin selection parameters, primarily `add_excess_to_recipient_position`.

I am opening this PR as a draft to get feedback and suggestions on the concept and my implementation.

I have also included two additional coin selection parameters to this PR for specifying the target change amount and for disabling different coin selection algorithms. I can break them out into
...

📝 hebasto opened a pull request: "refactor: Remove unused code from `subprocess.h` header"
(https://github.com/bitcoin/bitcoin/pull/30081)
This PR continues https://github.com/bitcoin/bitcoin/pull/29961.

Please note that `Popen::poll()` is not required for https://github.com/bitcoin/bitcoin/pull/29868 anymore.

💬 katesalazar commented on pull request "refactor: Remove unused code from `subprocess.h` header":
(https://github.com/bitcoin/bitcoin/pull/30081#issuecomment-2104687531)
Concept ACK