💬 l0rinc commented on pull request "refactor: Remove UB in prevector reverse iterators":
(https://github.com/bitcoin/bitcoin/pull/32490#issuecomment-2883326795)
This PR is an extension of https://github.com/bitcoin/bitcoin/pull/32296 (which originally just removed the unnecessary `prevector` suppression, but @maflcko bisected the cause and removed the dead code as well).
Your reviews would be appreciated there as well.

💬 hebasto commented on pull request "build: document why we check for `std::system`":
(https://github.com/bitcoin/bitcoin/pull/32491#issuecomment-2883331380)
> Aside: i would like to eventually convert usage of `std::system` to `subprocess`, especially now (after #32343) that it doesn't leak file descriptors anymore.

:+1:

💬 hebasto commented on pull request "test: Introduce `SUPPRESS_ABORT_MESSAGE` environment variable":
(https://github.com/bitcoin/bitcoin/pull/32409#issuecomment-2883341081)
This issue seems related: https://developercommunity.visualstudio.com/t/Debug-CRT-shows-abort-message-box-instea/10904782.

👍 rkrux approved a pull request: "psbt: add non-default sighash types to PSBTs and unify sighash type match checking"
(https://github.com/bitcoin/bitcoin/pull/31622#pullrequestreview-2843186024)
re-ACK ee045b61efc1479c1866b786661ef39a863677d0

Only rebase changes from #32305
```
git range-diff f09e1f94...ee045b61
```

💬 hodlinator commented on issue "test: feature_framework_startup_failures.py fails with `--timeout-factor=0`":
(https://github.com/bitcoin/bitcoin/issues/32506#issuecomment-2883371822)
Thanks for finding & reporting. I've been preparing a follow-up PR for the one that introduced the test (#30660) last week, will add a commit to resolve this issue there.

💬 maflcko commented on issue "test: wallet_reorgsrestore.py failure under valgrind":
(https://github.com/bitcoin/bitcoin/issues/32493#issuecomment-2883411301)
I guess it is the same reason that feature_init.py was disabled in the CI only

📝 maflcko opened a pull request: "ci: Exclude failing wallet_reorgsrestore.py from valgrind task for now"
(https://github.com/bitcoin/bitcoin/pull/32507)
Fixes https://github.com/bitcoin/bitcoin/issues/32493

For some reason terminate or kill do not work inside the CI system under valgrind.

So disable the test for now, until a solution is found.

💬 fanquake commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2883423011)
Guix Build:
```bash
bcf993e048118c80beae601700c50f8d707474b0e5f89359dfc8781a12efee81 guix-build-c0f41523973e/output/aarch64-linux-gnu/SHA256SUMS.part
a6cf6122393f86b8a34e036b8a237daa3153613d154eae2f8c66720c8bad0b52 guix-build-c0f41523973e/output/aarch64-linux-gnu/bitcoin-c0f41523973e-aarch64-linux-gnu-debug.tar.gz
3330a1a5848fa80f24b181f8fe22341dca8abe9a1934a8c75834f51b311ad780 guix-build-c0f41523973e/output/aarch64-linux-gnu/bitcoin-c0f41523973e-aarch64-linux-gnu.tar.gz
eb8ba4643c8b2774
...

⚠️ fanquake opened an issue: "ci: remove third-party javascript usage from Windows CI"
(https://github.com/bitcoin/bitcoin/issues/32508)
See:

https://github.com/bitcoin/bitcoin/blob/c779ee3a4044df3a263394bbb8b104aeeaa7c727/.github/workflows/ci.yml#L189-L190

We shouldn't need to use a third-party repo, that runs some Javascript, to configure a command prompt. The comment in our code also doesn't explain why this is necessary. We should be able to drop this dependency.

Also discussed in #32396.

💬 hebasto commented on pull request "doc: Add hint about avoiding spaces in paths when building on Windows":
(https://github.com/bitcoin/bitcoin/pull/32397#issuecomment-2883438040)
> What is the status of this? Do we need to add something to the docs?

I believe it would be helpful to mention a workaround in the build documentation. [Here](https://github.com/hebasto/bitcoin/commit/6a2a5428346634c80e6856e17b2c737bb523fd14)'s a suggestion you could consider including.

Feel free to grab it.

cc @davidgumberg

💬 fanquake commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#discussion_r2090923730)
Opened #32508 to follow up with the CI.

💬 brunoerg commented on pull request "test: descriptor: cover invalid multi/multi_a cases":
(https://github.com/bitcoin/bitcoin/pull/32504#issuecomment-2883461802)
> Nice, but two commits for adding three lines of related tests seems like it could be squashed?

Done.

👍 fanquake approved a pull request: "ci: Exclude failing wallet_reorgsrestore.py from valgrind task for now"
(https://github.com/bitcoin/bitcoin/pull/32507#pullrequestreview-2843310006)
ACK fa981b90f53101bff2eda606d9479233e71736b5

💬 hodlinator commented on pull request "cmake: Restrict MSVC-specific workaround to affected versions":
(https://github.com/bitcoin/bitcoin/pull/32499#issuecomment-2883487720)
nit: Should PR description include "ref #31303"?

Tried removing the `compiler_has_bug`-logic completely and building with the version I had installed: [17.13.4](https://learn.microsoft.com/en-us/visualstudio/releases/2022/release-notes-v17.13#17.13.4)...

```
cmake -B build --preset vs2022 -DVCPKG_INSTALL_OPTIONS="--x-buildtrees-root=C:\vcpkg" -DBUILD_FUZZ_BINARY=ON -DBUILD_GUI=OFF
cmake --build build --config Release -j16
```
...did not encounter the internal compiler error?

Couldn'
...

💬 fanquake commented on pull request "cmake: Add application manifests when cross-compiling for Windows":
(https://github.com/bitcoin/bitcoin/pull/32396#issuecomment-2883491267)
This seems to be broken for mac cross builds:
```bash
make -C depends HOST=x86_64-w64-mingw32
cmake -B build --toolchain depends/x86_64-w64-mingw32/toolchain.cmake
<snip>
CMake Error at cmake/module/AddWindowsResources.cmake:9 (target_sources):
Cannot find source file:

/Users/xxx/bitcoin/src/bitcoin-util-manifest.rc
Call Stack (most recent call first):
cmake/module/AddWindowsResources.cmake:25 (add_windows_resources)
src/CMakeLists.txt:427 (add_windows_application_manifest)
...

💬 maflcko commented on pull request "test: descriptor: cover invalid multi/multi_a cases":
(https://github.com/bitcoin/bitcoin/pull/32504#issuecomment-2883537760)
lgtm ACK 58e55b17e632dbd4425dd64825b087f242ac4b7b

💬 sipa commented on pull request "test: descriptor: cover invalid multi/multi_a cases":
(https://github.com/bitcoin/bitcoin/pull/32504#discussion_r2090994342)
Strange, GitHub renders this line in the diff view as indended by 4 spaces compared to the line above. I don't see anything strange in the code itself, though.

💬 sipa commented on pull request "test: descriptor: cover invalid multi/multi_a cases":
(https://github.com/bitcoin/bitcoin/pull/32504#issuecomment-2883546999)
Concept ACK

📝 hodlinator opened a pull request: "qa: feature_framework_startup_failures.py fixes & improvements (#30660 follow-up)"
(https://github.com/bitcoin/bitcoin/pull/32509)
* Handle multiple `--tmpdir` args properly.
* Handle `--timeout-factor=0` properly (fixes #32506).
* Improve readability of expected error message (`assert_raises_message()`).
* Make suppressed error output less confusing (`wait_for_rpc_connection()`).

💬 hodlinator commented on issue "test: feature_framework_startup_failures.py fails with `--timeout-factor=0`":
(https://github.com/bitcoin/bitcoin/issues/32506#issuecomment-2883569700)
Proposed fix in #32509.