💬 maflcko commented on pull request "test: run functional tests from ctest":
(https://github.com/bitcoin/bitcoin/pull/31312#discussion_r1846159390)
I guess the downside is that one can't pass any options to it.

Also, it is using the default `-j` here to run in parallel with the ctest `-j`.

Maybe it wasn't a good idea to begin with and using a bash alias or justfile is good enough?

✅ maflcko closed an issue: "Run functional tests from make check"
(https://github.com/bitcoin/bitcoin/issues/18816)

💬 maflcko commented on issue "Run functional tests from make check":
(https://github.com/bitcoin/bitcoin/issues/18816#issuecomment-2482396339)
I am using a bash alias myself, and given the upvotes, I guess everyone else is doing that, too.

So I guess this isn't needed, and not trivially doable anyway: https://github.com/bitcoin/bitcoin/pull/31312/files#r1846159390

👍 TheCharlatan approved a pull request: "build: Temporarily disable compiling `fuzz/utxo_snapshot.cpp` with MSVC"
(https://github.com/bitcoin/bitcoin/pull/31307#pullrequestreview-2441987631)
ACK b2d536100282bd901d3e0be7f7f4a6966e0ef817

📝 TheCharlatan opened a pull request: "refactor: Clamp worker threads in ChainstateManager constructor"
(https://github.com/bitcoin/bitcoin/pull/31313)
This ensures the options are applied consistently from contexts where they might not pass through the args manager, such as in some tests, or when used through the kernel library.

This is similar to the patch applied in 09ef322acc0a88a9e119f74923399598984c68f6, used to make applying the mempool options consistent.

---

This is part of the libbitcoinkernel project https://github.com/bitcoin/bitcoin/issues/27587

👍 maflcko approved a pull request: "refactor: Clamp worker threads in ChainstateManager constructor"
(https://github.com/bitcoin/bitcoin/pull/31313#pullrequestreview-2442087438)
lgtm ACK 09c9ba846ec44ba903de2b32a1cb0f49c7f93cb0

💬 maflcko commented on pull request "refactor: Clamp worker threads in ChainstateManager constructor":
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1846233431)
nit: For new code it would be good to use `LogInfo` instead of the deprecated alias. Also, no need for the trailing newline.

📝 maflcko opened a pull request: "nomerge: test malicious bidi char"
(https://github.com/bitcoin/bitcoin/pull/31314)

📝 maflcko opened a pull request: "build: Enable -Wbidi-chars=any"
(https://github.com/bitcoin/bitcoin/pull/31315)
I don't see a use-case for UTF-8 bidirectional control characters in this codebase. So disable them for now.

Ref: https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wbidi-chars_003d

💬 maflcko commented on pull request "build: Enable -Wbidi-chars=any":
(https://github.com/bitcoin/bitcoin/pull/31315#issuecomment-2482515067)
Can be tested with the patch in https://github.com/bitcoin/bitcoin/pull/31314

💬 l0rinc commented on pull request "refactor: Clamp worker threads in ChainstateManager constructor":
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1846243805)
Slightly unrelated, but this doesn't actually return the number of cores, but [number of concurrent threads](https://en.cppreference.com/w/cpp/thread/thread/hardware_concurrency)

💬 l0rinc commented on pull request "refactor: Clamp worker threads in ChainstateManager constructor":
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1846226372)
Seems deliberate to not clamp here instead, could you explain the reason?

💬 l0rinc commented on pull request "refactor: Clamp worker threads in ChainstateManager constructor":
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1846235202)
`// -par=-n means "leave n cores free" (number of cores - n - 1 script threads)` is this still true?

💬 l0rinc commented on pull request "refactor: Clamp worker threads in ChainstateManager constructor":
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1846266844)
as mentioned in the other comment, could we push this into `CCheckQueue` instead to avoid having to repeat this for other such calls, e.g. https://github.com/bitcoin/bitcoin/pull/31132/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R6278

💬 l0rinc commented on pull request "refactor: Clamp worker threads in ChainstateManager constructor":
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1846226818)
Do you happen to know how this threshold was derived and why it's needed in the first place?
If it's because `GetNumCores` is considered only a hint, why not clamp that instead?

💬 maflcko commented on pull request "depends: Update minimum required CMake version":
(https://github.com/bitcoin/bitcoin/pull/31300#discussion_r1846301539)
Why not submit this upstream and update? See also https://gitlab.freedesktop.org/freetype/freetype/-/merge_requests/143/diffs

💬 TheCharlatan commented on pull request "refactor: Clamp worker threads in ChainstateManager constructor":
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1846314866)
Looking at the original PR there seems to be little justification for it. It was also submitted in 2013, where machines with more than 16 cores were not common.

This current PR is just a refactor, if this should be changed, a separate PR should be opened.

💬 TheCharlatan commented on pull request "refactor: Clamp worker threads in ChainstateManager constructor":
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1846327910)
We already control the batch size from the chainman, so seemed logical to me to control the number of threads too.

🤔 l0rinc requested changes to a pull request: "ci: Update Clang in "tidy" job"
(https://github.com/bitcoin/bitcoin/pull/31306#pullrequestreview-2442201349)
Concept ACK, but approach NACK, I think some of the examples can be solved by removing the "feature" instead of just patching them.

💬 l0rinc commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1846324884)
This was already removed in the ACK-ed https://github.com/bitcoin/bitcoin/pull/30906/files#diff-3d0856e8b7f136c588b229e0cbd3b2e2c309cd097ade0879521daba4e1bb2a33L774-L777, could you please either revert this line or do it in a separate PR, since I think removing it completely is a better fix anyway.