💬 maflcko commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1764902520)
No. It seems fragile to fall back to tinyformat when a tinyformat error occurred. Even if unlikley, that just seems like it could lead to another tfm exception, rendering this whole code useless in the first place.

💬 maflcko commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1764903584)
A refactor is not allowed to change behavior. If you want to change behavior, a separate issue or pull request seems more appropriate.

📝 l0rinc opened a pull request: "test: generalize HasReason and use it in FailFmtWithError"
(https://github.com/bitcoin/bitcoin/pull/30921)
Standardized boost exception checking in recent tests introduced in https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1756493521 by extending `HasReason` to accept `const char*` through `string_view`.

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1764904714)
Pushed this change in https://github.com/bitcoin/bitcoin/pull/30921

💬 maflcko commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1764906085)
No

💬 maflcko commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1764908089)
Added "This refactor does not change behavior of the compiled executables." to the pull description and applied the label.

💬 l0rinc commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1764909660)
I understand (and somewhat share) the reasoning, but doesn't this mean that we don't trust `tfm::format` even in a fixed case?
E.g. isn't it possible after this call for `log_msg` to contain dangling format specifiers, which would be passed on to `LogInfo` anyway?

💬 maflcko commented on pull request "test: generalize HasReason and use it in FailFmtWithError":
(https://github.com/bitcoin/bitcoin/pull/30921#discussion_r1764913463)
Previously, in `FailFmtWithError` the full error was checked, now it checks that the error was found.

💬 maflcko commented on pull request "test: generalize HasReason and use it in FailFmtWithError":
(https://github.com/bitcoin/bitcoin/pull/30921#discussion_r1764912413)
Changing string to string_view here may introduce lifetimebound violations. Just seems like premature optimization with downsides and no benefit.

💬 l0rinc commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1764918069)
That's my concern as well, it seems to me we're changing behavior here by swapping a neutral `LogPrintf` to `Info` level logging, when in fact it's sometimes a `warning` or `error`.

💬 l0rinc commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1764918674)
So can we unify the two and use only one to avoid confusion?

💬 maflcko commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1764920307)
> doesn't this mean that we don't trust `tfm::format` even in a fixed case?

See https://github.com/bitcoin/bitcoin/pull/30889#issuecomment-2347330955, but this should never happen. So I don't think micro-optimizing this is worth it.

💬 l0rinc commented on pull request "test: generalize HasReason and use it in FailFmtWithError":
(https://github.com/bitcoin/bitcoin/pull/30921#discussion_r1764920916)
Yes

💬 l0rinc commented on pull request "test: generalize HasReason and use it in FailFmtWithError":
(https://github.com/bitcoin/bitcoin/pull/30921#discussion_r1764924022)
This is only used for tests, I don't think that's a real problem, but I've reverted it to be sure.

💬 maflcko commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1764925192)
I don't want to touch lines in this pull request that don't have to be touched. Also, I don't want to increase the scope here.

💬 maflcko commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1764929168)
`LogInfo` is neutral, because there is no level. See `GetLogPrefix`:

```
// If there is no category, Info is implied
if (!has_category && level == Level::Info) return {};
```

In any case, it is literally a macro alias:

```
// Deprecated unconditional logging.
#define LogPrintf(...) LogInfo(__VA_ARGS__)

💬 l0rinc commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1764932679)
Ok, what about the case when `fmterr.what()` contains formatting, like `%s`?

💬 l0rinc commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1764934401)
Got it, thanks.
Would it make sense to add an optional log-level parameter here to be able to set some of those logs to errors/warns in a future PR?

🤔 l0rinc requested changes to a pull request: "scripted-diff: Use LogInfo over LogPrintf"
(https://github.com/bitcoin/bitcoin/pull/29641#pullrequestreview-2312517789)
Concept ACK

💬 l0rinc commented on pull request "scripted-diff: Use LogInfo over LogPrintf":
(https://github.com/bitcoin/bitcoin/pull/29641#discussion_r1764956576)
Since the file contains `LogInfo` examples now, we should rename the file as well.