💬 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

💬 hodlinator commented on pull request "test: generalize HasReason and use it in FailFmtWithError":
(https://github.com/bitcoin/bitcoin/pull/30921#discussion_r1765022377)
Unless `HasReason` gets a new ctor that takes non-ref `string` which we can `std::move` into (and it can `move` from internally into the member), we are now allocating extra `string` copies on every `FailFmtWithError`-call.

I prefer going back to `string_view` here.

Yet another possible alternative is to construct `HasReason` copies outside and pass them in. Nice to memory bad slightly ugly using `HasReason` outside of the Boost macro.
```diff
diff --git a/src/test/util_string_tests.cpp
...

💬 hodlinator commented on pull request "test: generalize HasReason and use it in FailFmtWithError":
(https://github.com/bitcoin/bitcoin/pull/30921#discussion_r1765024172)
nit: Curly-maximalism.
```suggestion
BOOST_CHECK_EXCEPTION(util::ConstevalFormatString<WrongNumArgs>::Detail_CheckNumFormatSpecifiers(wrong_fmt), const char*, HasReason{error});
```

💬 andrewtoth commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1765039768)
Do we need a new function if we're only calling it the one time here? Just seems like unnecessary misdirection.

💬 l0rinc commented on pull request "test: generalize HasReason and use it in FailFmtWithError":
(https://github.com/bitcoin/bitcoin/pull/30921#discussion_r1765059519)
Nice, pushed!

💬 l0rinc commented on pull request "test: generalize HasReason and use it in FailFmtWithError":
(https://github.com/bitcoin/bitcoin/pull/30921#discussion_r1765059960)
Not needed after your previous suggestion.

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2358480289)
@glozow could we get a unit test for that specific behavior change :pray:

@marcofleon I presume it's simply things that "shouldn't happen":
0) Parent `MempoolRejectedTx`, put in reconsiderable filter
1) Child`MempoolRejectedTx`, put in orphanage
2) Child `MempoolRejectedTx` for other reason, now in reject filter
3) Parent `ReceivedTx`, found in reconsiderable filter, fetches package with fully rejected child

I'd feel better if we also simply not return a package we should never consid
...

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1765046861)
I think these checks on down can be pulled into `CheckPackageToValidate`?