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

💬 fanquake commented on pull request "Add more cmake presets":
(https://github.com/bitcoin/bitcoin/pull/30871#issuecomment-2344009225)
> And then uses these ... in CI.

I'm not sure about doing this. At least, not if it's going to use the same presets as the "general" dev presets. Then we've spread the CI config into 2 different places, made it harder to change the CI (as that might break/make the preset less useful for devs), and opened ourselves up to changes in the presets (silently) breaking the CI. i.e devs decide they no-longer want `BUILD_GUI_TESTS` in the preset, that gets removed, and the CI silently stops running th
...

💬 maflcko commented on pull request "ci: parse TEST_RUNNER_EXTRA into an array":
(https://github.com/bitcoin/bitcoin/pull/30244#issuecomment-2344009687)
Actually, this is wrong. Sorry for the wrong review.

This breaks all existing configs:

```
$ git grep '\-exclude' ci/test/00_setup_env*
ci/test/00_setup_env_native_previous_releases.sh:export TEST_RUNNER_EXTRA="--previous-releases --coverage --extended --exclude feature_dbcrash" # Run extended tests so that coverage does not fail, but exclude the very slow dbcrash
ci/test/00_setup_env_native_valgrind.sh:export TEST_RUNNER_EXTRA="--exclude feature_init,rpc_bind,feature_bind_extra" # fe
...

💬 fanquake commented on pull request "Add more cmake presets":
(https://github.com/bitcoin/bitcoin/pull/30871#discussion_r1754988382)
> "WITH_USDT": "ON",

Something to keep in mind with a generic "dev mode", is that this wont work for all devs. i.e `WITH_USDT=ON` will always fail to configure for devs building on macOS, trying to use this preset.

💬 tdb3 commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#issuecomment-2344018591)
> 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.

It would be. I took a look at `rpc_sccanblocks` when building the PR.
The existing test for status is pretty minimal

💬 maflcko commented on issue "CI timeouts":
(https://github.com/bitcoin/bitcoin/issues/30851#issuecomment-2344019006)
One contributing factor to the timeouts may be a recently introduced bug in the exclude parsing https://github.com/bitcoin/bitcoin/pull/30244#issuecomment-2344009687, which affects one specific CI task. However this still does not explain why suddenly all(?) CI tasks in all branches (at least master, 28.x) are affected.