💬 maflcko commented on pull request "refactor: rename `fees.{h,cpp}` to `fees/block_policy_estimator{h,cpp}`":
(https://github.com/bitcoin/bitcoin/pull/33218#issuecomment-3385146910)
Re-open to trigger the newly added GHA CI tasks

📝 maflcko reopened a pull request: "refactor: rename `fees.{h,cpp}` to `fees/block_policy_estimator{h,cpp}`"
(https://github.com/bitcoin/bitcoin/pull/33218)
This PR is a simple refactoring that does four things:

1. Renames `test/policy_fee_tests.cpp` to `test/feerounder_tests.cpp`.
2. Renames `policy/fees.{h,cpp}` to `policy/fees/block_policy_estimator.{h,cpp}`.
3. Renames `policy/fees_args.cpp` to `policy/fees/block_policy_estimator_args.cpp`.
4. Modifies `estimateSmartFee` to return the block height at which the estimate was made by adding a `best_height` unsigned int value to the `FeeCalculation` struct.

**Motivation**

In preparation
...

👍 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.