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

👋 remyers's pull request is ready for review: "wallet: add coin selection parameter `add_excess_to_recipient_position` for changeless txs with excess that would be added to fees"
(https://github.com/bitcoin/bitcoin/pull/30080)

👍 hodlinator approved a pull request: "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors"
(https://github.com/bitcoin/bitcoin/pull/30765#pullrequestreview-2297471281)
ACK e6a5ab7637e60d3ae731c2ac854c170bc009ab99

`git range-diff master 4574e45 e6a5ab7`

#### a39f57f1a062eb6c3872a35defaf6d893df1d4a4

Nice work on the GCC bug!
Wish the example error in the commit message didn't come from code that only becomes possible in later commit. Maybe worth at least noting that in the message?

#### e6a5ab7637e60d3ae731c2ac854c170bc009ab99

Commit message:
"Replace _hex_v_u8 CScript appends to _hex"
->
"Replace _hex_v_u8 CScript appends with _hex"

💬 l0rinc 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_r1754867304)
Appreciate all your reviews and valuable feedback, thanks for taking the time!

💬 sipa commented on pull request "Add more cmake presets":
(https://github.com/bitcoin/bitcoin/pull/30871#discussion_r1754875081)
Done.

👋 l0rinc's pull request is ready for review: "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors"
(https://github.com/bitcoin/bitcoin/pull/30765)

💬 hodlinator commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754885614)
I reacted in the opposite way when I saw this, happy to pop the `%%`-case off the mental stack early and focus on other cases.

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754891827)
Thanks @ryanofsky, it seems there is a need for that after all, please resolve this comment.

🤔 marcofleon reviewed a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2297515466)
ACK 69bf58dc0e25897e9fde435c9823a921590a90dc

<details>
<summary>Mac Output</summary>

```
bcli -regtest loadwallet
error code: -1
error message:
loadwallet "filename" ( load_on_startup )

Loads a wallet from a wallet file or directory.
Note that all wallet command-line options used when starting bitcoind will be
applied to the new wallet.

Arguments:
1. filename (string, required) You may pass in the absolute path of your wallet directory or .dat file. Otherwise, the i
...