👍 hebasto approved a pull request: "randomenv: Fix MinGW dllimport warning for `environ`"
(https://github.com/bitcoin/bitcoin/pull/33570#pullrequestreview-3318292404)
ACK 97762b1fcf5d389b4f9c06ae46d6408fb78627b0.

📝 MamunC0der opened a pull request: "Upgrade GitHub Action to download-artifact@v5"
(https://github.com/bitcoin/bitcoin/pull/33584)
Release notes:https://github.com/actions/download-artifact/releases/tag/v5.0.0

Change:
uses: actions/download-artifact@v4 -> uses: actions/download-artifact@v5

🤔 ismaelsadeeq reviewed a pull request: "kernel: Introduce initial C header API"
(https://github.com/bitcoin/bitcoin/pull/30595#pullrequestreview-3317819837)
Thanks for the addressing comments quickly and the thread safety clarification.

First Pass

- Overall the commits look solid. I've left a few more comments mostly non-blocking nice to haves.
This PR currently does three things, introduces the C header API, adds a C++ wrapper and tests, updates `bitcoin-chainstate` to use `libbitcoinkernel` wrapper

Maybe split this into two PRs:

- C header API, C++ wrapper here, then bitcoin-chainstate usage of the wrapper, other things can also be added befo
...

💬 ismaelsadeeq commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2416004410)
In "kernel: Add functions for the block validation state to C header" 43d64194940028071b9f3774f938936d5c6b57d7


nit: this makes sense here, but won't makes sense when you are reading the logs I think, perhaps just remove the "excluding any below reasons" phrase?

💬 ismaelsadeeq commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2416094811)
In "kernel: Add functions to read block from disk to C header" 8653972e2b0c3c698c4fc426ebfd23d44be14983

I think it will be worth indicating the several scenarios that the user should guard here not just a single example.
or provide a one fit all example of how that inconsistency can be prevented.
```suggestion
* chainstate manager, e.g. processing blocks, will change the chain.
```

💬 ismaelsadeeq commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2416130383)
In "kernel: Add function to read block undo data from disk to C header"

Nice that we are calling this data structure spent outputs 👍🏾

💬 ismaelsadeeq commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2416058549)
In "kernel: Add block validation to C header" f97a2428fe252c8eea4841487c89ae52ec981833

For some invalid blocks they are not new and we won't set this to 1.

💬 ismaelsadeeq commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2416281630)
This makes sense, thanks for the explanation :)

💬 ismaelsadeeq commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2416281267)
Maybe add a comment saying that?

💬 ismaelsadeeq commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2416148293)
In "kernel: Add function to read block undo data from disk to C header" c32db779023c4e0f5384174cb6269ff50f1acc41

Another usecase for the undo data is non-assumevalid swiftsync.

💬 ismaelsadeeq commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2416281014)
Fair point

💬 ismaelsadeeq commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2416282679)
> We previously had similar data structs and managed to eliminate them over time,

Why, it seems okay to me to have those structs ?

👍 hebasto approved a pull request: "Upgrade GitHub Action to download-artifact@v5"
(https://github.com/bitcoin/bitcoin/pull/33584#pullrequestreview-3318331270)
ACK b35341b9ba63a0108596e56e9eecc851a4558d98, I have reviewed the code and it looks OK.

🤔 hebasto reviewed a pull request: "Upgrade GitHub Action to download-artifact@v5"
(https://github.com/bitcoin/bitcoin/pull/33584#pullrequestreview-3318357238)
I think this might be update as well: https://github.com/bitcoin/bitcoin/blob/b510893d00760083ac36948747aa6ebd84656192/.github/actions/configure-docker/action.yml#L22

cc @willcl-ark

👍 maflcko approved a pull request: "refactor: rename `fees.{h,cpp}` to `fees/block_policy_estimator{h,cpp}`"
(https://github.com/bitcoin/bitcoin/pull/33218#pullrequestreview-3318249545)
lgtm, just one nit, which is harmless and can be ignored.

review ACK c70781d4241cb8b30fe33e7205868382031b4670 💬

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yu
...

💬 maflcko commented on pull request "refactor: rename `fees.{h,cpp}` to `fees/block_policy_estimator{h,cpp}`":
(https://github.com/bitcoin/bitcoin/pull/33218#discussion_r2416298819)
nit in 8d06b3006f48ff84f2324c485a8141118849b919: Unrelated unused include added?

💬 maflcko commented on pull request "Upgrade GitHub Action to download-artifact@v5":
(https://github.com/bitcoin/bitcoin/pull/33584#issuecomment-3385314006)
Missing `ci: ` prefix in pull title? Otherwise:

lgtm ACK b35341b9ba63a0108596e56e9eecc851a4558d98

💬 maflcko commented on pull request "randomenv: Fix MinGW dllimport warning for `environ`":
(https://github.com/bitcoin/bitcoin/pull/33570#issuecomment-3385322535)
Reminds me that the windows guix build is actually disabled in DrahtBot: https://github.com/maflcko/DrahtBot/blob/cd13339b9c72a12dc47931cd40e1c8872d9ac74d/guix/src/main.rs#L299, so pls ignore the result above, sry.

💬 dayer3 commented on issue "Error: Cannot resolve -bind address: 'bitcoind:8334=onion'":
(https://github.com/bitcoin/bitcoin/issues/26484#issuecomment-3385337855)
Hi all,
Sorry, so what is the recommended workaround for creating a Bitcoin Core onion service if Tor service is running on a different host or container, without following the _Manually create a Bitcoin Core onion service_ way and bind=a.b.c.d:8334=onion?
Best regards

💬 maflcko commented on issue "29.x: using a local libmultiprocess install will no-longer work":
(https://github.com/bitcoin/bitcoin/issues/33576#issuecomment-3385348769)
> This does seem like something good to document. Maybe the 29.x [dependencies.md](https://github.com/bitcoin/bitcoin/blob/29.x/doc/dependencies.md) file could document the version of multiprocess known to work with v29, consistent with the version used in depends

Yeah, makes sense to document this. (Looks like it isn't documented at all right now). Just documenting the exact commit should be good enough and I think tagging isn't needed, but no strong opinion.