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

💬 theuni commented on pull request "depends: set AR & RANLIB for CMake":
(https://github.com/bitcoin/bitcoin/pull/30078#discussion_r1596871427)
> 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?

It seems `CMAKE_AR` is interpreted as relative to the build dir for some reason. What works for me is:
```
-DCMAKE_AR=`which ar`
```

💬 theuni commented on pull request "depends: set AR & RANLIB for CMake":
(https://github.com/bitcoin/bitcoin/pull/30078#discussion_r1596873482)
Bug here: https://gitlab.kitware.com/cmake/cmake/-/issues/18087

💬 willcl-ark commented on issue "Slow memory leak in v22.0?":
(https://github.com/bitcoin/bitcoin/issues/24542#issuecomment-2104779556)
OK, I think I have pretty much identified the root cause here.

Calling [`blockToJSON`](https://github.com/bitcoin/bitcoin/blob/98dd4e712efaa2b77cb168426756879c6405c3f4/src/rest.cpp#L340) on a maximum-sized block allocates 70-80MB on the heap during runtime.

There is then an additional 15-20MB allocated for `strJSON`.

After these allocations are made they then seem to be re-used when possible in the future, so long as subsequent blocks are small enough to fit. This totals up to the ~100M
...