💬 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.
(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.
(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:
...
(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"
],
```
(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).
(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).
💬 hebasto commented on pull request "Add more cmake presets":
(https://github.com/bitcoin/bitcoin/pull/30871#issuecomment-2344054131)
> > 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"
> ],
> ```
Which in turn requires CMake >= 3.23.
(https://github.com/bitcoin/bitcoin/pull/30871#issuecomment-2344054131)
> > 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"
> ],
> ```
Which in turn requires CMake >= 3.23.
👍 ryanofsky approved a pull request: "util: Use consteval checked format string in FatalErrorf, LogConnectFailure"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2297572187)
Code review ACK faa5a8e4480de69086796dba49b65ef73058f8d7
I like this approach of implementing a loose compile time checker that avoids too much complexity by supporting a superset of tinyformat syntax. This approach is more flexible and should be easier to roll out than the opposite approach which would avoid complexity by only supporting a subset of tinyformat syntax.
In longer run though, I think just supporting a subset of tinyformat syntax would be preferable to provide more safety, an
...
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2297572187)
Code review ACK faa5a8e4480de69086796dba49b65ef73058f8d7
I like this approach of implementing a loose compile time checker that avoids too much complexity by supporting a superset of tinyformat syntax. This approach is more flexible and should be easier to roll out than the opposite approach which would avoid complexity by only supporting a subset of tinyformat syntax.
In longer run though, I think just supporting a subset of tinyformat syntax would be preferable to provide more safety, an
...
💬 ryanofsky commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754990024)
In commit "util: Add ConstevalFormatString" (fa512d7bd333c6bf5868fd131f9513bc51aab5aa)
I think other invalid but accepted case that would be good to check for here or above (depending on whether or not tiny format treats them as runtime errors) would be:
```
ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%123%");
ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%123%s");
```
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754990024)
In commit "util: Add ConstevalFormatString" (fa512d7bd333c6bf5868fd131f9513bc51aab5aa)
I think other invalid but accepted case that would be good to check for here or above (depending on whether or not tiny format treats them as runtime errors) would be:
```
ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%123%");
ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%123%s");
```
💬 ryanofsky commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754940176)
In commit "util: Add ConstevalFormatString" (fa512d7bd333c6bf5868fd131f9513bc51aab5aa)
This comment is true, but potentially misleading because it isn't mentioning that this case will also be reached in cases this function isn't checking like when `#` `+` `-` or space flags are used, when any `.precision` value is specified, as well as in invalid unterminated format arguments like `"%0%s"` that may treated as valid.
Would suggest adding something like "This case is also reached if any flag
...
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754940176)
In commit "util: Add ConstevalFormatString" (fa512d7bd333c6bf5868fd131f9513bc51aab5aa)
This comment is true, but potentially misleading because it isn't mentioning that this case will also be reached in cases this function isn't checking like when `#` `+` `-` or space flags are used, when any `.precision` value is specified, as well as in invalid unterminated format arguments like `"%0%s"` that may treated as valid.
Would suggest adding something like "This case is also reached if any flag
...
💬 ryanofsky commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755013263)
re: https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754821069
> They are not invalid, because in tinyformat there isn't an invalid specifier, see [#30546 (comment)](https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2333261303)
When you say these are not invalid you are saying tinyformat accepts `strprintf("hello %\n", "world")`? Or saying something else?
In any case, it would be good to clarify in unit test comments which of the test cases tinyformat supports and whi
...
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755013263)
re: https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754821069
> They are not invalid, because in tinyformat there isn't an invalid specifier, see [#30546 (comment)](https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2333261303)
When you say these are not invalid you are saying tinyformat accepts `strprintf("hello %\n", "world")`? Or saying something else?
In any case, it would be good to clarify in unit test comments which of the test cases tinyformat supports and whi
...
💬 ryanofsky commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754971776)
In commit "util: Add ConstevalFormatString" (fa512d7bd333c6bf5868fd131f9513bc51aab5aa)
Think it could be a good idea to some tests positional argument combined with width or precision options or flags like:
```c++
ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%1$5i");
ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%1$-5i");
ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%1$.5i");
```
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1754971776)
In commit "util: Add ConstevalFormatString" (fa512d7bd333c6bf5868fd131f9513bc51aab5aa)
Think it could be a good idea to some tests positional argument combined with width or precision options or flags like:
```c++
ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%1$5i");
ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%1$-5i");
ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers("%1$.5i");
```
💬 ryanofsky commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755038937)
In commit "util: Add ConstevalFormatString" (fa512d7bd333c6bf5868fd131f9513bc51aab5aa)
Maybe replace "can" with "will"? It seems like there is no usage of * that ConstevalFormat would accept and tinyformat would not treat as a runtime error. Or is that not the case?
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755038937)
In commit "util: Add ConstevalFormatString" (fa512d7bd333c6bf5868fd131f9513bc51aab5aa)
Maybe replace "can" with "will"? It seems like there is no usage of * that ConstevalFormat would accept and tinyformat would not treat as a runtime error. Or is that not the case?
💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755082651)
> This states: if the current char is not a %, ignore it - which wouldn't be the first thing I'd say ...
For counting the specifiers, anything before the specifier begins with `%` can safely be ignored. I'd explain it this way, but I can see how someone may want to explain it in another way.
Personally, I am fine either way, because this is just a style nit for me. I'll leave this as-is for now, but I may or may not change something if I have to push again.
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755082651)
> This states: if the current char is not a %, ignore it - which wouldn't be the first thing I'd say ...
For counting the specifiers, anything before the specifier begins with `%` can safely be ignored. I'd explain it this way, but I can see how someone may want to explain it in another way.
Personally, I am fine either way, because this is just a style nit for me. I'll leave this as-is for now, but I may or may not change something if I have to push again.
💬 fanquake commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2344083999)
> What is your system and Ccache version?
That was on a macOS machine, with 4.10.2. Retested just now, and got the same: `Hits: 394 / 445 (88.54%)`.
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2344083999)
> What is your system and Ccache version?
That was on a macOS machine, with 4.10.2. Retested just now, and got the same: `Hits: 394 / 445 (88.54%)`.
💬 ismaelsadeeq commented on pull request "interfaces: #30697 follow ups":
(https://github.com/bitcoin/bitcoin/pull/30828#issuecomment-2344085293)
> The PR looks good in it's current form, but could consider improving documentation and simplifying the deleteRwSettings implementation with
Done in the latest force push see diff https://github.com/bitcoin/bitcoin/compare/adc019860d7df6848b98aec58f1c3ff47936e06b..84663291275248fd52da644b0c2566bbf9cc780b
(https://github.com/bitcoin/bitcoin/pull/30828#issuecomment-2344085293)
> The PR looks good in it's current form, but could consider improving documentation and simplifying the deleteRwSettings implementation with
Done in the latest force push see diff https://github.com/bitcoin/bitcoin/compare/adc019860d7df6848b98aec58f1c3ff47936e06b..84663291275248fd52da644b0c2566bbf9cc780b
🤔 pablomartin4btc reviewed a pull request: "docs: updated developer notes for --with-sanitizers to -DSANITIZERS"
(https://github.com/bitcoin/bitcoin/pull/30870#pullrequestreview-2297780864)
ACK 4b1ce3cac81497dd094e8c34550edf633f4d38ae
nit: if you have to retouch, commit title looks [too long](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches) and [prefix](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#creating-the-pull-request) is `doc` usually.
(https://github.com/bitcoin/bitcoin/pull/30870#pullrequestreview-2297780864)
ACK 4b1ce3cac81497dd094e8c34550edf633f4d38ae
nit: if you have to retouch, commit title looks [too long](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#committing-patches) and [prefix](https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#creating-the-pull-request) is `doc` usually.
💬 fanquake commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2344102724)
Just tested the same on a Fedora box (ccache 4.10.2) and ended up with `Hits: 447 / 528 (84.66%)`.
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2344102724)
Just tested the same on a Fedora box (ccache 4.10.2) and ended up with `Hits: 447 / 528 (84.66%)`.
💬 hebasto commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2344111410)
> Just tested the same on a Fedora box (ccache 4.10.2) and ended up with `Hits: 447 / 528 (84.66%)`.
What if you `ccache --zero-stats` after configuring for a new build tree, just before building?
On Fedora, `ccache` masquerades as a compiler, so all compilation during configuration can affect the final hit rate.
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2344111410)
> Just tested the same on a Fedora box (ccache 4.10.2) and ended up with `Hits: 447 / 528 (84.66%)`.
What if you `ccache --zero-stats` after configuring for a new build tree, just before building?
On Fedora, `ccache` masquerades as a compiler, so all compilation during configuration can affect the final hit rate.
👍 ryanofsky approved a pull request: "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors"
(https://github.com/bitcoin/bitcoin/pull/30765#pullrequestreview-2297822100)
Code review ACK 5e190cd11f6ed8b7ab4db0f01192de63deaf6fd7. Looks good!
Since last review, just renamed push methods to append, and applied a few other suggested tweaks
(https://github.com/bitcoin/bitcoin/pull/30765#pullrequestreview-2297822100)
Code review ACK 5e190cd11f6ed8b7ab4db0f01192de63deaf6fd7. Looks good!
Since last review, just renamed push methods to append, and applied a few other suggested tweaks
💬 m3dwards commented on pull request "ci: parse TEST_RUNNER_EXTRA into an array":
(https://github.com/bitcoin/bitcoin/pull/30244#issuecomment-2344112521)
@maflcko no apology needed, apologies I introduced a bug!
I will work out what's going on and propose a fix PR ASAP.
Thanks for the logs.
(https://github.com/bitcoin/bitcoin/pull/30244#issuecomment-2344112521)
@maflcko no apology needed, apologies I introduced a bug!
I will work out what's going on and propose a fix PR ASAP.
Thanks for the logs.