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

🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: optimized candidate search"
(https://github.com/bitcoin/bitcoin/pull/30286#pullrequestreview-2297522865)
Re-ACK 59028de422bbf8ccf53da27f1153e343952e585f

I am having consistently similar result like @glozow although some bench were unstable.

| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 13,409.81 | 74,572.28 | 8.4% | 0.01 | :wavy_dash: `Linearize16TxWorstCase120Iters` (Unstable with ~71.1 iters. Increase `minEpochIterations` to e.g. 711)
| 2
...

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754908585)
but that's not what this does, that's below this condition.
This states: if the current char is not a `%`, ignore it (which wouldn't be the first thing I'd say when explaining to someone how the formatter works)

💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#issuecomment-2343967142)
@ismaelsadeeq I typically overcome that with:
```
-min-time=<milliseconds>
Minimum runtime per benchmark, in milliseconds (default: 10)
```

to make it long enough to be stable

💬 maflcko commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1754932214)
not sure about the naming here. This `PushData` does not push any data (at least not in a sense what the `OP_PUSHDATA*` are referring to). `InsertData` may be a better name. However, this would also be confusing, because the method is private and just a wrapper around the public `insert` method. It seems easier to just call `insert` directly instead of creating another wrapper.

Also, it may be easier to review to leave the code where it was, instead of moving it and coming up with new functio
...

💬 l0rinc commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1754944106)
Unlike `insert`, this always adds element to the `end()`, so it's either an append or a `push`.

> . The "deprecated" wrapper could be a simple return *this << std::as_bytes(b); which compilers should be able to optimize.

Please see my arguments against that in https://github.com/bitcoin/bitcoin/pull/30765#issuecomment-2342178439

💬 pablomartin4btc commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#issuecomment-2344004014)
> Good idea. Description updated.

Thanks! It looks good.

Forgot to mention that it would be nice to add some test checking `relevant_blocks` is returned by `scanblocks status` in `test/functional/rpc_scanblocks.py` if possible.

💬 sipa commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1754975816)
Pushing has a specific meaning in the context of Bitcoin Script (it's an opcode that pushes data onto the stack), and that's not what's happening here so that would be very confusing.

I don't understand the argument you link to. `*this << std::as_bytes(b);` is well-defined, obviously correct, concise, and trivially optimized out.