💬 maflcko commented on pull request "CMake: Add dynamic test discovery":
(https://github.com/bitcoin/bitcoin/pull/33483#issuecomment-3346048339)
> Can you explain the difference in ctest tests-cases? Is the script double discovering tests ?

Previously there was a single bench sanity check, now there is one bench sanity check for each bench target. (This is the goal of this pull request, but maybe it isn't obvious from the description)

💬 optout21 commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3346064980)
reACK d870caca33ac9f0fdd76969a7341535c5722417e
([prev](https://github.com/bitcoin/bitcoin/pull/33192#pullrequestreview-3171424512))

👍 willcl-ark approved a pull request: "ci: Turn CentOS config into Alpine musl config"
(https://github.com/bitcoin/bitcoin/pull/33480#pullrequestreview-3278870275)
ACK fa6b2e9efece2d728bdc257c36c95db03e1a7bc4

Alpine/musl feels much more useful to me than a frozen-in-time CentOS build.

Not tested locally as I'm a bit bandwidth-contrained at the moment, but the code changes and CI look good to me.

💬 willcl-ark commented on pull request "ci: Turn CentOS config into Alpine musl config":
(https://github.com/bitcoin/bitcoin/pull/33480#discussion_r2387353020)
What do you think about setting this to `alpine:latest` to automatically use new versions? Might give us the occasionaly false positive when the alpine version is auto-bumped, but feels useful to catch musl changes as early as possible?

Perhaps that's a jobs for a nightly though...

💬 willcl-ark commented on pull request "ci: Turn CentOS config into Alpine musl config":
(https://github.com/bitcoin/bitcoin/pull/33480#discussion_r2387381492)
I wonder if this needs to be `lg` any more? Could probably be `md` I think?

💬 maflcko commented on issue "How to backport libmultiprocess changes?":
(https://github.com/bitcoin/bitcoin/issues/33439#issuecomment-3346073946)
> (1) should be either _all_ of the changes or _none_ of them. At least for the next few weeks I don't think there will be any libmultiprocess changes that aren't either safe or desirable to backport, like bugfixes, documentation improvements, CI improvements, and error handling improvements. I think the only changes to avoid backporting might be PRs that add new features like [#32387](https://github.com/bitcoin/bitcoin/pull/32387) (adding windows support) or [bitcoin-core/libmultiprocess#209](h
...

💬 willcl-ark commented on issue "ci: GHA fallback centos task runs out of space":
(https://github.com/bitcoin/bitcoin/issues/33293#issuecomment-3346080573)
This will be "fixed" by #33480 (as a side-effect). If we decide not to merge that, I can open a PR with something like https://github.com/willcl-ark/bitcoin/commit/f5885d05160fe0ac167976f2b36b6d5bec373e08 in to fix the space issue on CentOS.

💬 vasild commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3346117320)
Updated the PR description. It referred to a older version of the PR.

💬 maflcko commented on pull request "ci: Turn CentOS config into Alpine musl config":
(https://github.com/bitcoin/bitcoin/pull/33480#discussion_r2387430697)
Sure, pushed a commit

💬 maflcko commented on pull request "ci: Turn CentOS config into Alpine musl config":
(https://github.com/bitcoin/bitcoin/pull/33480#discussion_r2387430936)
Yeah, it is already part of this nightly: https://github.com/maflcko/b-c-nightly/blob/9d6cf19851e57a9717da28c894b5dce6882017ca/.cirrus.yml#L147

I don't think it every found something.

Also, for consistency and reproducibility, I'd prefer to stick with the best-effort-pinned-versions, like in the other CI tasks.

💬 optout21 commented on pull request "net: support overriding the proxy selection in ConnectNode()":
(https://github.com/bitcoin/bitcoin/pull/33454#issuecomment-3346170120)
Concept ACK ec7c21534505bb727ad0344c7f6881836b4e9ec5

One doubt: adding a return type to `OpenNetworkConnection` is not described in the commit msg nor in the PR; and it seems to be needed only later by #29415 . I suggest leaving it out (as the return value is not used in this PR), or alternatively, just documenting it in the commit msg & PR description.

💬 maflcko commented on issue "ci: GHA fallback centos task runs out of space":
(https://github.com/bitcoin/bitcoin/issues/33293#issuecomment-3346177632)
> This will be "fixed" by [#33480](https://github.com/bitcoin/bitcoin/pull/33480) (as a side-effect).

Are you sure? The task config should be unchanged in this pull request. So if the pull works around it, it doesn't seem like something that can be relied upon.



> I can open a PR with something like [willcl-ark@f5885d0](https://github.com/willcl-ark/bitcoin/commit/f5885d05160fe0ac167976f2b36b6d5bec373e08) in to fix the space issue on CentOS.

That'd be fine. Maybe just apply it to all GHA run
...

💬 maflcko commented on pull request "net: support overriding the proxy selection in ConnectNode()":
(https://github.com/bitcoin/bitcoin/pull/33454#issuecomment-3346183668)
The CI seems to fail reliably?

💬 katesalazar commented on pull request "ci: check if file or directory already exists for ${HOME}/.bitcoin instead of failing":
(https://github.com/bitcoin/bitcoin/pull/33486#issuecomment-3346192356)
I think it's best when as many CI scripts as possible can be run on _any_ host. But I can agree on not cahnging this if there is no clear underlining issue being fixed.

🤔 janb84 reviewed a pull request: "CMake: Add dynamic test discovery"
(https://github.com/bitcoin/bitcoin/pull/33483#pullrequestreview-3279048407)
Approach ACK 6318195b8ecea6d4ced57c8aac97e1ca713d9822

This PR changes how (en when) we do test discovery. From configure time -> Test time and by use of a CMake module.

NIT: Please extend the PR description of the intended changed behaviour of bench sanity check tests. From single bench sanity check to one bench sanity check for each bench target.

💬 janb84 commented on pull request "CMake: Add dynamic test discovery":
(https://github.com/bitcoin/bitcoin/pull/33483#discussion_r2387475262)
This comment line, is this intended as a reminder to remove this module if the code is added upstream ?

if so, Nit: please make the intent more explicit. e.g

```suggestion
#TODO remove file if fixed upstream:
# https://gitlab.kitware.com/cmake/cmake/-/issues/26920
```

🤔 janb84 reviewed a pull request: "ci: Turn CentOS config into Alpine musl config"
(https://github.com/bitcoin/bitcoin/pull/33480#pullrequestreview-3279090957)
reACK 444409ff2b78d8f3e541bd6e883af8da7adfd264

changes since last ACK:
- changed container size to md. CI still works with this size. ✅

👍 willcl-ark approved a pull request: "ci: Turn CentOS config into Alpine musl config"
(https://github.com/bitcoin/bitcoin/pull/33480#pullrequestreview-3279091535)
ACK 444409ff2b78d8f3e541bd6e883af8da7adfd264

This took 29 minutes for an alpine depends, gui build, with no docker/ccache/depends caches (as no merges to master yet) which seems in line with other jobs.

📝 AmelkaDzikwk opened a pull request: "Create based point of view"
(https://github.com/bitcoin/bitcoin/pull/33490)
#2

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that
...

💬 AmelkaDzikwk commented on pull request "Create based point of view":
(https://github.com/bitcoin/bitcoin/pull/33490#issuecomment-3346308216)
not sure what does it mean?