💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756557857)
Well, I promise to review it. If you don't believe yourself that it is going to be merged, then maybe it isn't worth it in the first place. Closing for now. This can be a follow-up, or separate pull.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756561525)
I'll leave this as-is for now. GitHub is already hiding the diff for me, so I'll close this for now. Discussion can continue in the follow-up, which will also make it easier to follow.

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756561875)
I understand that, but why are we making sure that the validation allows nonsensical values?

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756562372)
Ok, looking forward to the follow-ups

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756569850)
The goal is not to create a full POSIX-validator here, so there will always be an example where something is "invalid". The goal is to catch tinyformat-invalid *and* POSIX-invalid concerning the argument count.

Apart from that, I don't think it makes sense to spend a massive amount of time on format strings that are POSIX-invalid, but currently happen to be tinyformat-valid. No one should be using them, but if someone uses them it won't cause a runtime error but a harmless formatting error (o
...

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756570344)
Closing for now

🚀 fanquake merged a pull request: "test: Drop no longer needed workarounds"
(https://github.com/bitcoin/bitcoin/pull/30847)

🚀 fanquake merged a pull request: "security-check: test for `_FORTIFY_SOURCE` usage in release binaries"
(https://github.com/bitcoin/bitcoin/pull/27038)

🚀 fanquake merged a pull request: "build: Add more cmake presets"
(https://github.com/bitcoin/bitcoin/pull/30871)

💬 maflcko commented on pull request "ci: Print inner env, Make ccache config more flexible":
(https://github.com/bitcoin/bitcoin/pull/30869#discussion_r1756594931)
Thanks, adjusted commit message.

💬 maflcko commented on pull request "ci: Print inner env, Make ccache config more flexible":
(https://github.com/bitcoin/bitcoin/pull/30869#discussion_r1756595040)
Thanks, adjusted commit message.

💬 maflcko commented on pull request "ci: Print inner env, Make ccache config more flexible":
(https://github.com/bitcoin/bitcoin/pull/30869#discussion_r1756595333)
Yes.

I do not know. I presume this is just the initial value that was set when there were no debug builds or builds with sanitizers (which are larger).

💬 maflcko commented on pull request "ci: Print inner env, Make ccache config more flexible":
(https://github.com/bitcoin/bitcoin/pull/30869#discussion_r1756595452)
Nah, it was just copy-pasted from above. Fixed the style here.

💬 l0rinc commented on pull request "ci: Print inner env, Make ccache config more flexible":
(https://github.com/bitcoin/bitcoin/pull/30869#issuecomment-2345968106)
utACK 2222b44d35c092b5aaefed12bc5ca608e6e78760

💬 fanquake commented on pull request "test: remove unused src_dir param from run_tests after CMake migration":
(https://github.com/bitcoin/bitcoin/pull/30733#issuecomment-2345997775)
@davidgumberg fyi you've ack'd the previous commit has here.

🚀 fanquake merged a pull request: "test: remove unused src_dir param from run_tests after CMake migration"
(https://github.com/bitcoin/bitcoin/pull/30733)

💬 fanquake commented on pull request "build: drop obj/ subdirectory for generated build.h":
(https://github.com/bitcoin/bitcoin/pull/30856#issuecomment-2346016603)
Could you rebase this, now that guix builds are reproducible again.

💬 l0rinc commented on pull request "coins: remove logic for spent-and-FRESH cache entries and writing non-DIRTY entries":
(https://github.com/bitcoin/bitcoin/pull/30673#discussion_r1756663411)
It seems to me that production code never actually calls `AddFlags` with `0` value - and we're always setting `DIRTY`, even when `FRESH` is set.
So it would make sense to change `AddFlags` from having a custom `uint8_t flags` parameter which is always `DIRTY` (and sometimes `FRESH`) in reality (and is only called with `0` flag in tests) to have a freshness parameter like `inline void SetDirtyAndFresh(CoinsCachePair& self, CoinsCachePair& sentinel, bool is_fresh = false) noexcept` instead.
This
...

💬 hebasto commented on pull request "build: Skip secp256k1 ctime tests when tests are not being built":
(https://github.com/bitcoin/bitcoin/pull/30865#discussion_r1754576656)
> I think I'm missing something here. Isn't it a bug that us already setting `SECP256K1_BUILD_CTIME_TESTS=OFF` is sometimes overridden later on? Otherwise, this change is unclear, because the ctime tests (along with the other secp256k1 tests) are already supposedly controlled by `BUILD_TESTS`, but now the ctime tests are being wrapped in another `BUILD_TESTS` conditional? (adding a comment would be good, otherwise I assume anyone reading this code will assume the second conditional is redundant)
...

💬 willcl-ark commented on issue "Increasing self-hosted runner raw performance":
(https://github.com/bitcoin/bitcoin/issues/30852#issuecomment-2346027116)
> There are a bunch of places where `CCACHE_MAXSIZE` is hardcoded, making it impossible to set a larger size. This needs to be fixed first, so I did that in #30869 (among other stuff).

Awesome!

> Assuming .5GB of caches per task, 10 tasks per push, 100 pushes per day, and a cache duration of 20 days, means that 10TB of storage should be sufficient.

Not sure I understand this; the idea is to have "one ccache per task" surely, not one per push?

I was thinking we should set up with:

...