💬 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)

🚀 fanquake merged a pull request: "cmake: decouple `FORTIFY_SOURCE` check from `Debug` build type"
(https://github.com/bitcoin/bitcoin/pull/30824)

💬 fanquake commented on pull request "build: Use CMake's default permissions in macOS `deploy` target":
(https://github.com/bitcoin/bitcoin/pull/30838#issuecomment-2337875369)
Concept ACK - looks like switching this to use CMakes default permissions, rather than ours, does fix the issue, but I guess could use more discussion given: https://github.com/bitcoin/bitcoin/issues/30815#issuecomment-2336699084.

💬 l0rinc commented on pull request "scripted-diff: Use LogInfo over LogPrintf":
(https://github.com/bitcoin/bitcoin/pull/29641#discussion_r1750101002)
Based on the message and
> - `LogError(fmt, params...)` should be used in place of `LogInfo` for
severe problems that require the node (or a subsystem) to shut down
entirely (e.g., insufficient storage space).

and the discussion in https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1750090426, this looks like a `LogError` instead (will throw in https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L895-L898 if `nullopt` is returned).

💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1750113102)
Thanks, used `LogError` in [`90aa839` (#30849)](https://github.com/bitcoin/bitcoin/pull/30849/commits/90aa839456163a48e675f601ff6a05330aa5c5c6)

⚠️ willcl-ark opened an issue: "Increasing self-hosted runner raw performance"
(https://github.com/bitcoin/bitcoin/issues/30852)
_disclaimer_: The following should **not** replace us investigating and fixing the root causes of timeouts and intermittent test runtime performance.

Now seems an opportune time to open a discussion on some investigation I have been doing into our self-hosted runners, as our CI has been struggling again recently.

I wanted to see what the cost/benefit implications would be on upgrading our self-hosted runners would look like. Hooking up a single [Hetzner AX52](https://www.hetzner.com/dedica
...

💬 l0rinc commented on pull request "Update libsecp256k1 subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/30845#issuecomment-2337973235)
Manually recopied https://github.com/bitcoin-core/secp256k1/commit/2f2ccc469540fde6495959cec061e95aab033148 into https://github.com/bitcoin/bitcoin/tree/ff54395de421fa1cbcf80ddbca4d71f0d2a4d12f/src/secp256k1 - seems to be done correctly, no changes detected by git.

ACK ff54395de421fa1cbcf80ddbca4d71f0d2a4d12f

💬 maflcko commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2337987820)
> each job run[s] on average 3-5x faster

Nice find. I didn't expect such a difference, but your results look promising to give this a try.

💬 maflcko commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1750175818)
Maybe I am missing something, but commit a2a6f320878ddbedfca856d512d862fb46df4b23 says:

```
> "LogError(fmt, params...) should be used ... for severe problems that require the node ... to shut down entirely".

Since this will throw a `TX_PREMATURE_SPEND` in `MemPoolAccept::PreChecks`, we'll use `LogError` instead.
```

`TX_PREMATURE_SPEND` may happen in normal operation (for example, it happens in the tests) and does not require the node to shut down entirely. Also, the node does not sh
...

💬 ryanofsky commented on issue "cmake: passing options from depends build to main build":
(https://github.com/bitcoin/bitcoin/issues/30813#issuecomment-2338011842)
> I'm still confused. Why should we "avoid writing the cache" for variables such as `WITH_ZMQ`? Using cache variables is a standard CMake method that allows the user to set their values.

Sorry that was not clear. I should have said that "force writing the cache" and overwriting values set by users should be avoided. Ideal behavior is that user can run depends build and it will set default cache values. Then they can run `ccmake` or plain `cmake` and change settings freely and those values wil
...