👍 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?

💬 achow101 commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2122204265)
In ca5781e23a8f299ff4f143d2355218f551e65944 "config: allow setting -proxy per network"

nit: I think using `Split()` would make this easier to read

💬 achow101 commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2122190719)
In ca5781e23a8f299ff4f143d2355218f551e65944 "config: allow setting -proxy per network"

nit: `Split()`? (from `src/util/string.h`)

💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2121983471)
Yes, but when breaking apart a struct and two of the new variable names match struct field names with identical types, it would be nice to have some kind of guard rail.

💬 hodlinator commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2122154034)
Just realized a different possible approach: passing `HEADER_COMMITMENT_PERIOD` and `REDOWNLOAD_BUFFER_SIZE` values into the `HeadersSyncState`-ctor. Right now that aspect of the class is hardcoded for mainnet, despite the ctor accepting `Consensus::Params`. Having these values be sent in would enable tests to send in different (smaller) variables for regtest, while still testing behavior around the threshold.

In fact, it would fit nicely with other values updated during the release process t
...

💬 enirox001 commented on issue "wallet: render BIP388 wallet policies":
(https://github.com/bitcoin/bitcoin/issues/32659#issuecomment-2932640309)
Would be open to explore how we can add those `policy` and `keys` fields to the `getdescriptorinfo` and `listdescriptors` RPCs for BIP 388 wallet policies. I'll be diving into the BIP and the existing code in the next few days, and definitely checking out that older HWI PR for insights.

🤔 achow101 reviewed a pull request: "Feature: Use different datadirs for different signets"
(https://github.com/bitcoin/bitcoin/pull/29838#pullrequestreview-2890052727)
Concept ACK

💬 achow101 commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#discussion_r2122232664)
In 69139049194e55a7fd99b7125b9afccb8585922d "test: Add test for diffrent signet data directories"

This test is not sufficient. All it checks is that the test framework has computed the expected datadir. It does not check that the directory even exists, or that the node is using that as the data directory.

I think this will need to read the debug log to see that it has the expected `Using data directory` line, but it would also probably be better if the node itself could report the current
...

📝 hebasto opened a pull request: "refactor: Drop unused `#include <boost/operators.hpp>`"
(https://github.com/bitcoin/bitcoin/pull/32668)
From https://api.cirrus-ci.com/v1/task/5082537556443136/logs/ci.log:
```
[10:04:20.418] /ci_container_base/src/node/mini_miner.cpp should remove these lines:
[10:04:20.418] - #include <boost/operators.hpp> // lines 8-8
```

💬 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_r2122243801)
Did I forget to explicitly mention the [`tuple`](https://www.boost.org/doc/libs/latest/libs/tuple/doc/html/tuple_users_guide.html) library?

See: https://github.com/bitcoin/bitcoin/blob/f999c3775c12ef732a1920dd52257a8cd34cdcc8/src/txrequest.cpp#L18