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

💬 hebasto commented on pull request "Add more cmake presets":
(https://github.com/bitcoin/bitcoin/pull/30871#issuecomment-2344024032)
> I think _if_ we want to use presets in the CI, they should encapsulate all config, and exist under `/ci/`.

From https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html:
> `CMakePresets.json` and `CMakeUserPresets.json` live in the project's root directory.

💬 l0rinc commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1755005950)
I've called it `Append*` originally but `Push*` was [requested](https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750910623).
Thanks for the input, renamed back.

> `*this << std::as_bytes(b);` is well-defined, obviously correct, concise, and trivially optimized out.

Thanks, seems there's an overwhelming majority for this, thanks for the inputs, [done](https://github.com/bitcoin/bitcoin/compare/e6a5ab7637e60d3ae731c2ac854c170bc009ab99..5e190cd11f6ed8b7ab4db0f01192de63deaf6fd7)!

💬 maflcko commented on pull request "Add more cmake presets":
(https://github.com/bitcoin/bitcoin/pull/30871#issuecomment-2344031208)
The CI fails, because the `ci/test/03_test_script.sh` script does not set the build dir, but uses `cd` to navigate to it and set it implicitly. With a preset build dir, this will fail. A fix would have to make the ci build dir explicit, or remove the preset build dir.

💬 sipa commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1755002681)
I think emitting the address here is confusing. In all of Bitcoin Core's RPC interface, addresses are seen as receive-only entities, through which a wallet receives coins, but the resulting balance then belongs to the wallet. This notion of a "from address" you're using here only makes sense in an "address balance" situation, which doesn't match the model of rest of the RPC interface. E.g. the wallet's `listtransactions` does list an address for both send and receive, but for sending it's the ad
...