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

💬 sipa commented on pull request "rpc: add getdescriptoractivity":
(https://github.com/bitcoin/bitcoin/pull/30708#discussion_r1755015149)
That's a really roundabout way of generating specialized descriptors, but there doesn't seem to be a better way unfortunately.

💬 sipa commented on pull request "Add more cmake presets":
(https://github.com/bitcoin/bitcoin/pull/30871#discussion_r1755023118)
@fanquake That's deliberate; the goal is enabling everything by default in this preset, so that one cannot *accidentally* miss a dependency/feature. If you're knowingly on an environment where one or more things don't work they can be disabled explicitly with `-DWITH_USDT=OFF` still together with the preset.

💬 hebasto commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2344043907)
> > 100% cache hit rate is expected.
>
> I tested this, and got about 90%:
>
> ```shell
> ccache -C
> ccache --zero-stats
>
> cmake -B some_build_dir -DWITH_CCACHE=ON
> cmake --build some_build_dir -j17
>
> ccache --show-stats
> Cacheable calls: 445 / 445 (100.0%)
> Hits: 0 / 445 ( 0.00%)
> Direct: 0
> Preprocessed: 0
> Misses: 445 / 445 (100.0%)
> Local storage:
> Cache size (GiB): 0.2 / 30.0 ( 0.75%)
> Hits:
...

💬 fanquake commented on pull request "Add more cmake presets":
(https://github.com/bitcoin/bitcoin/pull/30871#issuecomment-2344044926)
> From https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html:
> CMakePresets.json and CMakeUserPresets.json live in the project's root directory.

Ok? We can put files where ever we want and just include them. i.e:
```json
"include": [
"ci/ci_presets.json"
],
```

💬 sipa commented on pull request "Add more cmake presets":
(https://github.com/bitcoin/bitcoin/pull/30871#issuecomment-2344047118)
@fanquake I've dropped the CI changes, as they're not really the main thing I want to achieve here. I do think there is value in reducing duplication of some groups of settings, but that can be discussed separately (or perhaps after our cmake presets have had some evolution and considered more stable - I assume these aren't the only ones we'll be adding).