💬 ryanofsky commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754990024)
In commit "util: Add ConstevalFormatString" (fa512d7bd333c6bf5868fd131f9513bc51aab5aa)

I think other invalid but accepted case that would be good to check for here or above (depending on whether or not tiny format treats them as runtime errors) would be:

```
ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%123%");
ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%123%s");
```

💬 ryanofsky commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754940176)
In commit "util: Add ConstevalFormatString" (fa512d7bd333c6bf5868fd131f9513bc51aab5aa)

This comment is true, but potentially misleading because it isn't mentioning that this case will also be reached in cases this function isn't checking like when `#` `+` `-` or space flags are used, when any `.precision` value is specified, as well as in invalid unterminated format arguments like `"%0%s"` that may treated as valid.

Would suggest adding something like "This case is also reached if any flag
...

💬 ryanofsky commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755013263)
re: https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754821069

> They are not invalid, because in tinyformat there isn't an invalid specifier, see [#30546 (comment)](https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2333261303)

When you say these are not invalid you are saying tinyformat accepts `strprintf("hello %\n", "world")`? Or saying something else?

In any case, it would be good to clarify in unit test comments which of the test cases tinyformat supports and whi
...

💬 ryanofsky commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754971776)
In commit "util: Add ConstevalFormatString" (fa512d7bd333c6bf5868fd131f9513bc51aab5aa)

Think it could be a good idea to some tests positional argument combined with width or precision options or flags like:

```c++
ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%1$5i");
ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%1$-5i");
ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%1$.5i");
```

💬 ryanofsky commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755038937)
In commit "util: Add ConstevalFormatString" (fa512d7bd333c6bf5868fd131f9513bc51aab5aa)

Maybe replace "can" with "will"? It seems like there is no usage of * that ConstevalFormat would accept and tinyformat would not treat as a runtime error. Or is that not the case?

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755082651)
> This states: if the current char is not a %, ignore it - which wouldn't be the first thing I'd say ...

For counting the specifiers, anything before the specifier begins with `%` can safely be ignored. I'd explain it this way, but I can see how someone may want to explain it in another way.

Personally, I am fine either way, because this is just a style nit for me. I'll leave this as-is for now, but I may or may not change something if I have to push again.

💬 fanquake commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2344083999)
> What is your system and Ccache version?

That was on a macOS machine, with 4.10.2. Retested just now, and got the same: `Hits: 394 / 445 (88.54%)`.

💬 ismaelsadeeq commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#issuecomment-2344085293)
> The PR looks good in it's current form, but could consider improving documentation and simplifying the deleteRwSettings implementation with

Done in the latest force push see diff https://github.com/bitcoin/bitcoin/compare/adc019860d7df6848b98aec58f1c3ff47936e06b..84663291275248fd52da644b0c2566bbf9cc780b

🤔 pablomartin4btc reviewed a pull request: "docs: updated developer notes for --with-sanitizers to -DSANITIZERS"
(https://github.com/bitcoin/bitcoin/pull/30870#pullrequestreview-2297780864)
ACK 4b1ce3cac81497dd094e8c34550edf633f4d38ae

nit: if you have to retouch, commit title looks [too long](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches) and [prefix](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#creating-the-pull-request) is `doc` usually.

💬 fanquake commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2344102724)
Just tested the same on a Fedora box (ccache 4.10.2) and ended up with `Hits: 447 / 528 (84.66%)`.

💬 hebasto commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2344111410)
> Just tested the same on a Fedora box (ccache 4.10.2) and ended up with `Hits: 447 / 528 (84.66%)`.

What if you `ccache --zero-stats` after configuring for a new build tree, just before building?

On Fedora, `ccache` masquerades as a compiler, so all compilation during configuration can affect the final hit rate.

👍 ryanofsky approved a pull request: "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors"
(https://github.com/bitcoin/bitcoin/pull/30765#pullrequestreview-2297822100)
Code review ACK 5e190cd11f6ed8b7ab4db0f01192de63deaf6fd7. Looks good!

Since last review, just renamed push methods to append, and applied a few other suggested tweaks

💬 m3dwards commented on pull request "ci: parse TEST_RUNNER_EXTRA into an array":
(https://github.com/bitcoin/bitcoin/pull/30244#issuecomment-2344112521)
@maflcko no apology needed, apologies I introduced a bug!

I will work out what's going on and propose a fix PR ASAP.

Thanks for the logs.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755133612)
I think using positional args may work for `*`, but I haven't checked it.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755134264)
For tinyformat, almost any format specifier type is valid. `%` as well: https://godbolt.org/z/f8zWsP6nT

Thanks, added the tests and a comment!

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755135236)
Thanks, added the tests!

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755135557)
Thanks, added a comment!

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755136016)
Thanks, added a comment!

💬 ryanofsky commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1755134235)
In commit "chain: simplify `deleteRwSettings` code and improve it's doc" (84663291275248fd52da644b0c2566bbf9cc780b)

Might be good to say in commit message that this change is a refactoring and does not change behavior. Could have "refactor:" prefix in the title.

👍 ryanofsky approved a pull request: "interfaces: #30697 follow ups"
(https://github.com/bitcoin/bitcoin/pull/30828#pullrequestreview-2297841266)
Code review ACK 84663291275248fd52da644b0c2566bbf9cc780b. Looks good, thanks for taking suggestions and applying them to the right commits. Only changes since last review were documentation improvements and simplifying delete method.