π¬ maflcko commented on pull request "build: Drop support for EOL macOS 13":
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2397392467)
> nit: This change to ci.yml in this PR should probably be broken out to a separate commit or mentioned in the commit message as now it's just "Drop support for EOL macOS 13".
Thx, I'll adjust the commit message on the next push, if there is one. Leaving as-is for now.
(https://github.com/bitcoin/bitcoin/pull/33489#discussion_r2397392467)
> nit: This change to ci.yml in this PR should probably be broken out to a separate commit or mentioned in the commit message as now it's just "Drop support for EOL macOS 13".
Thx, I'll adjust the commit message on the next push, if there is one. Leaving as-is for now.
π¬ vasild commented on pull request "net: support overriding the proxy selection in ConnectNode()":
(https://github.com/bitcoin/bitcoin/pull/33454#issuecomment-3359702419)
`5b22928a8e...c76de2eea1`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/33454#issuecomment-3359702419)
`5b22928a8e...c76de2eea1`: rebase due to conflicts
π¬ maflcko commented on pull request "ci: fix buildx gha cache authentication on forks":
(https://github.com/bitcoin/bitcoin/pull/33508#issuecomment-3359890309)
Hmm, is the GHA cache even large enough to hold them? https://github.com/m3dwards/bitcoin/actions/caches says " Approaching total cache storage limit (9.41 GB of 10 GB Used) "
So I wonder, while this may fix the auth issue, the cache will still not be more useful.
It could be useful to know what is the sum of the size of all "normal" caches (ccache, depends, ...), and then the sum of the size of all docker caches.
(https://github.com/bitcoin/bitcoin/pull/33508#issuecomment-3359890309)
Hmm, is the GHA cache even large enough to hold them? https://github.com/m3dwards/bitcoin/actions/caches says " Approaching total cache storage limit (9.41 GB of 10 GB Used) "
So I wonder, while this may fix the auth issue, the cache will still not be more useful.
It could be useful to know what is the sum of the size of all "normal" caches (ccache, depends, ...), and then the sum of the size of all docker caches.
π¬ Sjors commented on pull request "init: Fix Ctrl-C shutdown hangs during wait calls":
(https://github.com/bitcoin/bitcoin/pull/33511#issuecomment-3359916481)
ACK 68cad90dace40f7a015ca4ff81b878fc8fdc1dd5
(https://github.com/bitcoin/bitcoin/pull/33511#issuecomment-3359916481)
ACK 68cad90dace40f7a015ca4ff81b878fc8fdc1dd5
π¬ Raimo33 commented on pull request "multiprocess: Fix high overhead from message logging":
(https://github.com/bitcoin/bitcoin/pull/33517#issuecomment-3360234292)
Concept ACK
I agree with the backporting
(https://github.com/bitcoin/bitcoin/pull/33517#issuecomment-3360234292)
Concept ACK
I agree with the backporting
π¬ Sjors commented on pull request "init: Fix Ctrl-C shutdown hangs during wait calls":
(https://github.com/bitcoin/bitcoin/pull/33511#issuecomment-3360247170)
Mmm, the GUI experience is less great:
<img width="760" height="558" alt="SchermΒafbeelding 2025-10-02 om 11 55 50" src="https://github.com/user-attachments/assets/78eaf83c-b598-4fd1-abae-86aac2c47f24" />
This seems to stay stuck.
(https://github.com/bitcoin/bitcoin/pull/33511#issuecomment-3360247170)
Mmm, the GUI experience is less great:
<img width="760" height="558" alt="SchermΒafbeelding 2025-10-02 om 11 55 50" src="https://github.com/user-attachments/assets/78eaf83c-b598-4fd1-abae-86aac2c47f24" />
This seems to stay stuck.
π¬ Eunovo commented on pull request "log: print every script verification state change":
(https://github.com/bitcoin/bitcoin/pull/33336#issuecomment-3360275363)
re-ACK https://github.com/bitcoin/bitcoin/pull/33336/commits/45bd8914658a675d00aa9c83373d6903a8a9ece8
(https://github.com/bitcoin/bitcoin/pull/33336#issuecomment-3360275363)
re-ACK https://github.com/bitcoin/bitcoin/pull/33336/commits/45bd8914658a675d00aa9c83373d6903a8a9ece8
π¬ vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3360280902)
`b01d6df464...21efe8fa82`: rebase due to conflicts
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-3360280902)
`b01d6df464...21efe8fa82`: rebase due to conflicts
π€ 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.
(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
(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.
(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.
(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
...
(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
...
(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
...
(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
...
(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?
(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
(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.
(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.
(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.