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

💬 sipa commented on issue "test: pycapnp doesn't support free threaded Python":
(https://github.com/bitcoin/bitcoin/issues/33582#issuecomment-3385401468)
@maflcko My understanding is that from the Python side, GIL or no GIL is largely unobservable, apart from a few minor things. How do you think it would affect our test framework?

💬 fanquake commented on issue "test: pycapnp doesn't support free threaded Python":
(https://github.com/bitcoin/bitcoin/issues/33582#issuecomment-3385402711)
> I'd expect there is a bunch of our own test code which relies on the GIL. I guess there is no way to find such "unsafe" Python code other than to try to run with the GIL disabled and wait for an intermittent issue to pop up at some point in time?

Possibly; however this was the only test I've seen fail so far. I'm just assuming devs/people will start using the free-threaded pythons (maybe in the near future) on their machines, and if they do, this test will fail.

💬 ryanofsky commented on issue "test: pycapnp doesn't support free threaded Python":
(https://github.com/bitcoin/bitcoin/issues/33582#issuecomment-3385442350)
Not sure about the larger issue, but it seems practically speaking it might be nice if test framework just ignored or suppressed the GIL RuntimeWarning for interface_ipc.py if the test otherwise passes.