🤔 danielabrozzoni reviewed a pull request: "refactor: Header sync optimisations & simplifications"
(https://github.com/bitcoin/bitcoin/pull/32740#pullrequestreview-3124489895)
Thanks for the reviews!

In my last push:
- Rebased onto the right commit :sweat_smile: (see https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2269846966)
- Addressed review comments

💬 danielabrozzoni commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2281794387)
Ooops, not sure what happened there, thanks for noticing!

💬 danielabrozzoni commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2281924397)
Thanks, I didn't like the initial formatting either, but I thought it was common (looking at other constructors in the repository, no, it's not...)

I updated the formatting both in `HeadersSyncState` and `CompressedHeaders`!

💬 danielabrozzoni commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2279448653)
I think it fits slightly better with the span change... since in c++20 span doesn't have a `cbegin` method, we needed to change the initial code, and update to `ranges::all_of`

🤔 janb84 reviewed a pull request: "cmake: Drop python dependency for translate"
(https://github.com/bitcoin/bitcoin/pull/33209#pullrequestreview-3128722396)
re ACK df07b07b2d8b338b9af60ec8ffb82c20b9901f86

changes since last ACK:
- dropped shebang
- script optimizations
- ifdef style change (thanks for including my nit)

💬 alexanderwiederin commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2282651470)
If `0` is success, we should invert this, right?

```suggestion
return result ? 0 : 1;
```
The earlier error returns should also be updated from `return 0;` to `return 1;` if I am not wrong.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2282652539)
I am asking myself why need this function at all now. You can retrieve the position of an entry by getting its height, and then just retrieving the next one in the chain. How about just removing it instead?

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2282658564)
I wanted to mirror the existing behaviour of the deprecated consensus library here. There we return `1` on success of this function.

💬 furszy commented on issue "Indexes stuck on unknown best block after unclean shutdown":
(https://github.com/bitcoin/bitcoin/issues/33208#issuecomment-3197313401)
> That's the sha256 hash digest of the MuHash object, not the full `MuHash3072`. I think [@mzumsande](https://github.com/mzumsande) is correct that we can't roll `m_muhash` back if the blocks are not available.

Yeah ok, that was close. I missed the uint256 there.

🤔 mzumsande reviewed a pull request: "index: fix wrong assert of current_tip == m_best_block_index"
(https://github.com/bitcoin/bitcoin/pull/32878#pullrequestreview-3128891694)
Code Review ACK 3aef38f44b76dfda77f47dc1a0e1fdc6ff3c7766

💬 alexanderwiederin commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2282769073)
Yes, I agree.

👍 hodlinator approved a pull request: "refactor: Header sync optimisations & simplifications"
(https://github.com/bitcoin/bitcoin/pull/32740#pullrequestreview-3128785887)
ACK 50e61f448eedfe0ccfa07f3124aeef9c7762a69b

PR improves the code in several minor ways, such as:
* Computing PoW for a `CBlockHeader` (or `CBlock`) without having to jump through temporarily constructed `CBlockIndex` (552b9c3a565c1817074468bf71fb4526f3a47f42).
* Removing `CBlock::GetBlockHeader()` leaves less room for forgetting to properly set fields in the new `CBlockHeader` instance (67aa62c80e7d2faa485ba020cefe262f479190f4).
* Changes `const CBlockIndex* m_chain_start` to a reference
...

💬 hodlinator commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2282695753)
(Still have a slight preference for using the original way of formatting the `HeadersSyncState` initializer-list for optimal `git blame` integrity, but this is tolerable compared to the previous push, I see it matches `PeerManagerImpl`).

🤔 ismaelsadeeq reviewed a pull request: "Add functional test for IPC interface"
(https://github.com/bitcoin/bitcoin/pull/33201#pullrequestreview-3128857140)
Approach ACK

First pass initial comments

💬 ismaelsadeeq commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2282752457)
In "ci: add functional test for IPC interface" https://github.com/bitcoin/bitcoin/commit/3f7ff4849e6165a6bcc4d5977c35a467fd7fc232

This currently does not work on macOs when the pycapnp uses system installed capnproto and the system installed capnproto is installed from homebrew.

Editing it to

```python
imports = ["/opt/homebrew/opt/capnp/include", str(src_dir), str(mp_dir)]
```

I thought of two ways to fix it but was not successful.
1. Whether we can get the path to the include
...

💬 ismaelsadeeq commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2282735877)
In "ci: add functional test for IPC interface" 3f7ff4849e6165a6bcc4d5977c35a467fd7fc232

Should modules be a test parameter initialised in `set_test_params` so that we can later reuse in mining test?

💬 ismaelsadeeq commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2282757571)
In "ci: add functional test for IPC interface" https://github.com/bitcoin/bitcoin/commit/3f7ff4849e6165a6bcc4d5977c35a467fd7fc232

Should be abstracted away as a helper since to be reused ?
```python
async def parse_and_deserialize_block(self, block_template, ctx):
block_data = BytesIO((await block_template.result.getBlock(ctx)).result)
block = CBlock()
block.deserialize(block_data)
return block
```
```suggestion
block = await self.parse
...

💬 ismaelsadeeq commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2282776829)
In "ci: add functional test for IPC interface" https://github.com/bitcoin/bitcoin/commit/3f7ff4849e6165a6bcc4d5977c35a467fd7fc232

Is their benefit of having context for each test, else we can just have a global context?

💬 ismaelsadeeq commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2282772502)
In "ci: add functional test for IPC interface" https://github.com/bitcoin/bitcoin/commit/3f7ff4849e6165a6bcc4d5977c35a467fd7fc232

Also add a branch for when we have a better template based on increase in transaction fees in the mempool above the fee threshold.

```suggestion
assert_equal(len(block2.vtx), 1)
# Wait for another, get one after increase in fees in the mempool.
waitnext = template2.result.waitNext(ctx, waitoptions)
self.nodes[0
...

💬 ismaelsadeeq commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2282774168)
In "ci: add functional test for IPC interface" https://github.com/bitcoin/bitcoin/commit/3f7ff4849e6165a6bcc4d5977c35a467fd7fc232

nit: use our internal assertion helpers for better errors?

💬 maflcko commented on issue "ci: failure in `logging_tests`":
(https://github.com/bitcoin/bitcoin/issues/33195#issuecomment-3197464559)
> Edit: running the test from the flash drive just made the test so slow that it was triggering the [20s reset window,](https://github.com/bitcoin/bitcoin/blob/7b4a1350dfd6b3892a9c314eeff28b22ef0c1a73/src/test/logging_tests.cpp#L459) causing the log test to fail because the suppression status was reset. This is however not what happened in https://cirrus-ci.com/task/5318274667249664?logs=ci#L721, as the test started and failed all in a span of a few milliseconds.

I guess it is rare that someone
...