💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2260016193)
I think this first line is no longer relevant since `_create` also initializes the chainman, so this is probably confusing for users?

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2325512434)
I think (didn't check if we 100% comply already) all of this can be replaced with something like:

> When a pointer is returned or passed as non-const, it implies ownership is transferred. The user is
> responsible for destroying non-const pointers received, and must not destroy non-const pointers
> passed to functions.
>
> Const pointers represent views, and do not transfer ownership. Lifetime guarantees of these objects
> are described in the respective documentation.

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2325593850)
This is just a handle for objects for which we haven't defined a `copy` function, right? I'm not entirely convinced exposing a separate type for this is a good idea, I don't think there's an inherent reason we wouldn't ever allow copying a `ContextOptions`, for example, so this might change in the future? An alternative approach would be to unify everything under `Handle` making `CopyFunc` optional? That should make the API more intuitive?

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2325497062)
Is this a debugging leftover? I'm not sure this should be here?

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2325537067)
It seems like it would make sense to move all this `btck_logging_connection_create` and `btck_logging_connection_destroy` into the `LoggingConnection` ctor/dtor?

🚀 glozow merged a pull request: "Add functional test for IPC interface"
(https://github.com/bitcoin/bitcoin/pull/33201)

💬 l0rinc commented on pull request "test: Run bench sanity checks in parallel with functional tests":
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3259245336)
Concept ACK, thanks for fixing this!

💬 polespinasa commented on pull request "rpc: generateblock to allow multiple outputs":
(https://github.com/bitcoin/bitcoin/pull/32468#discussion_r2325759242)
Forgot to :)
Anyway I think this PR needs to wait for https://github.com/bitcoin/bitcoin/pull/33230 to get merged. In that case this line can come back.

💬 ajtowns commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2325764786)
More to the point, taking an alias `auto& foo = bar` means that the later comparison `foo.hashSerialized == bar.hashSerialized` is just `bar.hashSerialized == bar.hashSerialized` which isn't very useful. `const uint256 prev_hash_serialized{utxo_stats.hashSerialized};` seems like it would be better.

✅ achow101 closed an issue: "`combinepsbt` RPC does not work with P2TR inputs"
(https://github.com/bitcoin/bitcoin/issues/27219)

🤔 mzumsande reviewed a pull request: "test: fix p2p_leak_tx.py"
(https://github.com/bitcoin/bitcoin/pull/33121#pullrequestreview-3190527540)
I have now pushed an update - I went with the approach of @davidgumberg because it's simpler - when we use mocktime, `MAX_REPEATS` should no longer be necessary.

I made small changes to the suggestion above:
- used current time with `setmocktime`, going back in time seems brittle.
- I didn't understand the comment "Checking that the inv contains the wtxid would have a 1 in 10^6 chance of spuriously failing (...)", so I didn't take that part.
If `NextInvToInbounds` doesn't fire, we should
...

💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2325948813)
I think this can be removed now, it was added to be used with `BOOST_CHECK_THROW` / `BOOST_CHECK_NO_THROW` which are no longer used after https://github.com/bitcoin/bitcoin/pull/33011.

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2326001691)
Done.

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2326001953)
Added a comment to explain that.

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2326002614)
I've rewritten this using the "loop of comand-- approach".

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2326002899)
I have rewritten this.

💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#issuecomment-3259692570)
I have implemented the "abstract Cluster" approach anyway, as special-casing singletons outside of the Cluster logic was too cumbersome.

Old numbers:
* 384 bytes per singleton cluster
* 512 or 576 bytes per pair cluster

New numbers:
* 224 bytes per singleton cluster
* 520 or 584 bytes per pair cluster

📝 ryanofsky opened a pull request: "Update libmultiprocess subtree to improve build and logs"
(https://github.com/bitcoin/bitcoin/pull/33322)
Includes:

- https://github.com/bitcoin-core/libmultiprocess/pull/197
- https://github.com/bitcoin-core/libmultiprocess/pull/202
- https://github.com/bitcoin-core/libmultiprocess/pull/203
- https://github.com/bitcoin-core/libmultiprocess/pull/200
- https://github.com/bitcoin-core/libmultiprocess/pull/205

These changes should give better feedback when there are build errors, and also make IPC logs more readable.

The changes can be verified by running `test/lint/git-subtree-check.sh sr
...

💬 ryanofsky commented on issue "build: suggest -DENABLE_IPC=OFF when missing capnp":
(https://github.com/bitcoin/bitcoin/issues/33315#issuecomment-3259719642)
Subtree update is #33322

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2326052487)
I removed the `destroy` functions, like you said, there is no good reason to keep them, if we only have `const` pointers anyway. I also removed the `nullptr` checks altogether. I don't think there is really a good reason to keep them.