🤔 stringintech reviewed a pull request: "kernel: Introduce C header API"
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3395010361)
Comments on commits 8 (ffd4ae23) through 15 (0e973b20) and a couple of follow-ups.

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2474390216)
Seems unnecessary since `Block` is extending a public `Handle` (also `friend` statements in `BlockTreeEntry`). Seems only the ones currently extending non-public `UniqueHandle`s would need the `friend` statements. Any reason why extending `View` and `Handle` is done publicly but not for `UniqueHandle`?

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2474403542)
Unused variable. Also line 661.

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2474982074)
Maybe not important but we could use `::ref()` to avoid the copy you [mentioned](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2468790150)? (`CChainParams` copy ctor is called in `::create()` right now)
```cpp
btck_ChainParameters::ref(const_cast<CChainParams*>(CChainParams::Main().release()));
```

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2474401085)
In commit bc6788ab description:
"Adds options for wiping the chainstate and block tree indexes to the chainstate load options" -> "... chainstate manager options" (since only one is a load option)

Also for efcfe2df commit title and 2ecd926c commit description.

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2478084857)
```suggestion
/**
* @brief Triggers the start of a reindex if the wipe options were previously set for
* the chainstate manager. Can also import an array of existing block
* files selected by the user.
*
* @param[in] chainstate_manager Non-null.
* @param[in] block_file_paths_data Nullable, array of block files described by their full filesystem paths.
* @param[in] block_file_paths_lens Nullable, array containing the lengths of each of the paths.
* @param[in] block
...

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2474412320)
It seems [this](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2459840316) was not applied (force push [message](https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3387750815) suggests otherwise)?

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2477740569)
In commit 2ecd926c description:
"import blocks from a single specified file path" -> "import blocks from specified file paths"

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2479513352)
Could this be also declared as final? (like `KernelValidationInterface`)

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2481021568)
Is this line necessary?

💬 stringintech commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2481753366)
Also related to the discussion [here](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2347348366).

💬 l0rinc commented on pull request "log: avoid collecting `GetSerializeSize` data when compact block logging is disabled":
(https://github.com/bitcoin/bitcoin/pull/33738#discussion_r2481763116)
> As far as I can tell, CTransaction's hashes are computed and cached on construction

No, I mean `uint256 hash = header.GetHash()` is only needed for the debug log - let me try separating all calculations that are only needed for debugging. Given that we're clearing `header` and `txn_available` I understand why the split wasn't done before. But we can still use `block` for the hash and `vtx_missing` for the missing, so I think it should be possible to split the two.
That's a more invasive ch
...

💬 A-Manning commented on pull request "zmq: Log bind error at Error level, abort startup on init error":
(https://github.com/bitcoin/bitcoin/pull/33727#discussion_r2481787433)
I agree that `zmqDebug`/`zmqError` would be ideal, however `zmqError` is currently used to log at the `Debug` level - changing it to log at `Error` level while keeping the type signature the same seems likely to cause misuse.

💬 maflcko commented on pull request "rest: Query predecessor headers using negative count param":
(https://github.com/bitcoin/bitcoin/pull/33752#discussion_r2481808137)
MAX_REST_HEADERS_RESULTS is a small number, so I don't think we need any architecture-specific and special PTRDIFF handling and can just use fixed size integral values.

Also, to walk backwards, I am sure you can just use `pprev` and not need to create a new method.

💬 instagibbs commented on pull request "Relax standardness rules regarding CHECKMULTISIG":
(https://github.com/bitcoin/bitcoin/pull/33755#issuecomment-3473633999)
If anyone has documentation showing the format (pre-OP_RETURN) this would be useful.

🚀 fanquake merged a pull request: "test: Format strings in `test_runner`"
(https://github.com/bitcoin/bitcoin/pull/33753)

💬 fanquake commented on pull request "test: ipc: resolve symlinks in `which capnp`":
(https://github.com/bitcoin/bitcoin/pull/33749#issuecomment-3473673447)
cc @ryanofsky

🤔 Sjors reviewed a pull request: "interfaces: enable cancelling running `waitNext` calls"
(https://github.com/bitcoin/bitcoin/pull/33676#pullrequestreview-3404815806)
tACK dcb56fd4cb59e6857c110dd87019459989dc1ec3

I left some suggestions for extra clarity.

💬 Sjors commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2481810673)
Is this checking that the response to `waitNext()` is a null template?

💬 Sjors commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#discussion_r2481802226)
nit: you could move these helper methods out of the test:

```diff
diff --git a/test/functional/interface_ipc.py b/test/functional/interface_ipc.py
index 0c9de28da0..7a447eba92 100755
--- a/test/functional/interface_ipc.py
+++ b/test/functional/interface_ipc.py
@@ -82,10 +82,20 @@ class IPCInterfaceTest(BitcoinTestFramework):
coinbase_data = BytesIO((await block_template.result.getCoinbaseTx(ctx)).result)
tx = CTransaction()
tx.deserialize(coinbase_data)
retur
...