💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754643534)
Must be a compiler difference, you can use `if ('0' <= *it && *it <= '9')` instead:
```C++
constexpr static void Detail_CheckNumFormatSpecifiers(std::string_view str)
{
unsigned count{0};
unsigned maybe_num{0};
bool inside_format_specifier = false;
bool has_normal = false, has_positional = false;
for (auto it = str.begin(); it < str.end(); ++it) {
if (*it == '%') inside_format_specifier = !inside_format_specifier;
else if (inside_format_specifier) {
...

💬 maflcko commented on pull request "docs: updated developer notes for --with-sanitizers to -DSANITIZERS":
(https://github.com/bitcoin/bitcoin/pull/30870#discussion_r1754645983)
unrelated nit in the same section: I think ` * [Issue #12691: Enable -fsanitize flags in Travis](https://github.com/bitcoin/bitcoin/issues/12691)` can be removed. Not sure why it would be important or relevant to read today.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754654470)
If you want to prohibit them, a separate pull request may better suited to remove and prohibit them.

💬 kevkevinpal commented on pull request "docs: updated developer notes for --with-sanitizers to -DSANITIZERS":
(https://github.com/bitcoin/bitcoin/pull/30870#discussion_r1754658100)
agreed, I remove it in this commit [12f9d33](https://github.com/bitcoin/bitcoin/pull/30870/commits/12f9d336108f78ad7a0c711085de12937aca2e73)

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754662640)
Absolutely, but we may not want to spend time implementing positional parsing, if we agree that we don't want to encourage their usage

💬 maflcko commented on pull request "docs: updated developer notes for --with-sanitizers to -DSANITIZERS":
(https://github.com/bitcoin/bitcoin/pull/30870#issuecomment-2343771189)
I think a 4-line doc fixup can be a single commit. Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754674269)
Personally I think it can be useful (`std::format` also allows them), but I don't really care. I am happy to put this pull into a draft while it is waiting on the other issue/pull. However, I don't think this pull is the right place to make such a change.

🤔 ryanofsky reviewed a pull request: "interfaces: #30697 follow ups"
(https://github.com/bitcoin/bitcoin/pull/30828#pullrequestreview-2297231104)
Code review ACK adc019860d7df6848b98aec58f1c3ff47936e06b. These are nice, well-organized improvements to the settings interface to make it more consistent, remove ability to write null values to settings.json, and add ability to atomically read and delete settings.

I think not allowing null values in `settings.json` is good change to encourage clearer representations of settings, maintain parity between settings.json / bitcoin.conf / command line settings representations, avoid making ArgsMan
...

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754696512)
I don't want to delay the PR, just to clarify if it makes sense to work on this feature

💬 maflcko commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1754702526)
In commit "build: Improve ccache performance for different build directories" (https://github.com/bitcoin/bitcoin/commit/66b73bbf375ff96bf4a77539c9cf3fe52fbc15d5)

Now that base_dir is set by the build system, would it be fine to remove this from the prod notes?

```
$ git grep --line-number base_dir doc/productivity.md
doc/productivity.md:42:base_dir = /home/yourname # or wherever you keep your source files
doc/productivity.md:45:Note: base_dir is required for ccache to share cached co
...

💬 kevkevinpal commented on pull request "docs: updated developer notes for --with-sanitizers to -DSANITIZERS":
(https://github.com/bitcoin/bitcoin/pull/30870#issuecomment-2343804392)
> I think a 4-line doc fixup can be a single commit. Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

no problem squashed into [121dd14](https://github.com/bitcoin/bitcoin/pull/30870/commits/121dd14db40fa6bb6263fdea54d2f6be7b1f238e)

💬 maflcko commented on pull request "docs: updated developer notes for --with-sanitizers to -DSANITIZERS":
(https://github.com/bitcoin/bitcoin/pull/30870#issuecomment-2343809372)
review ACK 121dd14db40fa6bb6263fdea54d2f6be7b1f238e

👍 ryanofsky 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-2297250030)
Code review ACK 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa. Only changes since last review view were auto type and fuzz test tweaks.

💬 ryanofsky commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1754703576)
re: https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1754600019

> if you insist

I don't insist. Definitely follow your own judgement, I'm just throwing out suggestions.

💬 hodlinator commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754713518)
These lines (except maybe `%5.2f`?) should be moved to their own `static_assert` with a comment admitting they are invalid, but currently not caught (unlike the `static_assert` below which is valid but not properly handled).

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

`git range-diff master faa32ad fa7819b`

- Removed validation of `aAcdfeEfFgGinopsuxX` as tinyformat does not implement it either - acceptable.
- Implemented support for width/padding-specification.
- Now applies to `LogConnectFailure`.
- Adds commit fafe6313cce2b97d86bd6f907ffe93c98cf9cb59 which deduplicates lints.

#### fafe6313cce2b97d86bd6f907ffe93c98cf9cb59 - lint: Remove forbidden functions from lint-format-strings.py

Could mov
...

💬 hodlinator commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754649705)
Could be simply `for (char c : str)`.

Also the [coding style](https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c) only permits omitting braces on `if` when it isn't followed by `else`.

💬 rkrux commented on issue "28.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/30854#issuecomment-2343833900)
@fjahr Thanks, I'll add it.

💬 kevkevinpal commented on pull request "docs: updated developer notes for --with-sanitizers to -DSANITIZERS":
(https://github.com/bitcoin/bitcoin/pull/30870#issuecomment-2343845982)
sorry had a lint issue in commit

fixed lint issue in [4b1ce3c](https://github.com/bitcoin/bitcoin/pull/30870/commits/4b1ce3cac81497dd094e8c34550edf633f4d38ae)

📝 sipa opened a pull request: "Add more cmake presets"
(https://github.com/bitcoin/bitcoin/pull/30871)
Add three more cmake presets to the project-wide `CMakePresets.json` file:
* `dev-mode`: enables all features and dependencies
* `libfuzzer`: builds for fuzzing with libfuzzer and the typical sanitizers (but not the optional ones that require suppressions) enabled.
* `libfuzzer-nosan`: builds for fuzzing with libfuzzer and no (other) sanitizers

And then uses these in some documentation, as well as in CI.