💬 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.
(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.
(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
...
(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.
(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
(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)
(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.
(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.
(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.`
(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
(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
...
(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
...
(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)
(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
(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
...
(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)
(https://github.com/bitcoin/bitcoin/pull/33620)
💬 maflcko commented on pull request "ci: Build ci_native_base image layer":
(https://github.com/bitcoin/bitcoin/pull/33620#issuecomment-3410066006)
> With the 10GB limit, it seems better to use if for ccache+depends than to cache installed packages?
Done that in https://github.com/bitcoin/bitcoin/pull/33639
For reference, we are probably better off using the limited cache for ccache, as there are several tasks, which run out of the 500MB ccache: (musl, and the previous_releases one)
```
# du -sh /var/lib/containers/storage/volumes/ci_native_*_ccache/
886M /var/lib/containers/storage/volumes/ci_native_alpine_musl_ccache/
293M /va
...
(https://github.com/bitcoin/bitcoin/pull/33620#issuecomment-3410066006)
> With the 10GB limit, it seems better to use if for ccache+depends than to cache installed packages?
Done that in https://github.com/bitcoin/bitcoin/pull/33639
For reference, we are probably better off using the limited cache for ccache, as there are several tasks, which run out of the 500MB ccache: (musl, and the previous_releases one)
```
# du -sh /var/lib/containers/storage/volumes/ci_native_*_ccache/
886M /var/lib/containers/storage/volumes/ci_native_alpine_musl_ccache/
293M /va
...
💬 fanquake commented on pull request "chore: remove repetitive word in src/leveldb/README.md":
(https://github.com/bitcoin/bitcoin/pull/33638#issuecomment-3410171836)
Pushed this and other changes into https://github.com/bitcoin-core/leveldb-subtree/pull/57.
(https://github.com/bitcoin/bitcoin/pull/33638#issuecomment-3410171836)
Pushed this and other changes into https://github.com/bitcoin-core/leveldb-subtree/pull/57.
💬 fanquake commented on pull request "Update secp256k1 subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/33625#issuecomment-3410179559)
cc @real-or-random @jonasnick
(https://github.com/bitcoin/bitcoin/pull/33625#issuecomment-3410179559)
cc @real-or-random @jonasnick
💬 fanquake commented on pull request "Update GitHub Action to github-script@v8":
(https://github.com/bitcoin/bitcoin/pull/33634#issuecomment-3410216142)
cc @willcl-ark
(https://github.com/bitcoin/bitcoin/pull/33634#issuecomment-3410216142)
cc @willcl-ark