👍 hebasto approved a pull request: "depends: Bump boost to 1.88.0 and use new CMake buildsystem"
(https://github.com/bitcoin/bitcoin/pull/32665#pullrequestreview-2889828316)
My Guix build:
```
aarch64
32f567efe7c9d428083d9eab15e9fb215e2860132b3a31a60f288d21d3ed3644 guix-build-3a350c8a1b51/output/aarch64-linux-gnu/SHA256SUMS.part
13bfeec126cef80d97a7b90f9cec32962cce8b3bbf445006f1233e1b45a74d43 guix-build-3a350c8a1b51/output/aarch64-linux-gnu/bitcoin-3a350c8a1b51-aarch64-linux-gnu-debug.tar.gz
22b967a5fdcc39fb999e9e30ac74b18509048f8918ddf08ab8a1de47bd587565 guix-build-3a350c8a1b51/output/aarch64-linux-gnu/bitcoin-3a350c8a1b51-aarch64-linux-gnu.tar.gz
3ce04680
...

💬 achow101 commented on pull request "util: Abort on failing CHECK_NONFATAL in debug builds":
(https://github.com/bitcoin/bitcoin/pull/32588#issuecomment-2932373695)
Concept ACK

🤔 hodlinator reviewed a pull request: "util: explicitly close all AutoFiles that have been written"
(https://github.com/bitcoin/bitcoin/pull/29307#pullrequestreview-2861079249)
Code review c7b68dceb29ef67796f7edf79a161bbda766169c

This is a clear incremental improvement over what is on master.

Thanks for taking my suggestions!

Don't foresee any further issues preventing an A-C-K beyond the inline comments below.

💬 hodlinator commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2122083385)
Not sure how to handle this in the callers, probably something along these lines:
```diff
diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp
index 6b47a0fa0a..332e700995 100644
--- a/src/index/blockfilterindex.cpp
+++ b/src/index/blockfilterindex.cpp
@@ -227,10 +227,13 @@ size_t BlockFilterIndex::WriteFilterToDisk(FlatFilePos& pos, const BlockFilter&

// Pre-allocate sufficient space for filter data.
bool out_of_space;
- m_filter_fileseq->Allocat
...

💬 hodlinator commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2110214040)
In this specific case of mempool_persist.cpp/`DumpMempool` my variant simply had:
```C++
FileWriter file{mockable_fopen_function(file_fspath, "wb"), [] (int err) {
Assume(std::uncaught_exceptions() > 0); // Only expected when exception is thrown before fclose() below.
}};
```
Which would serve to enforce that we either already explicitly `fclose()`d the file and checked `errno` further down in the function (leading to the lambda not being called upon destruction), or that a
...

💬 hodlinator commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2102466670)
Probably safer to check against `0` in this file as you've done elsewhere in this PR:
```suggestion
if (fclose(file) != 0) {
```
(Seems like we can be more certain of the value of `0`/success than `EOF`/failure:
https://www.man7.org/linux/man-pages/man3/fclose.3.html
https://en.cppreference.com/w/c/io/fclose)

🚀 achow101 merged a pull request: "wallet: init, don't error out when loading legacy wallets"
(https://github.com/bitcoin/bitcoin/pull/32449)

💬 achow101 commented on pull request "rpc: Use type-safe HelpException":
(https://github.com/bitcoin/bitcoin/pull/32660#issuecomment-2932409311)
It doesn't seem like this is catching the issue in https://github.com/bitcoin/bitcoin/pull/29136#discussion_r2120666028? I tried making a similar change to a RPC in this PR and the test still passed.

💬 hebasto commented on pull request "depends: Bump boost to 1.88.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/32665#discussion_r2122112935)
The latter.

👍 ryanofsky approved a pull request: "util: Abort on failing CHECK_NONFATAL in debug builds"
(https://github.com/bitcoin/bitcoin/pull/32588#pullrequestreview-2889836067)
Code review ACK faae9a294798c6808ec82f827385d920f8d697cf. I think this is a good change. It makes sense conceptually to have check macros that always abort in debug builds, but do different things depending on cost of the check & severity of the error in release builds.

re: https://github.com/bitcoin/bitcoin/pull/32588#issuecomment-2903414680

> I don't think this solves issue (1). Instead of NONFATAL being inaccurately named in debug builds, it will now be THROW, because the check is neith
...

💬 ryanofsky commented on pull request "util: Abort on failing CHECK_NONFATAL in debug builds":
(https://github.com/bitcoin/bitcoin/pull/32588#discussion_r2122090155)
In commit "util: Abort on failing CHECK_NONFATAL in debug builds" (fae8c02268c11f2ad6165b6437b3e4846babfaf5)

IMO, it would be better not to drop this test coverage so that we can verify that the RPC code catching the
`NonFatalCheckError` exception and turning into a JSON response works.

The commit message mentions Assume and Assert don't have test coverage either, but I don't see why they shouldn't have it.

It seems like it would be pretty easy to keep the test coverage here either by
...

💬 achow101 commented on pull request "wallet: add codex32 argument to addhdkey":
(https://github.com/bitcoin/bitcoin/pull/32652#issuecomment-2932435166)
Concept ACK-ish

This is certainly better than the previous approach.

💬 enirox001 commented on pull request "contrib: Add external RPC code generator with unit tests":
(https://github.com/bitcoin/bitcoin/pull/32140#issuecomment-2932446848)
> Are you still working on this, or can it be closed?

No, not working on this at the moment, can be closed. Will probably look into it in the future, but not at the moment.

💬 willcl-ark commented on pull request "depends: Bump boost to 1.88.0 and use new CMake buildsystem":
(https://github.com/bitcoin/bitcoin/pull/32665#issuecomment-2932452212)
Would we not want to bump the boost version in [AddBoostIfNeeded.cmake](https://github.com/bitcoin/bitcoin/blob/f999c3775c12ef732a1920dd52257a8cd34cdcc8/cmake/module/AddBoostIfNeeded.cmake#L29) too?

✅ fanquake closed a pull request: "contrib: Add external RPC code generator with unit tests"
(https://github.com/bitcoin/bitcoin/pull/32140)

💬 fjahr commented on pull request "index: Fix coinstats overflow and introduce index versioning":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2122135447)
> Yeah, I wonder if we can reliably test anything other than the happy path anyway. Generally, once UB happened, there is no reliable way to recover from it. While in practise here, most likely you'll end up with negative values, I don't think this is guaranteed by the C++ standard and probably not something to be able to rely on.

Sure, there is no guarantee that the check that the corruption test is testing will actually be hit for every corrupt index. I still think it would be good to have
...

📝 hebasto opened a pull request: "build: Find Boost in config mode"
(https://github.com/bitcoin/bitcoin/pull/32667)
The `FindBoost` module has been removed by policy [CMP0167](https://cmake.org/cmake/help/latest/policy/CMP0167.html).

Based on https://github.com/bitcoin/bitcoin/pull/32665.

💬 enirox001 commented on issue "Potential data race on fHavePruned flag":
(https://github.com/bitcoin/bitcoin/issues/21627#issuecomment-2932530938)
@pinheadmz are you still looking into this? Seems it's been a while since any progress has been made to it?

💬 achow101 commented on pull request "thread-safety: fix annotations with REVERSE_LOCK":
(https://github.com/bitcoin/bitcoin/pull/32465#issuecomment-2932543075)
Concept ACK

> preparing for the follow-up commits that will actually remove the lock/unlock methods completely, in order to force the exception-safe RAII wrappers.

I think that's a good idea.

💬 fanquake commented on pull request "build: Find Boost in config mode":
(https://github.com/bitcoin/bitcoin/pull/32667#discussion_r2122189722)
> We cannot rely on find_package(Boost ...) to work properly without
> Boost_NO_BOOST_CMAKE set until we require a more recent Boost because
> upstream did not ship proper CMake files until 1.82.0.

We still support Boost `1.73`+, and older verisons of CMake. Why is this ok to remove now?