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

🤔 ismaelsadeeq reviewed a pull request: "Add more cmake presets"
(https://github.com/bitcoin/bitcoin/pull/30871#pullrequestreview-2297364109)
Concept ACK

💬 instagibbs commented on pull request "Add more cmake presets":
(https://github.com/bitcoin/bitcoin/pull/30871#discussion_r1754795921)
-nosan option imo is worth noting directly here

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

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754821069)
They are not invalid, because in tinyformat there isn't an invalid specifier, see https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2333261303

Also, stuff like `%.2fs` is pretty standard and valid floating point number formatting. See for example `src/logging/timer.h`

💬 ryanofsky commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754827231)
I think positional args are more readable than repeated args (even in this example which is atypically long) because:

- People have limited working memories. it is a easier to see 4 parameters passed and remember what numbers 1-4 are than to see 6 parameters passed and remember what 1-6 are. Remembering a list is also harder if the list has duplicate values.
- Positional arguments are more explicit. "Configuration file %3$s" it is clearer than "Configuration file %s" because it tells you tha
...

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2343920319)
I reread my suggestions from https://github.com/bitcoin/bitcoin/pull/30043#pullrequestreview-2141814858, from those I see the following important enough that I work on them in a followup (but even better if they are addressed in this PR):

https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1658764974 avoid unreachable nets (not given to `-onlynet=`)

https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1658949236 could announce an addr:port where we do not listen (no -bind)

ht
...