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

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1740942391)
I had the same intuition, but kept them to be safe. Looking at it again, I agree with your view that they can be safely removed, so I've added a new commit 73a126f4f59470d839905d0eaaa26f689f7f2ba1 to do that and clean up the `arith_uint256S()` function entirely. Thanks!

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1740942976)
Resolved by [removing the function altogether](https://github.com/bitcoin/bitcoin/commit/73a126f4f59470d839905d0eaaa26f689f7f2ba1).

💬 vasild commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-2324802375)
`e0bb306436...f5fe172e85`: rebase due to conflicts,

@jonatack there you go!

🤔 furszy reviewed a pull request: "rpc: add getdescriptoractivity"
(https://github.com/bitcoin/bitcoin/pull/30708#pullrequestreview-2275714079)
> How slow is this actually, though? Some kind of bounded cache would probably fix this pretty easily if it wound up taking non-negligible time relative to the other operations for these calls, which I would expect to swamp a non-IO computation like descriptor derivation.

That was also my initial expectation but then faced stuff like https://github.com/bitcoin/bitcoin/issues/30632#issuecomment-2310966130, which takes ~4 seconds to parse + derive. And it could get worse if the user adds some t
...

👍 fanquake approved a pull request: "doc: add `-DWITH_BDB=ON` to unix build docs"
(https://github.com/bitcoin/bitcoin/pull/30779#pullrequestreview-2275722547)
ACK ddef914bbb1e4fe87e8a85f17e4e839639fd97da

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1740962468)
Well, changing the test to periodically flush after 55 minutes makes it a different test. That would mean the first flush is in the window and could randomly trigger a flush or not.
Was the failure after changing to `55min`?

💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#discussion_r1740966394)
Why would we need to test every value in the window? Testing the edges of the window accomplishes the same thing, no?