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

💬 stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2090831336)
In 9304257d67e6f280cc1b45021118c6bb37489d89:

> Since we don't remove blocks after inserting into
setBlockIndexCandidates anymore, this means that blocks could be inserted
multiple times now into the set - this won't have any effect, so
behavior isn't changed.

I suspect with "remove blocks" you mean erasing from `highpow_outofchain_headers`? In which case, I don't think this part of the commit message is correct, at least for this commit?

👍 stickies-v approved a pull request: "validation: stricter internal handling of invalid blocks"
(https://github.com/bitcoin/bitcoin/pull/31405#pullrequestreview-2843060953)
ACK ed172d3002da68a3ddbd5d13e7d3f0618c1378d4

I've thoroughly gone through this PR many times now, and I don't think I can assure myself anymore than I am now that it is correct and safe. Still, the code touched in this PR is fairly complicated and coupled, and I am not as familiar with this validation logic as others, so my main concern is that there are unexpected side-effects I've not considered.

Nits can be ignored, comments (if correct) can be addressed in other pulls.

💬 stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2090791378)
This code block now duplicates `InvalidBlockFound()`, minus the `setBlockIndex` erasure - but I think that should be a (fast) no-op, perhaps it's worth deduplicating this?

<details>
<summary>git diff on ed172d3002</summary>

```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 31a0f54c11..3312849024 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -4516,11 +4516,8 @@ bool ChainstateManager::AcceptBlock(const std::shared_ptr<const CBlock>& pblock,

if (
...

💬 stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2090976170)
nit: developer notes suggest line length < 100
```suggestion
const bool best_header_needs_update{
m_chainman.m_best_header->GetAncestor(invalid_walk_tip->nHeight) == invalid_walk_tip
};
if (best_header_needs_update) {
// pprev is definitely still valid at this point, but there may be better ones
m_chainman.m_best_header = invalid_walk_tip->pprev;
}
```

💬 stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2090892085)
grammar nit: parallel structures (and line-length)
```suggestion
// If invalidated, the block is irrelevant for setBlockIndexCandidates and for
// m_best_header so it can be removed from the cache
```

💬 stickies-v commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#discussion_r2090806322)
nit
```suggestion
highpow_outofchain_headers.insert({candidate->nChainWork, candidate});
```