💬 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: `
...

💬 willcl-ark commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2337825652)
Can we not bump the ccache size on the (cirrus) workflows which we use self-hosted runners for? Currently these values are pretty low IMO, 100-400MB.

These ccaches IIUC are mounted volumes from the runner host itself, and don't need to be e.g. restored from a GitHub Actions cache, so do not need to be constrained the same as is best for those workflows.

Even at 1GB per cache, this would be 12GB total size, and so should be fine on an 80 or 160GB disk?

💬 maflcko commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2337836316)
About the cache issue, a better solution may be to set `CCACHE_READONLY` in pull requests.

However, the CI timeout issue seems to be of a different cause, because the runtime for the unit tests shouldn't fluctuate by 100%: https://cirrus-ci.com/task/4803595281891328?logs=ci#L8618 vs https://cirrus-ci.com/task/4981166594326528?logs=ci#L9169

💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1750068315)
@maflcko should I [use `LogInfo`](https://github.com/bitcoin/bitcoin/pull/29641/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R186) here instead?

💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1750074771)
> having a dedicated test on each method is nice and consistent

But it's not validating anything (except for severe exceptions), it's just an empty call to increase line coverage (i.e. kinda' fake confidence boost).
But in your case even the return value is validated, so there's no point in leaving the empty one.

💬 maflcko commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1750090426)
> @maflcko should I [use `LogInfo`](https://github.com/bitcoin/bitcoin/pull/29641/files#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R186) here instead?

It can't hurt, because it is a refactor. However, in some cases an alternative like `LogWarning`, `LogError`, `Assume` or `Assert` may be more useful. Also, it could make sense to explain why an error is unexpected and what should happen when they are encountered. Otherwise, users and developers wouldn't know what to
...

✅ fanquake closed an issue: "cmake: incorrect assumption that `Debug` build type will not use optimisations"
(https://github.com/bitcoin/bitcoin/issues/30800)