💬 mzumsande commented on issue "InitError() doesn't always halt node startup when blockchain state exists":
(https://github.com/bitcoin/bitcoin/issues/33276#issuecomment-3258705593)
Is this maybe more about wording/definitions? I'd say node startup is finished when RPC is enabled and we log "[Done Loading](https://github.com/bitcoin/bitcoin/blob/37c21ebe4078d5a7b9d8a91aca2ab8b118b9d69f/src/init.cpp#L2157C4-L2157C48)" - in the log, the node does halt before that would happen.

It's true (and could be unexpected) that `initload` may do some work connecting blocks before that, but is there a downside to that? It's not possible to simply delay the initload start without other c
...

👍 TheCharlatan approved a pull request: "kernel: make blockTip index const"
(https://github.com/bitcoin/bitcoin/pull/33321#pullrequestreview-3189781820)
ACK 75d9b72475708ee0da13fb23ef65dcced805b6af

💬 josibake commented on pull request "Silent Payments: sending":
(https://github.com/bitcoin/bitcoin/pull/28201#discussion_r2325436395)
Thanks for finding this! I've rebased on master and pulled in the latest libsecp256k1 changes; I'll push a fix for this tomorrow.

💬 l0rinc commented on pull request "headerssync: Correct unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2325496639)
You can resolve it if you feel strongly about it, it's not a blocker from my side

💬 l0rinc commented on pull request "headerssync: Correct unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-3258918462)
reACK 33d550d3044f9075cc866093c453158288f12dec

Would be great if @sipa or @sdaftuar could also take a look.

🤔 marcofleon reviewed a pull request: "contrib: update fixed seeds"
(https://github.com/bitcoin/bitcoin/pull/33283#pullrequestreview-3189994622)
ACK 939678940f6c3fdbc36d57a9c9ef6f8edf89d065

Changes in the `makeseeds.py` script look fine. I ran the script myself and the direct diff isn't super useful, but it produced a similar number of seeds at least.

👍 l0rinc approved a pull request: "kernel: make blockTip index const"
(https://github.com/bitcoin/bitcoin/pull/33321#pullrequestreview-3189998416)
Code review ACK 75d9b72475708ee0da13fb23ef65dcced805b6af

💬 Sjors commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#issuecomment-3258956684)
ACK a341e11ac92b30aac856049c696a9ac39854569d

As if by magic, `interface_ipc.py` now _does_ run on the test each commit job.

💬 Sjors commented on pull request "multiprocess: Don't require bitcoin -m argument when IPC options are used":
(https://github.com/bitcoin/bitcoin/pull/33229#discussion_r2325532799)
I asked because it triggered a missing documentation error.

💬 Sjors commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-3258979508)
Meanwhile I managed to test a script path spend with Ledger as well (though I haven't tried MuSig2 inside a script path), using https://github.com/bitcoin-core/HWI/pull/794. With that I'm pretty happy with interoperability.

Will do another code review round soon(tm).

💬 stickies-v commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2325351750)
I think this should be a `const btck_BlockTreeEntry* entry`, so I've opened https://github.com/bitcoin/bitcoin/pull/33321 for the upstream change

```suggestion
typedef void (*btck_NotifyBlockTip)(void* user_data, btck_SynchronizationState state, const btck_BlockTreeEntry* entry, double verification_progress);
```

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