💬 fanquake commented on pull request "cmake: add `USE_SOURCE_PERMISSIONS` to all `configure_file()` usage":
(https://github.com/bitcoin/bitcoin/pull/30823#issuecomment-2337575044)
> If https://github.com/bitcoin/bitcoin/pull/30838 happens to fix https://github.com/bitcoin/bitcoin/issues/30815, it seems reasonable to switch to NO_SOURCE_PERMISSIONS in all cases for consistency.

I'm going to merge this, and we can follow up in #30838, and decide if we to change all call sites, or just those needed for reproducibility. That PR will also need to be updated with an explantion of why this started happening, and the fix (I assume there should be no issue always using CMakes d
...

💬 fanquake commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2337609950)
> A quick glance on the ccache stats may indicate that ccache stopped working for some tasks.

Yea, seems like our ccache efficiency has tanked post-CMake. Is it possible this is somehow a stagnant cache or related issue (although given it's been 2 weeks since the switch, I'd think caches would have cycled)?.

ccache hits for some CI jobs of the last merge into master (https://github.com/bitcoin/bitcoin/runs/29789973072):

32-bit CentOS : `Hits: 1 / 715 (0.14 %)`
Win64 Cros
...

🚀 fanquake merged a pull request: "doc: fix minor typo"
(https://github.com/bitcoin/bitcoin/pull/30850)

💬 l0rinc commented on pull request "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1749932934)
Good idea, added `BytesToUInt8s` and `UInt8sToBytes`.

🚀 fanquake merged a pull request: "cmake: add `USE_SOURCE_PERMISSIONS` to all `configure_file()` usage"
(https://github.com/bitcoin/bitcoin/pull/30823)

👋 l0rinc's pull request is ready for review: "refactor: Generalize `CScript`'s `std::vector` push to accept `std::array`"
(https://github.com/bitcoin/bitcoin/pull/30765)

🤔 TheCharlatan reviewed a pull request: "refactor: migrate `bool GetCoin` to return `optional<Coin>`"
(https://github.com/bitcoin/bitcoin/pull/30849#pullrequestreview-2289326934)
Concept ACK

Splitting up the removal of the `Coin&` parameter into another commit felt to me like it mostly introduced churn, without making review easier. I ended up diffing over both commits to look at the change.

💬 TheCharlatan commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1749884905)
In commit ee75cc39bd8be72372c9b43f49a965225b737978:

Not changing the return types here will lead to compile failure.

💬 TheCharlatan commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1749896151)
In commit ee75cc39bd8be72372c9b43f49a965225b737978

There are places where you sometimes adjust the `&` spacing and where you leave it in its old style. I see that you adjust some of them in the following commits. Can you try to change them only on the first time you are changing a line?

👍 TheCharlatan approved a pull request: "cmake: decouple `FORTIFY_SOURCE` check from `Debug` build type"
(https://github.com/bitcoin/bitcoin/pull/30824#pullrequestreview-2289457160)
ACK 30803a35d54acda19ded88474c205f8954fea5e1

Tested by adding `-DAPPEND_CXXFLAGS="-O0"` to the build configuration.

💬 hebasto commented on issue "build: reproducibility issue with macOS Guix builds":
(https://github.com/bitcoin/bitcoin/issues/30815#issuecomment-2337686768)
> Is it something else from the context outside GUIX that leaks in? Umasks? File system differences?

@fanquake @TheCharlatan

Can you confirm filesystem permissions for a source directory within a Guix container on your systems?

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1749974868)
> nit1: you might as well delete the empty `(void)au256.GetHex();` a few lines below

I'm not sure if that would be an improvement, though. I think having a dedicated test on each method is nice and consistent, so I think even though it's redundant keeping both calls seems sensible to me to e.g. avoid accidentally removing coverage when `uint256::FromHex()` is removed?

> nit2: `value()` is redundant here

ah right, thanks, will update if I have to force-push.

💬 maflcko commented on pull request "cmake: decouple `FORTIFY_SOURCE` check from `Debug` build type":
(https://github.com/bitcoin/bitcoin/pull/30824#discussion_r1749985709)
Wouldn't it be simpler to upgrade the warning to an error with `-Werror=cpp`? This should avoid the need for any source code?

💬 fanquake commented on pull request "cmake: decouple `FORTIFY_SOURCE` check from `Debug` build type":
(https://github.com/bitcoin/bitcoin/pull/30824#discussion_r1749993071)
Can you elaborate, the issue is that one of the warnings is something we don't want to turn into an error, otherwise the check will fail incorrectly?

💬 hebasto commented on pull request "build: Minimize I/O operations in `GenerateHeaderFrom{Json,Raw}.cmake`":
(https://github.com/bitcoin/bitcoin/pull/30842#issuecomment-2337726320)
> > This PR aims to reduce the build time.
>
> Can you update the PR description to clarify that this is only relevant for native windows builds. Can you also add the explanation for why this only makes a difference on Windows.

I did my best.

💬 maflcko commented on pull request "cmake: decouple `FORTIFY_SOURCE` check from `Debug` build type":
(https://github.com/bitcoin/bitcoin/pull/30824#discussion_r1750013275)
Ah right, I missed that this requires GCC 12, or later. It could make sense to first detect that (https://github.com/bitcoin/bitcoin/pull/27027#issuecomment-1460007675), however I am not sure if this is worth it, given that GCC11 support may be dropped at some point in the future.

💬 fanquake commented on pull request "build: Minor build system fixes and amendments":
(https://github.com/bitcoin/bitcoin/pull/30803#discussion_r1750014895)
Discussed offline, and we will change the code to try both in a follow up.

💬 fanquake commented on pull request "cmake: decouple `FORTIFY_SOURCE` check from `Debug` build type":
(https://github.com/bitcoin/bitcoin/pull/30824#discussion_r1750025404)
Yea, I'm not sure that making this logic more complicated is worth it, to suppress (harmless) warnings, given that hardening also continues to work for GCC 11 & FORTIFY=3 (2).

💬 hebasto commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2337795647)
@maflcko

Can you clear all `ccache` caches so that we can observe the CI jobs under these conditions?

💬 hebasto commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2337808352)
> > A quick glance on the ccache stats may indicate that ccache stopped working for some tasks.
>
> Yea, seems like our ccache efficiency has tanked post-CMake.

I believe that https://cirrus-ci.com/build/5697885973512192 is more representative, as the change is docs-only.

As for Win64 Cross, the statistics shows `Hits: 120 / 713 (16.83%)`, which also looks questionable.