🤔 stratospher reviewed a pull request: "net: support overriding the proxy selection in ConnectNode()"
(https://github.com/bitcoin/bitcoin/pull/33454#pullrequestreview-3293786225)
ACK c76de2e.

(non-blocking question) did you consider saving `proxy` in `ThreadPrivateBroadcast()` in `m_private_broadcast` object and then retrieving it later in `ConnectNode` instead of passing the `proxy` as a parameter through `OpenNetworkConnection`/`ConnectNode`? It would eliminate some parameter passing that's only used for private broadcast connections. but might make `proxy` lifetime unclear (so not sure about this), just curious if you considered this.

💬 ryanofsky commented on pull request "init: Fix Ctrl-C shutdown hangs during wait calls":
(https://github.com/bitcoin/bitcoin/pull/33511#issuecomment-3360338559)
> Mmm, the GUI experience is less great (testnet4, ctrl + c from the terminal where I launched it):
> [...]
> Doesn't happen on the first commit, so might be a regression.

Oh, thanks for testing. That does sound like a regression. Will mark as draft until this is debugged

📝 ryanofsky converted_to_draft a pull request: "init: Fix Ctrl-C shutdown hangs during wait calls"
(https://github.com/bitcoin/bitcoin/pull/33511)
Signal `m_tip_block_cv` when Ctrl-C is pressed or `SIGTERM` is received, the same way it is currently signaled when the `stop` RPC is called. This lets RPC calls like `waitforblockheight` and IPC calls like `waitTipChanged` be interrupted, instead of waiting for their original timeouts and delaying shutdown.

This issue was reported by plebhash in #33463. These hangs have been present since #30409.

🤔 optout21 reviewed a pull request: "net: support overriding the proxy selection in ConnectNode()"
(https://github.com/bitcoin/bitcoin/pull/33454#pullrequestreview-3293811866)
ACK c76de2eea18076f91dd80b52f66ba790f071a2b1

Changes since [last review](https://github.com/bitcoin/bitcoin/pull/33454#issuecomment-3352795379): rebase only.

💬 willcl-ark commented on pull request "ci: fix buildx gha cache authentication on forks":
(https://github.com/bitcoin/bitcoin/pull/33508#issuecomment-3360390598)
Yes you are correct, the GHA cache is very tiny, and we have the worst type of docker caching due to our ineffcient layering in the images. So whilst we could fit many ccache entries in there, when we start adding the docker build cache blobs (which can be 0.5GB each) into the mix we quickly run out of space...

And I agree this change will not make the GHA particularly more useful. I suppose the question to ask is "would we prefer ccache hits or docker build layer hits" (i.e. does building th
...

📝 ryanofsky opened a pull request: "Update libmultiprocess subtree to support reduced logging"
(https://github.com/bitcoin/bitcoin/pull/33518)
Includes:

- https://github.com/bitcoin-core/libmultiprocess/pull/213
- https://github.com/bitcoin-core/libmultiprocess/pull/214
- https://github.com/bitcoin-core/libmultiprocess/pull/221
- https://github.com/bitcoin-core/libmultiprocess/pull/220

The last change is needed to support #33517 and fix poor performance in some cases caused by slow logging.

The changes can be verified by running `test/lint/git-subtree-check.sh src/ipc/libmultiprocess` as described in [developer notes](https
...

📝 ryanofsky opened a pull request: "Update libmultiprocess subtree in 30.x branch"
(https://github.com/bitcoin/bitcoin/pull/33519)
Includes:

- https://github.com/bitcoin-core/libmultiprocess/pull/207
- https://github.com/bitcoin-core/libmultiprocess/pull/208
- https://github.com/bitcoin-core/libmultiprocess/pull/211
- https://github.com/bitcoin-core/libmultiprocess/pull/201
- https://github.com/bitcoin-core/libmultiprocess/pull/213
- https://github.com/bitcoin-core/libmultiprocess/pull/214
- https://github.com/bitcoin-core/libmultiprocess/pull/221
- https://github.com/bitcoin-core/libmultiprocess/pull/220

Corre
...

💬 m3dwards commented on pull request "ci: fix buildx gha cache authentication on forks":
(https://github.com/bitcoin/bitcoin/pull/33508#issuecomment-3360502400)
Right now, it seems to all just squeeze in for one run on master (I think that 9.4gb is just from one CI run on master). Pushes to forks won't write to the cache but could benefit from that.

> would we prefer ccache hits or docker build layer hits

I would say ccache hits. The docker build times aren't too bad?

I still think merging this PR is fine but would also support us just disabling docker build layer caching on forks in support of a lot more space for ccache. Or just caching msan
...

💬 hebasto commented on pull request "net: make m_nodes_mutex non-recursive":
(https://github.com/bitcoin/bitcoin/pull/32394#issuecomment-3360517943)
Time to rebase once more?

💬 maflcko commented on pull request "Update libmultiprocess subtree to support reduced logging":
(https://github.com/bitcoin/bitcoin/pull/33518#issuecomment-3360524889)
Looks like the tsan CI failed?

https://github.com/bitcoin/bitcoin/actions/runs/18190498400/job/51784277326?pr=33518#step:9:5885

```
SUMMARY: ThreadSanitizer: data race (/home/admin/actions-runner/_work/_temp/build/src/ipc/libmultiprocess/test/mptest+0x6ee095) (BuildId: 0a10180b43648715b83845ce52ff1702609db59d) in kj::Refcounted::disposeImpl(void*) const

💬 vasild commented on pull request "net: make m_nodes_mutex non-recursive":
(https://github.com/bitcoin/bitcoin/pull/32394#issuecomment-3360551005)
`9a49877f26...aaa75c1a41`: rebase due to conflicts, also remove the first 3 commits from where because they were merged via https://github.com/bitcoin/bitcoin/pull/32326. Now this PR is just one commit. Also clarify a bit the commit message and the PR description.

👍 ismaelsadeeq approved a pull request: "Mempool: Do not enforce TRUC checks on reorg"
(https://github.com/bitcoin/bitcoin/pull/33504#pullrequestreview-3293966051)
Code review ACK 06df14ba75be5f48cf9c417424900ace17d1cf4d

I've reviewed the code and it verify that after this PR TRUC rules are not enforced when limit is bypassed only for single transactions validation path.


Few nits on tests which can come in follow-up with glozow suggestions as well.

💬 ismaelsadeeq commented on pull request "Mempool: Do not enforce TRUC checks on reorg":
(https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2398322992)
In "test: add more TRUC reorg coverge" https://github.com/bitcoin/bitcoin/commit/06df14ba75be5f48cf9c417424900ace17d1cf4d

This is empty mempool check in between the block generation is redundant, because we have not send any transaction to the mempool.

💬 ismaelsadeeq commented on pull request "Mempool: Do not enforce TRUC checks on reorg":
(https://github.com/bitcoin/bitcoin/pull/33504#discussion_r2398320236)
In "test: add more TRUC reorg coverge" 06df14ba75be5f48cf9c417424900ace17d1cf4d

What does "Testing overly-large child size" mean here? the tx being created has a normal size
maybe use the pointer as used above 3<-2 (large size)?

💬 maflcko commented on pull request "ci: fix buildx gha cache authentication on forks":
(https://github.com/bitcoin/bitcoin/pull/33508#issuecomment-3360615324)
> (You can see an outline here if you're interested [master...willcl-ark:bitcoin:docker-ci-containers](https://github.com/bitcoin/bitcoin/compare/master...willcl-ark:bitcoin:docker-ci-containers) from march. This attempt tried to preserve the ability to run on bare metal too, but if we didn't want that then most of the scripts could be moved into dockerfiles too, further simplifying/locking us in to docker 😋 .)

bare metal is required for the macOS CI, so I don't think we can go pure docker.
...

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2398415091)
Ah yes, nice catch. I was wondering why the PoW check was still failing so many times in the coverage.

💬 vasild commented on pull request "net: support overriding the proxy selection in ConnectNode()":
(https://github.com/bitcoin/bitcoin/pull/33454#issuecomment-3360747581)
> did you consider saving `proxy` in `ThreadPrivateBroadcast()` in `m_private_broadcast` object and then retrieving it later in `ConnectNode` instead of passing the `proxy` as a parameter through `OpenNetworkConnection`/`ConnectNode`? It would eliminate some parameter passing that's only used for private broadcast connections. but might make `proxy` lifetime unclear (not sure about this), just curious if you considered this.

In general, saving the proxy somewhere and later retrieving it from
...

💬 ryanofsky commented on pull request "Update libmultiprocess subtree to support reduced logging":
(https://github.com/bitcoin/bitcoin/pull/33518#issuecomment-3360771493)
> Looks like the tsan CI failed?

Also happens in the 30.x PR (https://github.com/bitcoin/bitcoin/actions/runs/18190672725/job/51784809732?pr=33519) so interesting that it doesn't happen with -fsanitize=thread job in libmultiprocess repository (https://github.com/bitcoin-core/libmultiprocess/actions/runs/18190339842/job/51783802159). Will debug.

🤔 stickies-v reviewed a pull request: "rpc: refactor: use string_view in Arg/MaybeArg"
(https://github.com/bitcoin/bitcoin/pull/32983#pullrequestreview-3294226533)
Force-pushed to address merge conflict from #32326, and addressed 2 review nits from @pablomartin4btc :
- replaced `strprintf` usage with `tfm::format`
- replaced `descriptor.size() == 0` with `descriptor.empty()`

💬 stickies-v commented on pull request "rpc: refactor: use string_view in Arg/MaybeArg":
(https://github.com/bitcoin/bitcoin/pull/32983#discussion_r2398528866)
Keeping as `value_or`, no strong opinion either way.