💬 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

🤔 theuni reviewed a pull request: "crypto, refactor: add method for applying the taptweak"
(https://github.com/bitcoin/bitcoin/pull/30051#pullrequestreview-2050167463)
It's weird to see this hooked up but unused. It could also use some simple test vectors.

Mind killing both birds by adding some tests, at least 1 for each `merkle_root`?

💬 theuni commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1596806833)
This could use a comment.

💬 sr-gi commented on pull request "net: Favor peers from addrman over fetching seednodes":
(https://github.com/bitcoin/bitcoin/pull/29605#discussion_r1596813747)
Yeah, the mock time seems to reduce the runtime by 5-10 secs. I'm guessing with 10 secs of wait it could be the case that the connection is still being tried, so the timeout hasn't been considered. I'll add 5 more.

💬 ismaelsadeeq commented on pull request "Fee Estimation: Ignore all transactions with an in-block child":
(https://github.com/bitcoin/bitcoin/pull/30079#issuecomment-2104694897)
> Do we have charts anywhere tracking % of transactions that are in a cluster size of 1?

I will analyze the percentage of cluster size 1 transactions mined in previous blocks.

💬 theuni commented on pull request "crypto, refactor: add method for applying the taptweak":
(https://github.com/bitcoin/bitcoin/pull/30051#discussion_r1596815685)
That looks much better, thanks. Exposing the vector-like interface is a shame, but I don't see any way around it for `secp256k1_silentpayments_sender_create_outputs`.

💬 theuni commented on pull request "depends: set AR & RANLIB for CMake":
(https://github.com/bitcoin/bitcoin/pull/30078#discussion_r1596841762)
Hmm, looking at the docs and playing around, `CMAKE_AR` seems more correct to me too. It seems like it should work correctly, what was the problem?

There's also `CMAKE_C_COMPILER_AR` which may be more correct?

💬 theuni commented on pull request "depends: set AR & RANLIB for CMake":
(https://github.com/bitcoin/bitcoin/pull/30078#discussion_r1596844125)
There's also no CMAKE_NM, so that isn't going to work if we want to pass that through as well.

I see `CMAKE_NM` in my `CMakeCache.txt`, maybe it's just not documented for some reason?

💬 Sjors commented on pull request "Silent payment index (for light wallets and consistency check)":
(https://github.com/bitcoin/bitcoin/pull/28241#issuecomment-2104736197)
Comparison at height 831,370:

* without cut through: GB @
* with cut through:

Next step is to (occasionally) prune the cut-through index.

I suspect the easiest way to is just to build a new index in the background, shut both the new and old indexes down, delete the old index and move the new one in place, and then turn it on again.

Another approach would be to override its contents block by block. If LevelDB handles resulting fragmentation in a sane way, this approach seems better
...