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

🤔 janb84 reviewed a pull request: "depends: Use $(package)_file_name when downloading from the fallback"
(https://github.com/bitcoin/bitcoin/pull/33580#pullrequestreview-3318653692)
ut ACK 671b774d1b58c491b53f2b2f6ee42fb6b65a0e71

PR changes the third argument of `fetch_file_inner()` in the fallback scenario. This change ensures that, in the fallback scenario, the "friendly filename" is used to download the file instead of the "filename."

The file contains only the download logic, and I haven’t been able to verify whether the "friendly filename" is constructed the same way for the upload to the fallback server.


<details><summary>code analysis</summary>

The code
...

💬 l0rinc commented on pull request "refactor: throw `std::string_view` instead of `const char*` in constexpr/consteval functions":
(https://github.com/bitcoin/bitcoin/pull/33569#discussion_r2416657216)
Updated the PR description, thanks.

💬 l0rinc commented on pull request "refactor: throw `std::string_view` instead of `const char*` in constexpr/consteval functions":
(https://github.com/bitcoin/bitcoin/pull/33569#discussion_r2416662941)
> a more minimal one-line temporary(?) workaround

I don't like throwing primitives anyway, so I don't think the current change is a workaround only.

💬 l0rinc commented on pull request "doc: bump the template macOS version since 14 is now the minimum supported version":
(https://github.com/bitcoin/bitcoin/pull/33573#discussion_r2416668248)
+1 (except the double space in the suggestion)