✅ fanquake closed an issue: "build: reproducibility issue with macOS Guix builds"
(https://github.com/bitcoin/bitcoin/issues/30815)

🚀 fanquake merged a pull request: "build: Use CMake's default permissions in macOS `deploy` target"
(https://github.com/bitcoin/bitcoin/pull/30838)

💬 hodlinator commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#issuecomment-2345733518)
https://github.com/bitcoin/bitcoin/pull/30328#pullrequestreview-2201704324 still applies to 705d9eb0aef8831891e1cce80c33615440547e90 instead of d5d994c02bb54db395da457724ec45539f1c10a8.

(Clarified linked comment a bit).

⚠️ fanquake opened an issue: "cmake: adjust Find modules to try `find_package` & fallback to `pkg_check_modules`"
(https://github.com/bitcoin/bitcoin/issues/30876)
See the discussion in: https://github.com/bitcoin/bitcoin/pull/30803#discussion_r1743507523.

> I assume one thing we could do, is try find_package(ZeroMQ) first, and fallback to pkg_check_modules(libzmq), and just maintain this (pretty much) forever. That might be better than somewhat vauge comments about "modern" distros, because, (some) modern distros do already support this, and it's actually the case that all versions of all distros Bitcoin Core supports need to do this, before we could d
...

💬 fanquake commented on pull request "build: Minor build system fixes and amendments":
(https://github.com/bitcoin/bitcoin/pull/30803#discussion_r1756492407)
Opened an issue for this #30876.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756495304)
Thanks, the diff looks good, but would you mind if you submitted it in a follow-up? I presume you ran it locally, and it didn't uncover any bugs in this pull request, so that is a useful signal already. However, at some point I want to wrap up with this pull request myself. There are already 150 comments, most of which are hidden by GitHub, and thus almost impossible to navigate.

If I included your diff, someone may suggest to "reduce verbosity" by using the compiler to construct the args of
...

🚀 fanquake merged a pull request: "build: Minor build system fixes and amendments"
(https://github.com/bitcoin/bitcoin/pull/30803)

🤔 l0rinc requested changes to a pull request: "util: Use consteval checked format string in FatalErrorf, LogConnectFailure"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2299632852)
It seems to me we still have a remaining trailing formatter bug that we should fix before pushing

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756421832)
Some tense changes could clarify the exact behavior:
```suggestion
decltype(fmt)::Detail_CheckNumFormatSpecifiers(fmt.fmt); // This was already executed at compile-time, but is executed again at run-time to avoid -Wunused.
```

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756422993)
What does `NormallyPassFmt<sizeof...(Args)> would be used.` mean exactly?

```suggestion
// args directly. NormallyPassFmt<sizeof...(Args)> would be used.
```

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756483366)
Thanks, updated the example

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756493521)
We could standardize this by extending `HasReason` in `setup_common.h`:
```C++
class HasReason
{
public:
explicit HasReason(const std::string_view& reason) : m_reason(reason) {}
bool operator()(std::string_view s) const { return s.find(m_reason) != std::string_view::npos; }
bool operator()(const std::exception& e) const { return (*this)(e.what()); }

private:
const std::string_view m_reason;
};
```
resulting in
```C++
BOOST_CHECK_EXCEPTION(util::ConstevalFormatStri
...

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756489714)
This seems like an error to me, shouldn't trailing `%` be invalid? It fails for `"%123 %"`, shouldn't it fail for these as well?
The reason for the misclassification is https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754908585, i.e. that when detecting a `%` we skip the `if (++it >= str.end())` check.

The example in https://github.com/bitcoin/bitcoin/pull/30546/files#r1754643534 detects this as an error

🤔 l0rinc requested changes to a pull request: "util: Use consteval checked format string in FatalErrorf, LogConnectFailure"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2299744441)
It seems to me we still have a remaining trailing formatter bug that we should fix before pushing

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756500154)
> Otherwise, it will never be ready because there is always a way to come up with an idea to improve something

I think we're getting closer, your patience is appreciated :)

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756503078)
Ah, it should say "Normally PassFmt<sizeof...(Args)> would be used." (in code)

💬 hodlinator commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756508500)
Yeah, I can do it as a follow-up since by now it does not really modify your existing code that much.

If you need to re-touch, I would appreciate if you just took this diff:
```diff
- PassFmt<129>("%129$s 999$s %2$s");
+ PassFmt<12>("%12$s 999$s %2$s");
```
to make the follow-up cleaner.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756514460)
Please take a look at the line above your comment. It says `tinyformat accepts almost any "type" spec, even '%', or '_', or '\n'`.

If you don't believe the comment, and the compile time check, you can try for yourself: https://godbolt.org/z/n6ToecPKT

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756518642)
I don't want to modify `HasReason` here. There are already more than 150 comments and increasing the scope won't help. Maybe in a follow-up, or separate pull?

💬 maflcko commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2345782247)
Needs rebase and CI failures fixed (they are true ones)