💬 maflcko commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1844329517)
nit in cca0ddbaf4dde7c9b8b50f5f1fd29cefdcf301ea: Same here. I don't see the reason to have a different log message on `-par=1`, compared to `-par=2`.

Not that it matters, because this code is deleted anyway in a future commit. However, my recommendation would be to drop this 1-line hunk, so that no inconsistency is introduced and code churn in deleted lines is kept to a minimum.

💬 maflcko commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1844296431)
nit in 73417066867ea7920647054eb0752e0dd3100de7: This is unrelated to making the error messages uniform, but I think it would be nice to use the same log level as well. Otherwise it seems a bit odd that `-par=1` will print a different log level than `-par=2`. (Personally I think that `LogInfo` is probably enough)

Edit: Doesn't matter, as this is deleted anyway in a later commit.

💬 maflcko commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1844940865)
style nit in the last commit: For new code my recommendation would be to use `LogInfo`, instead of the deprecated alias, which does exactly the same.

<!--
However, I am somewhat thinking that all non-bench logging in this function can be deleted, as all callers will redundantly log the state, except for one. So it may be clearer, less code, and less log-spam to ensure every validation error is logged exactly once (in the caller).

💬 maflcko commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1844943672)
nit in 011d196c6fff4bbcd2bd03998c64b704091d0aff: Reads a bit odd to invert the return value in this case, but I understand that keeping it return error isn't trivially possible and doesn't matter anyway. Maybe add a comment `// return value does not matter, because m_request_stop is only set in the destructor` ?

🤔 maflcko reviewed a pull request: "Improve parallel script validation error debug logging"
(https://github.com/bitcoin/bitcoin/pull/31112#pullrequestreview-2439353625)
Just some non-blocking nits. Feel free to ignore.

review ACK cc6d3ea623933cc7f0db0daaabcbdda86272c8f7 🐱

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNL
...

💬 Sjors commented on issue "Proposed Timeline for Legacy Wallet and BDB removal":
(https://github.com/bitcoin/bitcoin/issues/20160#issuecomment-2480556327)
I wrote two years ago:

> However, not too soon.

Ok, now is fine :-)

It seems #30328 and #31248 is next on the review list and then actual removal in #28710?

💬 hebasto commented on issue "depends build fails - libevent not in CMakeLists":
(https://github.com/bitcoin/bitcoin/issues/31299#issuecomment-2480631513)
> cmake version 3.10.2

This CMake version does not understand the `-S` and `-B` command line options, which were introduced in the version 3.13.

@techy2

You can download CMake 3.22 or newer, which is required for the main build system, from https://cmake.org/download/.

💬 hebasto commented on pull request "wallet: Translate [default wallet] string in progress messages":
(https://github.com/bitcoin/bitcoin/pull/31296#issuecomment-2480658902)
Concept ACK.

👍 hebasto approved a pull request: "build: increase minimum supported Windows to 10.0"
(https://github.com/bitcoin/bitcoin/pull/31172#pullrequestreview-2440774187)
re-ACK ee1128ead846698db5e5633f193883837f2fbc64, only rebased, a commit message and a comment have been amended since my recent [review](https://github.com/bitcoin/bitcoin/pull/31172#pullrequestreview-2415452160).

💬 hebasto commented on pull request "guix: scope pkg-config to Linux only":
(https://github.com/bitcoin/bitcoin/pull/31276#issuecomment-2480674078)
Concept ACK.

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2480677422)
Rebased due to the conflict with the merged https://github.com/bitcoin/bitcoin/pull/31285.

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2480690102)
Rebased due to the conflict with the merged https://github.com/bitcoin/bitcoin/pull/26593.

✅ techy2 closed an issue: "depends build fails - libevent not in CMakeLists"
(https://github.com/bitcoin/bitcoin/issues/31299)

💬 techy2 commented on issue "depends build fails - libevent not in CMakeLists":
(https://github.com/bitcoin/bitcoin/issues/31299#issuecomment-2480721823)
installed 3.22, works now
Thank you

💬 hebasto commented on pull request "cmake: add optional source files to bitcoin_crypto directly":
(https://github.com/bitcoin/bitcoin/pull/31268#issuecomment-2480760881)
> Avoid having many static libraries by adding the optional sources to the target `bitcoin_crypto` directly. Set the necessary compile options at the source file level, rather than the target level.

While working on the CMake staging branch, this was done on purpose (see https://github.com/hebasto/bitcoin/pull/172 and similar https://github.com/hebasto/bitcoin/pull/171). However, the goal [PR](https://github.com/hebasto/bitcoin/pull/157) has since been replaced.

Additionally, from the [Pro
...

💬 andrewtoth commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1845238389)
Also added fuzz harness

📝 hebasto opened a pull request: "depends: Update minimum required CMake version"
(https://github.com/bitcoin/bitcoin/pull/31300)
From the CMake 3.31 [Release Notes](https://cmake.org/cmake/help/v3.31/release/3.31.html):

> Compatibility with versions of CMake older than 3.10 is now deprecated and will be removed from a future version.

This PR updates the `cmake_minimum_required` command across all packages in depends to ensure the minimum version is not set below 3.10.

Without this change, CMake 3.31 emits a warning:
```
$ make -C depends freetype
make: Entering directory '/home/hebasto/git/bitcoin/depends'
Ex
...

🤔 hebasto reviewed a pull request: "refactor: Drop deprecated space in operator""_mst"
(https://github.com/bitcoin/bitcoin/pull/31267#pullrequestreview-2440818843)
Post-merge ACK faf21625652fd0d4bbf9b86fd9ebedb5857505ea.

💬 luke-jr commented on pull request "RPC/Wallet: Convert walletprocesspsbt to use options parameter":
(https://github.com/bitcoin/bitcoin/pull/24963#issuecomment-2480841064)
CI failure appears to be a CI bug unrelated to this PR. I don't see a way to un-draft...

👋 luke-jr's pull request is ready for review: "RPC/Wallet: Convert walletprocesspsbt to use options parameter"
(https://github.com/bitcoin/bitcoin/pull/24963)