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

💬 maflcko commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1746614616)
nit in 0c02d4b2bdbc7a3fc3031a63b3b16bafa669d51c: Looks like this is a test-only option, like the previous one, so would it not make sense to mention this as well?

💬 maflcko commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1764961991)
style nit: When there is a local alias of a global, my preference is to use the local: `chainman.GetParams()`, over `Params()`, but up to you. (Same for FinalizeHeader)

💬 maflcko commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#discussion_r1764943665)
nit: Any reason to specify default values when they are not used?

💬 maflcko commented on pull request "test: add validation for gettxout RPC response":
(https://github.com/bitcoin/bitcoin/pull/30226#issuecomment-2358366997)
> 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 don't think `wallet_*.py` makes sense, because `gettxout` is not a wallet RPC. This would mean the test doesn't run at all unless a wallet is compiled in. The current place seems fine.

However, the CI fails.

💬 l0rinc commented on pull request "doc: Drop description of LogError messages as fatal":
(https://github.com/bitcoin/bitcoin/pull/30361#issuecomment-2358369107)
While I understand why @maflcko doesn't want to change twice, this seems to me like a slight improvement.

utACK b7aae361b273f2f439d3b278214b7e37908c8cb0