💬 ryanofsky commented on pull request "multiprocess: Don't require bitcoin -m argument when IPC options are used":
(https://github.com/bitcoin/bitcoin/pull/33229#issuecomment-3266988266)
All ci jobs are passing now that the new `tool_bitcoin.py` test is disabled on windows, so this PR should be ready to review. I'll set up a vm and address the windows test problem in a followup PR, since I think the _wexecvp/stdout issue might not be trivial to fix.

<!-- begin push-24 -->
Updated 0c82805f75438c38f5a7f942df2a96ec3d61d68e -> f9685d6a1389938b0cceb31d9eef201ab3dd2e86 ([`pr/ipc-auto.23`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-auto.23) -> [`pr/ipc-auto.24`](https://gi
...

💬 mzumsande commented on issue "use -loadblock to load blk*****.dat files, but the blocks in it are not recognized":
(https://github.com/bitcoin/bitcoin/issues/33280#issuecomment-3266992579)
I looked at the code, and it appears that `loadblock` currently only works with non-xor'ed block files:
[AutoFile file{fsbridge::fopen(path, "rb")};](https://github.com/bitcoin/bitcoin/blob/3eea9fd39532eeda648e44de365fc4c9112f6fc6/src/node/blockstorage.cpp#L1253C9-L1254C1) doesn't use an obfuscation key.

I don't know if that was discussed when block obfuscation was added (FYI @maflcko).
`loadblock` is probably not a feature that is used very much these days, but unless someone wants to add `
...

🚀 fanquake merged a pull request: "[29.x] *san CI backports"
(https://github.com/bitcoin/bitcoin/pull/33294)

👍 ryanofsky approved a pull request: "clang-tidy: Disable `UndefinedBinaryOperatorResult` check in `src/ipc`"
(https://github.com/bitcoin/bitcoin/pull/33312#pullrequestreview-3197272526)
Code review ACK 589b65f06c3376df4cde3fac5c1d39a2d3254920. Thanks for the fix!

It could make sense to make a similar change in the libmultiprocess repository, so the `src/ipc/libmultiprocess/` directory would be covered and this fix could be moved to `src/ipc/capnp/` instead of `src/ipc/`, but current approach seems simpler and in the long run when https://github.com/capnproto/capnproto/pull/2334 rolls out, or the LLVM isInMainFile check works better, this can be dropped.

Not running clang-
...

✅ ryanofsky closed a pull request: "clang-tidy: disable clang-analyzer UndefinedBinaryOperatorResult warning"
(https://github.com/bitcoin/bitcoin/pull/33281)

💬 ryanofsky commented on pull request "clang-tidy: disable clang-analyzer UndefinedBinaryOperatorResult warning":
(https://github.com/bitcoin/bitcoin/pull/33281#issuecomment-3267075492)
Closing in favor of #33312, which actually fixes the the error output, unlike this PR

💬 D33r-Gee commented on pull request "Interfaces: Expose UTXO Snapshot Loading and Add Progress Notifications":
(https://github.com/bitcoin/bitcoin/pull/33117#discussion_r2330771778)
Thanks @Sjors for providing feedback and suggesting to add test coverage via the RPC...

Looking into it now and will update shortly

👍 ryanofsky approved a pull request: "guix: strip binaries in libexec"
(https://github.com/bitcoin/bitcoin/pull/33342#pullrequestreview-3197287990)
Code review ACK 3cceda9f4855ac1f5df349f42efdbf5058d08f48. Good catch and thanks for the fix

💬 hebasto commented on pull request "guix: strip binaries in libexec":
(https://github.com/bitcoin/bitcoin/pull/33342#issuecomment-3267088084)
Concept ACK.

Building.

💬 D33r-Gee commented on pull request "[DRAFT] Expose AssumeUTXO Load Snapshot Functionality To The GUI":
(https://github.com/bitcoin-core/gui/pull/870#issuecomment-3267106706)
> I'm a bit confused about the relation between this and [bitcoin/bitcoin#33117](https://github.com/bitcoin/bitcoin/pull/33117). I would suggest rebasing [60f676a](https://github.com/bitcoin-core/gui/commit/60f676abf54a454361ae9f5dd17509ec929acbac) on that branch for clarity.
>

Will rebase shortly with the changes from [bitcoin/bitcoin#33117](https://github.com/bitcoin/bitcoin/pull/33117)

> Managed to load the most recent testnet4 snapshot at height 90,000! (after I rebased)

WooHoo!

...

💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2330818442)
Thanks! I’ve updated the comment to note that the tidy task uses `git diff`, which would be affected by the patch if it weren’t skipped.

💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2330819058)
Thanks! Taken.

💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#issuecomment-3267157378)
The feedback from @ryanofsky has been addressed.

💬 ryanofsky commented on pull request "draft: CMake Goals and Guidelines":
(https://github.com/bitcoin/bitcoin/pull/33317#issuecomment-3267190797)
Concept ACK. Seems nice to document goals of cmake build. To avoid confusion, I hope the document will be expanded to say which of the guidelines are currently being met, which are more aspirational, and what needs to change to meet the goals. i think it would also be helpful to link to PRs/issues where these topics have been discussed. Other ideas (not sure they are good but just in case):

- Maybe add this to the doc/design/ folder instead of the doc/ folder, especially if you think if will
...

💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2330865174)
An upstream issue: https://github.com/include-what-you-use/include-what-you-use/issues/1809.

💬 l0rinc commented on pull request "chore: compare against keys.size() in writeObject comma check":
(https://github.com/bitcoin/bitcoin/pull/33337#issuecomment-3267229143)
NACK, the existing value makes more sense to me, it's not solving any actual problem

👍 ryanofsky approved a pull request: "ci, iwyu: Treat warnings as errors for specific directories"
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-3197403345)
Code review ACK d03d861cb6d81f90f3b4add2db3dbdc0bf95a0ef. Just fixing up comment and changing variable name since last review. Thanks for the updates!

💬 ryanofsky commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2330883192)
re: https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2330818442

In commit "ci: Do not patch `leveldb` to workaround UB in "tidy" CI job" (b7248ee2462a26418f0ef1e0a0f04950269d37aa)

Thanks, with new comment I think I understand why this is needed now. This is fine as is, but in case you do want to update it, I think a change like following would make the link between `git diff` and the UB detector clearer:

```diff
--- a/ci/test/03_test_script.sh
+++ b/ci/test/03_test_script.sh
...

💬 l0rinc commented on pull request "Fix benchmark CSV output":
(https://github.com/bitcoin/bitcoin/pull/33340#discussion_r2330890677)
We should likely update the arm version as well: https://github.com/bitcoin/bitcoin/pull/33340/files#diff-687f22b3470aa3123586a32b91f6aaead5bd364298e6b91954cd9a2ef3f00763L683

💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2330899302)
Thanks! The comments have been reworked per your suggestion.

💬 purpleKarrot commented on pull request "draft: CMake Goals and Guidelines":
(https://github.com/bitcoin/bitcoin/pull/33317#issuecomment-3267270664)
Thanks for the review, @ryanofsky.

I wanted to write down the goals first, and then create issues/PRs to make changes towards the goals bottom up: that means starting from libmultiprocess, minisketch, secp256k1. bitcoin-core will serve as an example how those libraries are consumed as subprojects.

In the next step, bitcoin-core should follow the same goals and guidelines, because external clients may want to consume bitcoinkernel as a subproject, or because bitcoin-gui is extracted as anot
...