💬 maflcko commented on pull request "cmake: exclude secp256k1 from all":
(https://github.com/bitcoin/bitcoin/pull/33390#issuecomment-3337304020)
Could turn into draft while CI is red?

💬 jmoik commented on pull request "cmake: Inherit WERROR setting for secp256k1 build":
(https://github.com/bitcoin/bitcoin/pull/33297#discussion_r2381326351)
Good point, I was not aware of this variable, I have adjusted the code and tested it.

💬 maflcko commented on pull request "cmake: Inherit WERROR setting for secp256k1 build":
(https://github.com/bitcoin/bitcoin/pull/33297#discussion_r2381335649)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

💬 janb84 commented on pull request "ci: Turn CentOS config into Alpine musl config":
(https://github.com/bitcoin/bitcoin/pull/33480#issuecomment-3337344205)
> > nitpicker
>
> No worries. Happy to adjust the pull description, if you have any suggestions I could take over.

How about something like this:

The CentOS task aligns with Ubuntu/Debian CI tasks in terms of libc usage, but (slightly) differs in package naming and update philosophy. I am not aware of the task ever discovering a centos-related issue, so it seems fine to recycle it into an Alpine Linux task.

💬 maflcko commented on pull request "ci: Turn CentOS config into Alpine musl config":
(https://github.com/bitcoin/bitcoin/pull/33480#issuecomment-3337358394)
thx, edited description

🤔 janb84 reviewed a pull request: "ci: Turn CentOS config into Alpine musl config"
(https://github.com/bitcoin/bitcoin/pull/33480#pullrequestreview-3270943030)
ACK fa6b2e9efece2d728bdc257c36c95db03e1a7bc4

This PR introduces more libc diversity in the CI pipeline (in the form of using Alpine), which is welcome.

💬 maflcko commented on pull request "ci: use Mold linker for asan-lsan-ubsan-integer-no-depends-usdt workflow":
(https://github.com/bitcoin/bitcoin/pull/33370#issuecomment-3337423480)
lgtm ACK f031536f2d267655a0fb40ab84d03e7ffa903d4c

Makes the config a bit more verbose, but otherwise I don't see any harm, so should be fine.

💬 willcl-ark commented on pull request "test: Run bench sanity checks in parallel with functional tests":
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3337628247)
I have had a play with porting this to cmake-native (ctest) code, and IMO the main difference is that (in my implementation) it just that it feels more obscure to collect the benchmarks, cost them (not implemented in this PR), and run them in parallel using cmake files vs a few lines of python.

Perhaps I'm doing it wrong, but I found I needed a whole new target `generate_bench_tests` to dynamically parse benchmark names _after_ bitcoin_bench was built, which can then create the necessary `cte
...

💬 maflcko commented on pull request "test: Run bench sanity checks in parallel with functional tests":
(https://github.com/bitcoin/bitcoin/pull/33142#issuecomment-3337747034)
> Perhaps I'm doing it wrong, but I found I needed a whole new target `generate_bench_tests` to dynamically parse benchmark names _after_ bitcoin_bench was built, which can then create the necessary `ctest` file, which can then be run in "true parallel" by `ctest`.

Yes, it is possible (and thanks for trying), see also https://habla.news/u/purplekarrot.net/cmake-and-test-suites. However,

* it requires more code, and the review for it.
* the win-cross issue is pre-existing and even if the c
...

💬 hebasto commented on pull request "ci: Remove bash -c from cmake invocation using eval":
(https://github.com/bitcoin/bitcoin/pull/33401#discussion_r2381735638)
```suggestion
eval "CMAKE_ARGS=($BITCOIN_CONFIG_ALL $BITCOIN_CONFIG)"
```

👍 hebasto approved a pull request: "ci: Remove bash -c from cmake invocation using eval"
(https://github.com/bitcoin/bitcoin/pull/33401#pullrequestreview-3271334044)
ACK 8406c1e6321fbc3ce739aa7ccb18c67fb706a4b9.

💬 Sjors commented on pull request "descriptor: fix comments in descriptor.cpp::DescriptorImpl":
(https://github.com/bitcoin/bitcoin/pull/33384#issuecomment-3337806987)
cc @achow101

💬 hebasto commented on pull request "ci: Enable more shellcheck":
(https://github.com/bitcoin/bitcoin/pull/32970#discussion_r2381744613)
> (can be done in a follow-up)

See: https://github.com/bitcoin/bitcoin/pull/33401.

💬 maflcko commented on pull request "rpc: refactor: use string_view in Arg/MaybeArg":
(https://github.com/bitcoin/bitcoin/pull/32983#discussion_r2381751418)
I actually like the `value_or` version better, because it has less branches (can be read left-to-right without jumps). Also, it is shorter. So my recommendation would be to merge this as-is.

💬 hebasto commented on pull request "Modernize use of UTF-8 in Windows code":
(https://github.com/bitcoin/bitcoin/pull/32380#issuecomment-3337865481)
Rebased to resolve a conflict with the merged https://github.com/bitcoin/bitcoin/pull/33229.

💬 Sjors commented on pull request "wallet: derivehdkey RPC to get xpub at arbitrary path":
(https://github.com/bitcoin/bitcoin/pull/32784#issuecomment-3337924467)
(will rebase this later)

💬 vasild commented on pull request "test: fix p2p_leak_tx.py":
(https://github.com/bitcoin/bitcoin/pull/33121#discussion_r2381936863)
Here is a change against `master` that could replace the first commit in this PR. It solves the same problem - when `NextInvToInbounds()` decides to wait more than the test timeout. In addition, by using mocktime, the diff below eliminates the waits in a few places of the test due to the `NextInvToInbounds()` delays. As a result the test passes in about 1 second instead of the current random few seconds to few tens of seconds.

```diff
--- i/test/functional/p2p_leak_tx.py
+++ w/test/function
...

👍 vasild approved a pull request: "test: fix p2p_leak_tx.py"
(https://github.com/bitcoin/bitcoin/pull/33121#pullrequestreview-3271613419)
ACK a52c148ece2296f22e2fe1b7e9584d49e23f03f2

The changes are an improvement. It seems some of that maybe can be done better, see https://github.com/bitcoin/bitcoin/pull/33121#discussion_r2381936863. I do not see that as a blocker.

✅ maflcko closed an issue: "Corecheck isn't run for latest PRs"
(https://github.com/bitcoin/bitcoin/issues/33359)

💬 maflcko commented on issue "Corecheck isn't run for latest PRs":
(https://github.com/bitcoin/bitcoin/issues/33359#issuecomment-3338152690)
thx for the fix