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

💬 l0rinc commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/31306#discussion_r1846343700)
Looking at the actual usage, `ExpectSuccess` doesn't need to be a variadic template function:
```C++
ExpectSuccess(IntFn(5, true), {}, 5);
ExpectSuccess(NoCopyFn(5, true), {}, 5);
ExpectSuccess(StrFn(Untranslated("S"), true), {}, Untranslated("S"));
```
, we might as well simplify to something like:
```C++
template <typename T, typename U>
void ExpectSuccess(const util::Result<T>& result, const bilingual_str& str, U&& expected)
{
ExpectResult(result, true, str);
BOOST_CHECK_E
...

💬 m3dwards commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#issuecomment-2482647517)
Concept ACK

I like the motivation to cleanup the logs and remove the cascade of exceptions.

To test, I modified the included test to see what errors a user would have seen without the assertion:

```diff
--- a/test/functional/feature_framework_rpc_failure.py
+++ b/test/functional/feature_framework_rpc_failure.py
@@ -23,7 +23,8 @@ class FeatureFrameworkRPCFailure(BitcoinTestFramework):
self.log.info(f"Setting RPC timeout to 0 to simulate an unresponsive bitcoind, expect one w
...

💬 l0rinc commented on pull request "refactor: Clamp worker threads in ChainstateManager constructor":
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1846350891)
Is script verification completely CPU bound? If so, this limit may still be reasonable for a few more years - but unlike in 2013, it's not an unreasonably big value.

💬 TheCharlatan commented on pull request "refactor: Clamp worker threads in ChainstateManager constructor":
(https://github.com/bitcoin/bitcoin/pull/31313#issuecomment-2482649498)
Updated 09c9ba846ec44ba903de2b32a1cb0f49c7f93cb0 -> 8f85d36d68ab33ba237407a2ed16667eb149d61f ([chainstatemanagerArgs_0](https://github.com/TheCharlatan/bitcoin/tree/chainstatemanagerArgs_0) -> [chainstatemanagerArgs_1](https://github.com/TheCharlatan/bitcoin/tree/chainstatemanagerArgs_1), [compare](https://github.com/TheCharlatan/bitcoin/compare/chainstatemanagerArgs_0..chainstatemanagerArgs_1))

* Addressed @maflcko's [comment](https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1846233
...

💬 l0rinc commented on pull request "refactor: Clamp worker threads in ChainstateManager constructor":
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1846353272)
Do we expect [every call-site](https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1846266844) to do the clamping themselves?

✅ BrandonOdiwuor closed a pull request: "test: run functional tests from ctest"
(https://github.com/bitcoin/bitcoin/pull/31312)

💬 TheCharlatan commented on pull request "refactor: Clamp worker threads in ChainstateManager constructor":
(https://github.com/bitcoin/bitcoin/pull/31313#discussion_r1846366581)
I don't think this really matters, but seems consistent to control the arguments in the same place, just like with the batch size.

🤔 BrandonOdiwuor reviewed a pull request: "Bugfix: Correct first-run free space checks"
(https://github.com/bitcoin/bitcoin/pull/29678#pullrequestreview-2442281522)
Concept ACK using `GB` to represent 10<sup>3</sup> bytes for consistency with GUI

💬 Sjors commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#discussion_r1846404405)
435934fa020cec584b49c6151aa6b0d858054b37 oops, that should have been `&&`

💬 Sjors commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2482738819)
Pushed a bug fix, but still need to address the main feedback.

💬 maflcko commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2482760476)
Also, it would be good to add test coverage?

💬 maflcko commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1846425672)
re https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1834551086

Are you still working on this?

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

💬 Sjors commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2482785475)
I disabled ARM and macOS-cross as well, added a link to their failures to PR description TODO.

I think the `p2p_i2p_ports.py` failure in the previous releases job is spurious, so leaving that job in.

💬 Sjors commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2482789520)
@maflcko it's implicitly tested by the shutdown test, which is how I found this bug. Not sure how to explicitly test an early return?

💬 maflcko commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2482799390)
Are you sure it is covered, because the code is reported as uncovered in https://corecheck.dev/bitcoin/bitcoin/pulls/31297?