👍 ryanofsky approved a pull request: "doc: document capnproto and libmultiprocess deps in 29.x"
(https://github.com/bitcoin/bitcoin/pull/33623#pullrequestreview-3343442293)
Code review ACK bf236d7a8af0826efc39a04113b79f96fe05f0c7. Looks good but left some more suggestions below. I think I might also adjust PR description not to say documented version is the latest version that will ever work (see below). I think it would be more accurate just to say that later versions are known not to work.

💬 ryanofsky commented on pull request "doc: document capnproto and libmultiprocess deps in 29.x":
(https://github.com/bitcoin/bitcoin/pull/33623#discussion_r2434900293)
In commit "doc: document capnproto and libmultiprocess deps" (bf236d7a8af0826efc39a04113b79f96fe05f0c7)

I think I'd suggest dropping this paragraph because we can't really say 5.0 is the latest version that will work. There is a [v5](https://github.com/bitcoin-core/libmultiprocess/commits/v5) branch, so in theory if there were fixes or features we wanted to backport it would be possible to create 5.1, 5.2, etc versions that should be compatible with 5.0.

Libmultiprocess doesn't seem different
...

💬 ryanofsky commented on pull request "doc: document capnproto and libmultiprocess deps in 29.x":
(https://github.com/bitcoin/bitcoin/pull/33623#discussion_r2434877805)
In commit "doc: document capnproto and libmultiprocess deps" (bf236d7a8af0826efc39a04113b79f96fe05f0c7)

I think this could list 0.7.0 as minimum required version of Cap'n Proto linking to https://github.com/bitcoin-core/libmultiprocess/pull/88 (which dropped support for previous Cap'n Proto versions) and list 5.0 as minimum required version of libmultiprocess linking to https://github.com/bitcoin/bitcoin/pull/31740 (which added a dependency on the `mp/type-*.h` files that were introduced in v
...

✅ fanquake closed a pull request: "chore: remove repetitive word in src/leveldb/README.md"
(https://github.com/bitcoin/bitcoin/pull/33638)

💬 fanquake commented on pull request "chore: remove repetitive word in src/leveldb/README.md":
(https://github.com/bitcoin/bitcoin/pull/33638#issuecomment-3409791142)
Leveldb changes need to go to the upstream repo.

💬 fanquake commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3409804641)
Yea, I have seen the same, on one of my Fedora boxes, which would be a blocker, given it breaks running the CI locally.

💬 vasild commented on pull request "net: make m_nodes_mutex non-recursive":
(https://github.com/bitcoin/bitcoin/pull/32394#discussion_r2435059938)
Such an `Assume()` would make for stricter requirements, even in `master`, or more complex reasoning to confirm that it does not make stricter requirements. I mean, can `lNodesAnnouncingHeaderAndIDs` have e.g. 5 elements when this code snippet starts? It is not obvious by just looking at this code, so one has to assume that it can or study the code elsewhere to confirm that it cannot have 4 or more.

So, both in `master` and in this PR such an `Assume()` would be triggered if `lNodesAnnouncing
...

💬 vasild commented on pull request "net: make m_nodes_mutex non-recursive":
(https://github.com/bitcoin/bitcoin/pull/32394#discussion_r2435076628)
Yes, good catch! Done.

💬 vasild commented on pull request "net: make m_nodes_mutex non-recursive":
(https://github.com/bitcoin/bitcoin/pull/32394#issuecomment-3409829097)
`4aad3714d6...4b3a2c2360`: do https://github.com/bitcoin/bitcoin/pull/32394#discussion_r2433600255

🚀 fanquake merged a pull request: "Update libmultiprocess subtree in 30.x branch"
(https://github.com/bitcoin/bitcoin/pull/33519)

💬 fanquake commented on pull request "Update libmultiprocess subtree in 30.x branch":
(https://github.com/bitcoin/bitcoin/pull/33519#issuecomment-3409877108)
Added to `30.x` rel notes in #33609.

💬 fanquake commented on pull request "multiprocess: Fix high overhead from message logging":
(https://github.com/bitcoin/bitcoin/pull/33517#issuecomment-3409878268)
Backported to `30.x` in #33609.

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3409891919)
`c652deb3c1...6e0f3a4a58`: rebase due to conflicts

> Not sure why I get this failure ...

It is making requests to the DNS server at `1111:1111::1.53`, trying to resolve `x9.dummySeed.invalid.`

👍 ryanofsky approved a pull request: "rpc: Handle -named argument parsing where '=' character is used"
(https://github.com/bitcoin/bitcoin/pull/32821#pullrequestreview-3343599681)
Code review ACK f53dbbc5057b6f676db4be9bc720898149f293fc. Just applied comment & test suggestions since last review

💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2434995750)
re: https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2434307116

> Without -named [...]

Thanks for pointing out different error messages with and without `-named`. I do feel like it would be good to have more consistent error messages but I think requiring exact same messages in `-named` and `-nonamed` cases is probably setting bar a little too high, because the way arguments are interpreted in these modes is quite different. As long as the error messages clearly describe the probl
...

💬 ryanofsky commented on pull request "rpc: Handle -named argument parsing where '=' character is used":
(https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2434995447)
re: https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2434287898

> While passing the same to getrawchangeaddress shows

Thanks for pointing this out, and see also other reply to https://github.com/bitcoin/bitcoin/pull/32821#discussion_r2434307116 about goal of having more consistent error messages.

It seems to me PR is improving behavior of the `getnewaddress` method while leaving behavior of the `getrawchangeaddress` method unchanged. Errors in both cases seem pretty helpful and
...

💬 ryanofsky commented on pull request "[30.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/33609#issuecomment-3409990384)
It could make sense to backport #33229 too, I think. It does have the [Needs backport (30.x)](https://github.com/bitcoin/bitcoin/issues?q=label%3A%22Needs%20backport%20(30.x)%22) label, but doesn't appear by default because it is closed. (Sorry if this is the wrong place to discuss)

💬 Sjors commented on pull request "test: [move-only] binary utils to utils.py":
(https://github.com/bitcoin/bitcoin/pull/33633#issuecomment-3409995075)
lgtm ACK fa75ef4328f638221bcf85fcbefa885122084622

📝 maflcko opened a pull request: " ci: Only write docker build images to Cirrus cache "
(https://github.com/bitcoin/bitcoin/pull/33639)
The `DOCKER_BUILD_CACHE_ARG` env var holds the options on how to use cache providers. Storing the image layers is useful for the Cirrus cache provider, because it offers 10GB per runner (https://cirrus-runners.app/setup/#speeding-up-the-cache). The cached image layers can help to avoid issues when the upstream package manager infra (apt native, apt llvm, pip, apk, git clone, ...) has outages or network issues.

However, on the GitHub Actions cache provider, a *total* cache of 10GB is offered f
...

✅ maflcko closed a pull request: "ci: Build ci_native_base image layer"
(https://github.com/bitcoin/bitcoin/pull/33620)