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

💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1765009849)
Nice, added a ternary to simplify further, thanks!

💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1765029874)
Sir, yes sir!

💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1765031163)
removed the `else`

💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1765031560)
```suggestion
inline void SetClean() noexcept
```

👍 instagibbs approved a pull request: "fuzz: Add check in `p2p_headers_presync` that chain work never exceeds minimum work"
(https://github.com/bitcoin/bitcoin/pull/30918#pullrequestreview-2312643576)
ACK 7b85fd1443488afe60235e4c33bd956cb9c8d562

Thanks for the quick follow-up on suggestion! Makes the test significantly easier to read.

💬 instagibbs commented on pull request "fuzz: Add check in `p2p_headers_presync` that chain work never exceeds minimum work":
(https://github.com/bitcoin/bitcoin/pull/30918#discussion_r1765029395)
IIUC this would be a conservative overestimate, since the compact block and block messages don't move `base` forward?

If so, probably should note that here.

🤔 hodlinator requested changes to a pull request: "test: generalize HasReason and use it in FailFmtWithError"
(https://github.com/bitcoin/bitcoin/pull/30921#pullrequestreview-2312632081)
Concept ACK