💬 maflcko commented on issue "cmake: Step "-- Looking for C++ include boost/test/included/unit_test.hpp" takes a long time":
(https://github.com/bitcoin/bitcoin/issues/30787#issuecomment-2324729606)
> Where in Autotools were we running checks to verify the existence of singlular boost headers at configure time?

I haven't confirmed this, but I assumed it checked for `boost/version.hpp` existence (and version conformance). Though, maybe I am wrong. In any case, anything is fine by me. I just found that it would be nice if calling `rm -rf ./bld-cmake/ && cmake -B ./bld-cmake` was fast, so all developers can just call it every time, without having to think about it much.

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1740907250)
but we're never writing optionals on the right side here, i.e.
```C++
BOOST_CHECK_EQUAL(uint256::FromUserHex(""), std::make_optional(uint256::ZERO));
```
can simply be either:
```C++
BOOST_CHECK_EQUAL(uint256::FromUserHex(""), uint256::ZERO);
```
or if empty:
```C++
BOOST_CHECK(!uint256::FromUserHex(""));
```

📝 jadijadi opened a pull request: "test: fixing failing system_tests/run_command under some Locales"
(https://github.com/bitcoin/bitcoin/pull/30788)
the run_command test under system_tests fails if the locale is anything other than US/UK/C (English ones)
because it checks for things like `result.find_value("success");`

To prevent this, a `setenv("LC_ALL", "C", 1);` is added to make sure that the tests are being run under C locale even if something else is set in the terminal.

fixes #30608

<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed

...

💬 marcofleon commented on pull request "doc: fix compiler flags for macOS configuration":
(https://github.com/bitcoin/bitcoin/pull/30785#discussion_r1740908329)
The word "confgure" is still used in all the build docs to describe that step (not just mac), so I'll leave that as is. Maybe another PR can reword the descriptions if we want to do that.

💬 l0rinc commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1740911993)
Removed the `nullopt` overwrite and explained the usage in the comment, thanks for explaining your view.

💬 maflcko commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2324740342)
> if the header can only be generated using a Python script, then that's an argument for including it in the source code.

It is possible to generate it in native cmake as well. See for example `cmake/script/GenerateHeaderFromJson.cmake`. Note that previously the "cmake" generation used `xxd`, which isn't a build-requirement either.

👍 brunoerg approved a pull request: "doc: fix compiler flags for macOS configuration"
(https://github.com/bitcoin/bitcoin/pull/30785#pullrequestreview-2275643738)
ACK 8d7f8fabae252710e8e43bf18a0c33ec9f8a7137

💬 maflcko commented on pull request "doc: fix compiler flags for macOS configuration":
(https://github.com/bitcoin/bitcoin/pull/30785#issuecomment-2324742043)
review ACK 8d7f8fabae252710e8e43bf18a0c33ec9f8a7137

💬 vasild commented on issue "CMake `CheckPIESupported` doesn't always work":
(https://github.com/bitcoin/bitcoin/issues/30771#issuecomment-2324748131)
To summarize this: on FreeBSD, but not on Ubuntu and not on Fedora, if the user injects `-fprofile-instr-generate -fcoverage-mapping` into the compilation/link flags, then CMake wrongly considers that `-fPIE` is not supported. Compilation does not fail. CMake considers that `-fPIE` is not supported because it compiles the program without PIE and tries to link with PIE, which on FreeBSD only and only with `-fprofile-instr-generate -fcoverage-mapping` fails to link (so the test prog fails to link,
...

💬 Eunovo commented on pull request "Tr partial descriptors":
(https://github.com/bitcoin/bitcoin/pull/30243#issuecomment-2324750876)
- Rebased on top of master https://github.com/bitcoin/bitcoin/commit/a865494deeff7dedcad7140299aee00ab3cdd62c
- Added a test for rawnode with multipath descriptors in https://github.com/bitcoin/bitcoin/pull/30243/commits/133b30019c139a3a0e624df2015f65c64040c0c8

💬 maflcko commented on pull request "test: fixing failing system_tests/run_command under some Locales":
(https://github.com/bitcoin/bitcoin/pull/30788#issuecomment-2324757387)
Seems fine as a fix, but it seems incomplete, because the test may still fail if the file exists.

The test only wants to check the stderr and exit code, so instead of using `ls`, it may be easier to use a python command (https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1740457415), or something similar.

💬 fanquake commented on issue "cmake: Step "-- Looking for C++ include boost/test/included/unit_test.hpp" takes a long time":
(https://github.com/bitcoin/bitcoin/issues/30787#issuecomment-2324762342)
> I haven't confirmed this, but I assumed it checked for boost/version.hpp existence (and version conformance).

We did do a high-level check for the existence of a Boost installation, and sufficient version (and that is what is also done in CMake now), but not compilation checks for individual headers (otherwise we should add checks for all Boost headers that we might happen to `#include`, i.e `boost/multi_index/*`, if we think they might be missing for some reason).

I think either making
...

💬 fanquake commented on issue "CMake `CheckPIESupported` doesn't always work":
(https://github.com/bitcoin/bitcoin/issues/30771#issuecomment-2324763862)
> IMO this is low impact and low severity.

Randomly missing (hardening) flags isn't that great. The fact that compilation still succeds (is there a warning the check failed?), also means the user is also unaware of the deficiency. We've identified one time this happens, but there may also be others. Given the ambiguity, it may also just be easier to return to the previous behaviour of just always using the flags, rather than letting CMake guess, and sometimes get it wrong.


Is there an i
...

💬 maflcko commented on pull request "test: fixing failing system_tests/run_command under some Locales":
(https://github.com/bitcoin/bitcoin/pull/30788#issuecomment-2324767911)
Also, the commit message is wrong `echo success` is never translated, because `echo` is just a tool to echo something, not to translate a passed string.

💬 mzumsande commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1740685331)
nit: missing "can" (before or after "currently")

🤔 mzumsande reviewed a pull request: "assumeutxo: Add dumptxoutset height param, remove shell scripts"
(https://github.com/bitcoin/bitcoin/pull/29553#pullrequestreview-2275266675)
ACK 94b0adcc371540732453d70309c4083d4bd9cd6b

It would be good to fix the `networkactive` issue below, here or in a follow-up.

💬 mzumsande commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1740765568)
This should be conditional on networkactive being true - I don't think this should re-enable network activity if it was previously disabled by a user-specified `setnetworkactive false`, as it currently would.

💬 Sjors commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#issuecomment-2324779552)
The current version of `cmake/script/GenerateHeaderFromJson.cmake` in this PR uses `asmap-tool.py`. Is there a way to do this without relying either on Python _or_ xxd?

🚀 fanquake merged a pull request: "doc: fix compiler flags for macOS configuration"
(https://github.com/bitcoin/bitcoin/pull/30785)

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#issuecomment-2324784988)
Force-pushed to add commit 73a126f4f59470d839905d0eaaa26f689f7f2ba1 to remove the test-only `arith_uint256S()` function entirely. Since `arith_uint256` does not have any string string constructors, it uses `uint256` constructors, and those are already unit tested. In some cases, a string constructor can be avoid entirely, e.g. by using the `arith_uint256` `uint64_t` constructor.

Addresses @l0rinc's review comment about [unnecessary tests](https://github.com/bitcoin/bitcoin/pull/30773#discussi
...