💬 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.

💬 ryanofsky commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1755133114)
In commit "chain: dont check for null settings value in `overwriteRwSetting`" (f8d91f49c7091102138fcb123850399e8fadfbc7)

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.

💬 ismaelsadeeq commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1755152446)
thanks will do if there is need to retouch.

💬 ismaelsadeeq commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#discussion_r1755154932)
I will do it when their is need to retouch.