💬 maflcko commented on issue "MSVC 17.12.0 internal compiler error ":
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2482236115)
According to https://github.com/bitcoin/bitcoin/pull/27930#issuecomment-1602985185 MSVC is including Bitcoin Core in an internal test-suite, but I guess they are not compiling the fuzz tests? cc @CaseyCarter

💬 naumenkogs commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#issuecomment-2482237418)
ACK 5679f398e2a3ab8315d39ee674ccb8ad6a8f73c7

💬 naumenkogs commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#issuecomment-2482268618)
ACK https://github.com/bitcoin/bitcoin/commit/5736d1ddacc4019101e7a5170dd25efbc63b622a

My concern above was related to LimitMempoolSize(), but indeed it's easy to see it doesn't happen between the two `m_changeset->Apply()` calls .

💬 maflcko commented on pull request "refactor: Fix remaining clang-tidy performance-inefficient-vector errors":
(https://github.com/bitcoin/bitcoin/pull/31305#issuecomment-2482277836)
lgtm ACK 9cb24f82cf88607e289bd53833a9bb0976653b26

💬 CaseyCarter commented on issue "MSVC 17.12.0 internal compiler error ":
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2482280615)
I see a ` -DBUILD_FUZZ_BINARY=ON` in the setup, which suggests to me that we are at least building the fuzz tests. We are, however, at 0ca1d1b which seems to be about a month old. Our Real-World-Code projects don't live at head, they jump forward periodically. Is this a recent regression?

💬 maflcko commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1846091005)
Not sure about using releases which are basically EOL in less than 6 months after the release branch is created. This requires touching the release branches to bump those along.

My preference would be to use `APT_LLVM_V`

💬 maflcko commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1846097750)
are you sure this is correct? Not that it matters, but isn't it theoretically pessimising in `ExpectSuccess(StrFn(Untranslated("S"), true), {}, Untranslated("S"));`?

📝 sipsorcery opened a pull request: "Simplify constructor in fuzz test to help msvc compiler frontend parser."
(https://github.com/bitcoin/bitcoin/pull/31311)
Resolves #31303.

💬 maflcko commented on pull request "Simplify constructor in fuzz test to help msvc compiler frontend parser.":
(https://github.com/bitcoin/bitcoin/pull/31311#discussion_r1846128048)
It is easy to see that this code was generated by an LLM (https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2481584659), because it is wrong.

Luckily CI should catch this error.

📝 BrandonOdiwuor opened a pull request: "test: run functional tests from ctest"
(https://github.com/bitcoin/bitcoin/pull/31312)
Fixes https://github.com/bitcoin/bitcoin/issues/18816

This PR enables functional tests to be included in the ctest suite when the ENABLE_FUNCTIONAL_TESTS flag is set to ON during the CMake configuration.

i.e
```
cmake -B build -DENABLE_FUNCTIONAL_TESTS=ON
cmake --build build
ctest --test-dir build
```

💬 sipsorcery commented on pull request "Simplify constructor in fuzz test to help msvc compiler frontend parser.":
(https://github.com/bitcoin/bitcoin/pull/31311#discussion_r1846137713)
Whoops, I did inspect but yes, missed the static keyword getting dropped. Guess it will need to wait for an msvc fix then.

✅ sipsorcery closed a pull request: "Simplify constructor in fuzz test to help msvc compiler frontend parser."
(https://github.com/bitcoin/bitcoin/pull/31311)

💬 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)