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

💬 l0rinc commented on pull request "scripted-diff: Use LogInfo over LogPrintf":
(https://github.com/bitcoin/bitcoin/pull/29641#discussion_r1764953272)
We should likely rename the file and class as well to match

💬 l0rinc commented on pull request "scripted-diff: Use LogInfo over LogPrintf":
(https://github.com/bitcoin/bitcoin/pull/29641#discussion_r1764957988)
this line seems weird now, defining `debug` as `Info`

💬 l0rinc commented on pull request "scripted-diff: Use LogInfo over LogPrintf":
(https://github.com/bitcoin/bitcoin/pull/29641#discussion_r1764954484)
does the custom lint name need an update now?

💬 alfonsoromanz commented on pull request "test: add validation for gettxout RPC response":
(https://github.com/bitcoin/bitcoin/pull/30226#issuecomment-2358346238)
Hey @maflcko, sorry for tagging you, but I would appreciate some additional feedback on this PR.

I believe `rpc_blockchain.py` is a suitable place to test the "gettxout" RPC call/response, as it already handles similar RPC commands like "gettxoutsetinfo." However, brunoerg suggested moving this test to `wallet_basic.py`.

I would value your input on this. I'm open to moving the test if that's the more appropriate approach.

🤔 maflcko reviewed a pull request: "fuzz: Test headers pre-sync through p2p"
(https://github.com/bitcoin/bitcoin/pull/30661#pullrequestreview-2285132178)
Nice.

Left some style nits. Feel free to ignore.

Is there a set of fuzz inputs, or a coverage report?