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

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1765076854)
Excellent, fixed.

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1765076974)
Converting flags to enums isn't the responsibility of this method, that's why I've extracted it.

💬 hodlinator commented on pull request "test: generalize HasReason and use it in FailFmtWithError":
(https://github.com/bitcoin/bitcoin/pull/30921#discussion_r1765097890)
Avoid extra allocations (from my [original diff](https://github.com/bitcoin/bitcoin/pull/30921#discussion_r1765022377)):
```suggestion
inline void FailFmtWithError(std::string_view wrong_fmt, const HasReason& error)
```

💬 1440000bytes commented on issue "Use IPv4-encoded IPv6 address to get IPv4 node address with port number from DNS seeds":
(https://github.com/bitcoin/bitcoin/issues/30900#issuecomment-2358536436)
> @1440000bytes As you can see by clicking through to the removal pull-req, the problem wasn't "insufficient nodes", it was failing to implement filtering properly: https://github.com/bitcoin/bitcoin/issues/29911

Sharing the PR rationale/description below with people who ACKed it:

```
This seeder no longer appears to be serving sufficient addresses.

Fixes https://github.com/bitcoin/bitcoin/issues/29911
```

```
ACKs for top commit:
1440000bytes:
ACK https://github.com/bitco
...

💬 l0rinc commented on pull request "test: generalize HasReason and use it in FailFmtWithError":
(https://github.com/bitcoin/bitcoin/pull/30921#discussion_r1765109845)
Seems I'm not that sensitive to these things for tests :)
Done, thanks