🤔 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

💬 l0rinc commented on pull request "doc: Drop description of LogError messages as fatal":
(https://github.com/bitcoin/bitcoin/pull/30361#discussion_r1764974888)
nit: *`instead of`* seems [slightly more common](https://www.italki.com/en/post/question-107941) than *`in place of`* + `that` to streamline readability + `needs to address` to imply urgency:
```suggestion
- `LogError(fmt, params...)` should be used instead of `LogInfo` for severe errors
that the node admin needs to address (e.g., failure to write data).
```

💬 l0rinc commented on pull request "doc: Drop description of LogError messages as fatal":
(https://github.com/bitcoin/bitcoin/pull/30361#discussion_r1764986353)
nit: the logger order is weird, why debug -> info -> error -> warning -> trace?
Especially after announcing `LogInfo`, `LogDebug`, `LogTrace`, `LogWarning` and `LogError` in the intro.

💬 maflcko commented on pull request "scripted-diff: Use LogInfo over LogPrintf":
(https://github.com/bitcoin/bitcoin/pull/29641#issuecomment-2358376988)
(This draft pull request isn't ready for review. The previous 5 comments are on stuff that will be deleted anyway.)

For now, please focus review on non-draft pull requests. There should be plenty right now.

💬 alfonsoromanz commented on pull request "test: add validation for gettxout RPC response":
(https://github.com/bitcoin/bitcoin/pull/30226#issuecomment-2358383566)
> 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.

Thanks. Yes I just noticed the failure on the lint job. I will check it and push the fix.

💬 maflcko commented on pull request "log: Use ConstevalFormatString":
(https://github.com/bitcoin/bitcoin/pull/30889#discussion_r1765000717)
> Would it make sense ... in a future PR?

I don't know and I don't think it matters much one way or the other.

> Got it, thanks.

Ok, resolving this for now.

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

It may or may not contain it. It doesn't matter either way. Also, I am not changing that (or any behavior). So a separate issue or pull request seems better.

💬 maflcko commented on pull request "test: add validation for gettxout RPC response":
(https://github.com/bitcoin/bitcoin/pull/30226#issuecomment-2358399897)
You'll probably have to rebase, as it may be a silent conflict.

💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1765029453)
Actually, I'm modifying the method in which case it's best practice to normalize it - how else will would we improve all these inconsistencies?!