💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2325458060)
```suggestion
auto block{std::make_shared<CBlock>()};
```

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2325447195)
I think `::create` and `::copy` methods could further improve the implementation, e.g.:

<details>
<summary>git diff on 82c503641a</summary>

```diff
diff --git a/src/kernel/bitcoinkernel.cpp b/src/kernel/bitcoinkernel.cpp
index fc927ea3d7..51d914eaa1 100644
--- a/src/kernel/bitcoinkernel.cpp
+++ b/src/kernel/bitcoinkernel.cpp
@@ -87,6 +87,19 @@ struct Handle {
{
delete reinterpret_cast<CPP*>(ptr);
}
+
+ template <typename... Args>
+ static C* create(Args
...

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2325502582)
I think this distinction is irrelevant: if chain is always unowned, this function can never be called, because the caller would never have a non-const `btck_Chain*`, in which case we can probably just remove the function, or keep it for completeness because it's harmless. Alternatively, and preferably, let's just add a `Handler::destroy` function, so we can easily change the logic for all destroy functions later (e.g. add logging if nullptr was passed, which may be helpful for debugging):

<de
...

💬 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