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

💬 l0rinc commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2416674361)
I have extracted it on purpose to signal that both branches are doing something similar and have the same dependencies

💬 maflcko 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_r2416744379)
https://clang.llvm.org/extra/clang-tidy/checks/misc/throw-by-value-catch-by-reference.html says "the usage of string literals is idiomatic." But no strong opinion.

💬 ryanofsky commented on pull request "multiprocess: Fix high overhead from message logging":
(https://github.com/bitcoin/bitcoin/pull/33517#discussion_r2416741361)
re: https://github.com/bitcoin/bitcoin/pull/33517#discussion_r2414400956

Not important, but FWIW sjors suggestion makes a little more sense to me to I don't see sending unknown log messages to the trace stream, which is effectively a black hole, as being more conservative. IMO ideal thing to do would be to return `std::optional` and `nullopt` and then log unknown messages at debug level with a prefix like "[unrecognized flags "0x%08x]".

💬 ryanofsky commented on pull request "multiprocess: Fix high overhead from message logging":
(https://github.com/bitcoin/bitcoin/pull/33517#discussion_r2416732939)
re: https://github.com/bitcoin/bitcoin/pull/33517#discussion_r2410969953

Yeah just a tradeoff between having extra variables and having an extra level a nesting. I think having more variables is worse, but no strong opinion and you should follow your preference

👍 ryanofsky approved a pull request: "multiprocess: Fix high overhead from message logging"
(https://github.com/bitcoin/bitcoin/pull/33517#pullrequestreview-3318880294)
Code review ACK dcaf4b736f9ddfc6a2695ce19221a7f383e43e20. Just rebased onto #33518 and changed assert(false) to log trace and since last review

re: https://github.com/bitcoin/bitcoin/pull/33517#pullrequestreview-3313376282

> The former seems too chatty, the latter too quiet.

Good observations, created https://github.com/bitcoin-core/libmultiprocess/issues/227 to track this

🤔 brunoerg reviewed a pull request: "test: add functional test for `TestShell` (matching doc example)"
(https://github.com/bitcoin/bitcoin/pull/33546#pullrequestreview-3318966739)
Concept ACK

💬 jonbourne commented on issue "Release Schedule for 30.0":
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3385887546)
Please do not release this version now. Reschedule and rethink. It has many people who disagree with choices made, and they have not been discussed enough to release the code. This is a very important turning point, and the implications are very difficult, if not impossible, to take back. Specifically, remove any code that touches OP_RETURN. Please do not release this version.