👍 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-2296826968)
Code review ACK 6da47f6ba5450e7f2b2ab2a64c7cfd354f67a1a9. Nice change to make tests easier to read and setup_common code better organized

💬 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_r1754374879)
In commit "Use BOOST_CHECK_EQUAL for optional, arith_uint256, uint256, uint160" (57c1f9fb9580456f98c239e1a8aa9161c7014fc6)

In general I like using `auto` to avoid verbosity and repetition when types are obvious. But in this case I think using `std::string` makes code easier to read because `"0x123..."` literals in our codebase are more commonly used to create different types like c strings, arrays, vectors, and blobs. Using `auto` is less clear because it makes `std::string` type a surprise u
...

💬 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_r1754442046)
In commit "Compare FromUserHex result against other hex validators and parsers" (6da47f6ba5450e7f2b2ab2a64c7cfd354f67a1a9)

The three lines removed here are not equivalent to the similar lines added below because these are checking `random_hex_string`, while lines below are checking `result->ToString()`, these strings are not the same because `random_hex_string` can have mixed case. Would suggest not dropping these lines.

💬 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_r1754400746)
In commit "Use BOOST_CHECK_EQUAL for optional, arith_uint256, uint256, uint160" (57c1f9fb9580456f98c239e1a8aa9161c7014fc6)

Here and below I also think `const std::string` would be more helpful than `auto`

💬 dergoegge commented on pull request "fuzz: Test headers pre-sync through p2p":
(https://github.com/bitcoin/bitcoin/pull/30661#issuecomment-2343642089)
@brunoerg @maflcko review ping :)

👍 brunoerg approved a pull request: "fuzz: Test headers pre-sync through p2p"
(https://github.com/bitcoin/bitcoin/pull/30661#pullrequestreview-2296997010)
ACK a97f43d63a6e835bae20b0bc5d536df98f55d8a0

📝 kevkevinpal opened a pull request: "docs: updated developer notes for --with-sanitizers to -DSANITIZERS"
(https://github.com/bitcoin/bitcoin/pull/30870)
In the developer notes we are incorrectly using the Autotools `--with-sanitizers` configure flag which we should now be using `cmake -B build -DSANITIZERS=<values>` instead now

🤔 l0rinc requested changes to a pull request: "util: Use consteval checked format string in FatalErrorf, LogConnectFailure"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2296466018)
Thanks for your patience and for striving to eliminate developer frustration.
I like the concept a lot, but have some preferences regarding parser intuitiveness, hope you'll consider my suggestions.

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754093556)
nit: personally I'd prohibit positional args completely, they can be super confusing - and shouldn't be needed, we can extract variables before the call if we need to repeat them, instead of the reader being forced to count parameters... but if we do, I have a few suggestions

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754518405)
While it gets the job done, I think the parser is unnecessarily complicated for several reasons:
* Redundant Counters: Separate (but mutually exclusive) `count_normal` and `count_pos`.
* Repetition: The check for "Format specifier incorrectly terminated by end of string" is repeated in multiple places.
* Inefficient Character Handling: Multiple character checks for `%` (and manual digit detection).
* Scattered Iterations: Increment operations (++it), continue statements, and lookaheads (str.
...

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754542178)
I don't particularly like that we have lookaheads even though we should be able to detect this at the very end - which would enable us to remove the duplicated throw.

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754545143)
I find these jumps quite confusing, it doesn't reflect how I would manually read and interpret such a format string

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754239951)
👍, "indicate" may not be the best choice in this context, how about:
```suggestion
if (maybe_num == 0) throw "Positional format specifiers start at 1"};
```

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754535527)
Alternatively, we could cheaply collect the referenced indexes to a `std::set<unsigned> referenced_params;` and check that all of them were referenced, i.e. `if (referenced_params.size() != num_params) throw "Not all parameters are referenced!";`

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754284187)
since we can only have either `count_normal` or `count_pos`, but we also need to detect if both were called, could we separate the two concerns: have a single count which is used in both cases and two booleans, a "has_normal" and a "has_positional"?

💬 stickies-v 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_r1754555353)
This change was made because of [my suggestion](https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1742175853) but you're right there can be a case difference which I did not consider. Sorry for that @l0rinc .

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

(Side note: `count_format_specifiers` also does not implement `*` parsing correctly, which I guess was never detected because it was never used)

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

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

💬 hebasto commented on pull request "test: Drop no longer needed workarounds":
(https://github.com/bitcoin/bitcoin/pull/30847#issuecomment-2343712471)
cc @maflcko