💬 l0rinc commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1755550025)
It seemed to me that `SOURCE` is also unused here, see: https://github.com/bitcoin/bitcoin/commit/031ac7f366263bacab152aa94a51025c582b1191#diff-d7f6f763fed50046f1c8a5dcca088fabb6b8c3662a55516f08fc0d0ba24829e3L45

✅ fanquake closed a pull request: "curl -sSL "https://mempool.space/api/mempool/recent""
(https://github.com/bitcoin/bitcoin/pull/30874)

📝 fanquake locked a pull request: "curl -sSL "https://mempool.space/api/mempool/recent""
(https://github.com/bitcoin/bitcoin/pull/30874)

```python
[
{
txid: "4b93c138293a7e3dfea6f0a63d944890b5ba571b03cc22d8c66995535e90dce8",
fee: 18277,
vsize: 2585,
value: 4972029
},
...
]
```




<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear mot
...

🤔 hodlinator reviewed a pull request: "util: Use consteval checked format string in FatalErrorf, LogConnectFailure"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2298052654)
Reviewed fa26462e95291652b4021d91b014655f678149e8

`git range-diff master fa7819b fa26462`

- Moved independent commit to the beginning, thanks!
- Added some comments.

I really like that this is being worked upon and want to help us find the best trade-offs in settling on a solution.

💬 hodlinator commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755206069)
> but that's not what this does, that's below this condition.

Ah, yes, mixed them up. Still feels okay to ignore anything up to a '%' that way.

But I do also like l0rinc's approach in this thread https://github.com/bitcoin/bitcoin/pull/30546/files#r1754649705, although extra state tends to create room for more bugs.

💬 hodlinator commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755352516)
Duplicates

💬 hodlinator commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755589971)
#### A) Interlacing `tfm::format`

Partially in response to @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755013263)...

> Potentially we could even extend the test to compare tinyformat behavior with ConstevalFormat behavior to make things even clearer, but just having a comment to clarify how these cases are treated by tinyformat would be very helpful.

...and partially out of my own curiosity about how `tfm::format` really behaves - I added `tfm::form
...

💬 maflcko commented on pull request "http: Use 'starts_with' for matching URI prefix":
(https://github.com/bitcoin/bitcoin/pull/30868#issuecomment-2344697879)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

💬 hodlinator commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1755611792)
> Pushing has a specific meaning in the context of Bitcoin Script (it's an opcode that pushes data onto the stack), and that's not what's happening here so that would be very confusing.

The new pair of *private* functions together implement `OP_PUSHDATA`, and are *only used* in that context, that's the reason I recommended the naming.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755612640)
Thanks, I'll fix it up in the next follow-up, or if I have to re-push here, to avoid invalidating review over a harmless test style-nit.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755647995)
> While I agree with maflcko that [keeping functionality hard to use outside of the intended context](https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1752494409) has value, keeping tests straightforward is more important. If you prefer...

Thank you for the diff. However, I think that real code should be designed without taking tests into account (apart from being testable). Tests should always follow real code, not the other way round. I am sure the verbosity in the test can be redu
...

👍 TheCharlatan approved a pull request: "build: Add more cmake presets"
(https://github.com/bitcoin/bitcoin/pull/30871#pullrequestreview-2298717809)
ACK f15e817811e328423ea489870ead089128a6ef8c

👍 hodlinator approved a pull request: "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors"
(https://github.com/bitcoin/bitcoin/pull/30765#pullrequestreview-2298723649)
re-ACK 5e190cd11f6ed8b7ab4db0f01192de63deaf6fd7

`git range-diff master e6a5ab7 5e190cd`

- [Disagree](https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1755611792) with `AppendData`/`AppendDataSize` names over `PushData`/`PushDataSize`, but will let it go.
- Nice switch back from pointer arithmetic to `span` in `AppendData`.
- Touched up commit messages per my latest review, cheers!
- Implements legacy `operator<<` in terms of the other. I agree with l0rinc's reservations against
...

💬 hodlinator commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755702207)
> I think that real code should be designed without taking tests into account (apart from being testable). Tests should always follow real code, not the other way round.

Making the function return a value allowed for C) dropped negatives (and less runtime testing). But I can try to provide a new diff bringing them back and without changing *string.h*. My main point remains adding the interlaced tinyformat tests to prove parity.

> The diff you shared also changes `util::hex_literals::detail
...

💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1755723676)
Might be clearer like this:
```suggestion
{"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "You may pass in the absolute path of your wallet directory (or .dat file for legacy wallets). Otherwise, the interpreted path will be relative to the default wallet directory."},
```

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755737721)
> > The diff you shared also changes `util::hex_literals::detail::Hex`, which I don't really understand.
>
> It's a consequence of clashes between `util::detail` namespaces. (Added note about that in a later edit).

Thanks for explaining. I think this is another reason to keep `string.h` as-is for now.

> My main point remains adding the interlaced tinyformat tests to prove parity.

I think the `CheckTooMany` may be a bit too verbose and mostly unit tests for tinyformat itself. Especial
...

💬 maflcko commented on pull request "Lint: improve subtree exclusion":
(https://github.com/bitcoin/bitcoin/pull/29965#issuecomment-2344790531)
Are you still working on this?

👍 hodlinator approved a pull request: "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage"
(https://github.com/bitcoin/bitcoin/pull/30618#pullrequestreview-2298831735)
ACK 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa

Provides better error messages for tests.

nit: 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa - "Compare FromUserHex result against other hex validators and parsers" is not dependent on the other changes, so could be moved first instead of last if you re-touch.

💬 kravens commented on issue "GUI event loop should be block free":
(https://github.com/bitcoin/bitcoin/issues/17145#issuecomment-2344798265)
Screen recording: [BitcoinCore-screen0.webm](https://github.com/user-attachments/assets/db653667-6b83-4089-b85e-69d84bcebede)
I'm testing 28.X release candidate branch, is this behaviour due to these lock-ups in the GUI code?

💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1755770730)
Same re: `HaveCoin`