👍 maflcko approved a pull request: "ci: Test cross-built Windows executables on Windows natively"
(https://github.com/bitcoin/bitcoin/pull/31176#pullrequestreview-2476490310)
lgtm

💬 maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1868207380)
I am halfway thinking that this can be put under the `DANGER_CI_ON_HOST_CCACHE_FOLDER`. Either keeping the name or renaming it.

This avoids a complexity explosion for stuff that is only used by GHA IIRC.

🤔 maflcko reviewed a pull request: "ci: Test cross-built Windows executables on Windows natively"
(https://github.com/bitcoin/bitcoin/pull/31176#pullrequestreview-2476495514)
It would be good to mention in the pull request description one sentence on why this is a good idea, instead of just a single word "fixes #ref" and a link to a GHA run.

💬 maflcko commented on pull request "optimization: change XOR obfuscation key from `std::vector<std::byte>{8}` to `uint64_t`":
(https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2515307127)
> It seems like if you take the top 50 sizes it covers 99.6% of the writes

I had the same thought. Obviously there could be an unlikely problem if the remaining 0.04% of writes accounted for the majority of the time, but that seems unlikely. Other than that, taking only the top N seems preferable.

💬 theuni commented on pull request "cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree":
(https://github.com/bitcoin/bitcoin/pull/31379#issuecomment-2515323391)
🤦🤦

Could you split this into 2 commits? The fixes you describe sound simple enough, but the removal of the helpers makes this hard to review.

💬 theuni commented on pull request "[POC] cmake: Introduce LLVM's Source-based Code Coverage reports":
(https://github.com/bitcoin/bitcoin/pull/31394#issuecomment-2515362105)
I haven't played with coverage in CMake much, but this seems like a more reasonable approach than the build type.

> Would we keep supporting generating coverage with gcc?

I don't think we should keep both approaches. If we do want to continue to support gcc, IMO it should be migrated to this setting rather than keeping it as a build type.

But.. there's a lot of duplicated/hard-coded logic here. Can we not deduce how to filter/what to run based on target properties?

💬 mjdietzx commented on pull request "tests: add functional test for miniscript decaying multisig":
(https://github.com/bitcoin/bitcoin/pull/29156#issuecomment-2515462391)
@rkrux I updated to master and rebased. There were no conflicts and this test continues to pass without needing any modifications. This should be ready to merge

Note: I will update to multi-path descriptors now that it's merged in a tiny followup PR. but I don't want to mess with all the prior reviews on this PR so not introducing any new changes

💬 achow101 commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#issuecomment-2515463182)
ACK 492e1f09943fcb6145c21d470299305a19e17d8b

✅ achow101 closed an issue: "log/dump more information if a CheckQueue failure occurs"
(https://github.com/bitcoin/bitcoin/issues/30960)

🚀 achow101 merged a pull request: "Improve parallel script validation error debug logging"
(https://github.com/bitcoin/bitcoin/pull/31112)

👍 theuni approved a pull request: "cmake: Fix `IF_CHECK_PASSED` option handling"
(https://github.com/bitcoin/bitcoin/pull/31231#pullrequestreview-2476680219)
utACK 97a18c85458b898fe5e3abda9528a2de442766ad

Confirmed that this indeed a list.

To other reviewers: the `string(STRIP "${foo} ${bar}" ${baz})` syntax is confusing. It's just concatenating foo and bar and storing it in baz with no whitespace at the beginning or end.

@hebasto Is there a way to get CMake to warn us about these? It seems like it should know that we were attempting a string operation on a list.

💬 hodlinator commented on pull request "test: Shut down framework cleanly on RPC connection failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1868331201)
(Could possibly use `collections.defaultdict(int)` instead of `Counter` iff I really need to retouch).

💬 hebasto commented on pull request "cmake: Fix `IF_CHECK_PASSED` option handling":
(https://github.com/bitcoin/bitcoin/pull/31231#issuecomment-2515497249)
> @hebasto Is there a way to get CMake to warn us about these? It seems like it should know that we were attempting a string operation on a list.

I'm not aware of any such embedded functionality. CMake's internally represents both "strings" and "lists" as strings. A list can safely be treated as a string when it contains fewer than two elements.

🤔 ryanofsky reviewed a pull request: "multiprocess: Add bitcoin wrapper executable"
(https://github.com/bitcoin/bitcoin/pull/31375#pullrequestreview-2476650402)
Thanks for the reviews!

Updated da108a6e5be220654a65b6613ee7eb2c4ddc8677 -> 02567bf2bef5997ea5f0765780d196f36d3053e8 ([`pr/wrap.5`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.5) -> [`pr/wrap.6`](https://github.com/ryanofsky/bitcoin/commits/pr/wrap.6), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/wrap.5..pr/wrap.6)) to fix windows build warning and making a change to avoid a potentially confusing behavior https://github.com/bitcoin/bitcoin/pull/31375#discussion_r18618148
...

💬 ryanofsky commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1868303275)
re: https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1861814807

> [b06c7ad](https://github.com/bitcoin/bitcoin/commit/b06c7ad0ae91102fe8cdcac6f86d627aace2219b): this is potentially confusing. If I build without gui, I expect `build/src/bitcoin gui` to fail. Right now it would launch any `bitcoin-qt` in my `$PATH`.

Agree this behavior is potentially confusing, and prevented it for now, but I'm not totally I sure see it as a downside. I like the simplicity of `bitcoin daemon` being
...

💬 ryanofsky commented on issue "RFC: Multiprocess binaries and packaging options":
(https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-2515536756)
The `bitcoin` wrapper discussed above is added in PR #31375

💬 hebasto commented on pull request "cmake: Fix passing `APPEND_*FLAGS` to `secp256k1` subtree":
(https://github.com/bitcoin/bitcoin/pull/31379#issuecomment-2515540501)
> 🤦🤦
>
> Could you split this into 2 commits? The fixes you describe sound simple enough, but the removal of the helpers makes this hard to review.

Sure thing! Done.

💬 ryanofsky commented on pull request "multiprocess: add bitcoin-mine test program":
(https://github.com/bitcoin/bitcoin/pull/30437#issuecomment-2515543821)
Updated 27072547bb22cdd2080d1014eba9c30bc9d47650 -> 99f2f335b6a8c2b5fe518a0c0056eb2a42c0496b ([`pr/mine.14`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.14) -> [`pr/mine.15`](https://github.com/ryanofsky/bitcoin/commits/pr/mine.15), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/mine.14..pr/mine.15)) just moving a line to avoid a merge conflict.

💬 darosior commented on pull request "Multiprocess bitcoin":
(https://github.com/bitcoin/bitcoin/pull/10102#issuecomment-2515554249)
Not sure if it's worth sharing here but hey what's an 1300th comment.

Running `bitcoin-node` from https://github.com/bitcoin/bitcoin/pull/29409 with commits `1045047a73a0b3ae15647ab5bf1829d27141b7cd`..`9ca113f326fc9ba2f661595283f4aef390df984d` from this PR cherry-picked on top.

Simply compiled with `cmake -B multiprocbuild/ -DWITH_MULTIPROCESS=ON && cmake --build multiprocbuild/ -j20`. (Although i did have to apply the following diff to be able to compile on Debian stable.)
<details>
<su
...

💬 hebasto commented on pull request "qa: Fix `wallet_multiwallet.py`":
(https://github.com/bitcoin/bitcoin/pull/31410#discussion_r1868374227)
I'm not sure if testing the scenario with the modified ACL on Windows is worth the added complexity of the test script.