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

💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1750050508)
Indeed, fixed

💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1750050653)
thanks, fixed in https://github.com/bitcoin/bitcoin/compare/cc78ead1e22937fa32f2dde372e64e4e16734ecb..b53de0d5e39048352004522314f6962b5aa3ae70

💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#issuecomment-2337811187)
> I ended up diffing over both commits to look at the change.

Thanks, merged the last two commits

💬 maflcko commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2337816419)
The CI machines may work on pull requests, which may (in)validate the cache, so ccache stats may generally not be 100% reliable.

It is possible to trigger a run with a clean cache volume locally (by deleting all volumes), or on CI, by creating a new namespace for the volumes:

```diff
diff --git a/ci/test/02_run_container.sh b/ci/test/02_run_container.sh
index afd447c347..53c0b4d8bc 100755
--- a/ci/test/02_run_container.sh
+++ b/ci/test/02_run_container.sh
@@ -25,15 +25,15 @@ if [ -z "
...

💬 fanquake commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2337823266)
> I believe that https://cirrus-ci.com/build/5697885973512192 is more representative, as the change is docs-only.

Possibly, but this still looks all over the place:

32-bit CENTOS ` Hits: 0 / 715 (0.00 %)`
arm: ` Hits: 3 / 713 ( 0.42%)`
Win64 cross: ` Hits: 120 / 713 (16.83%)`
mac cross: ` Hits: 144 / 726 (19.83%)`
TSAN ` Hits: 165 / 724 (22.79%)`
No wallet, kernel: ` Hits: 307 / 695 (44.17%)`
MSAN: `
...