👍 willcl-ark approved a pull request: "depends: sqlite 3.50.4; switch to autosetup"
(https://github.com/bitcoin/bitcoin/pull/32655#pullrequestreview-3504114386)
tACK 1db74914706fcfafb22646288458604a4a7b6282
This looks good to me. Tested a few random wallet operations in addition to the tests, which all seem ok.
Noting that whilst `--disable-dynamic-extensions` was dropped from configure options it's added as a preprocessor directive directly (although I think it could have been re-added to configure options as `--disable-load-extension`, but both are equivalent).
(https://github.com/bitcoin/bitcoin/pull/32655#pullrequestreview-3504114386)
tACK 1db74914706fcfafb22646288458604a4a7b6282
This looks good to me. Tested a few random wallet operations in addition to the tests, which all seem ok.
Noting that whilst `--disable-dynamic-extensions` was dropped from configure options it's added as a preprocessor directive directly (although I think it could have been re-added to configure options as `--disable-load-extension`, but both are equivalent).
🤔 l0rinc reviewed a pull request: "RFC: bench: Add multi thread benchmarking"
(https://github.com/bitcoin/bitcoin/pull/33740#pullrequestreview-3504147480)
Not sure whether the concept of script validation has to creep into the benchmarking parameters, maybe we could generalize it to `-thread-count` or `-par` or something. Left a few nits.
(https://github.com/bitcoin/bitcoin/pull/33740#pullrequestreview-3504147480)
Not sure whether the concept of script validation has to creep into the benchmarking parameters, maybe we could generalize it to `-thread-count` or `-par` or something. Left a few nits.
💬 l0rinc commented on pull request "RFC: bench: Add multi thread benchmarking":
(https://github.com/bitcoin/bitcoin/pull/33740#discussion_r2559279775)
Script checks are currently the main source of threading, but not necessarily limited to this (e.g. https://github.com/bitcoin/bitcoin/pull/31132/commits or https://github.com/bitcoin/bitcoin/pull/26966 or https://github.com/bitcoin/bitcoin/pull/33689 or https://github.com/bitcoin/bitcoin/pull/32747), so I would suggest untangling multithreading from script validation here. If you insist this being about script validation (which sounds a bit too specific to include in a general benchmark), ignor
...
(https://github.com/bitcoin/bitcoin/pull/33740#discussion_r2559279775)
Script checks are currently the main source of threading, but not necessarily limited to this (e.g. https://github.com/bitcoin/bitcoin/pull/31132/commits or https://github.com/bitcoin/bitcoin/pull/26966 or https://github.com/bitcoin/bitcoin/pull/33689 or https://github.com/bitcoin/bitcoin/pull/32747), so I would suggest untangling multithreading from script validation here. If you insist this being about script validation (which sounds a bit too specific to include in a general benchmark), ignor
...
💬 l0rinc commented on pull request "RFC: bench: Add multi thread benchmarking":
(https://github.com/bitcoin/bitcoin/pull/33740#discussion_r2559261336)
hmmm, this looks like a refactor gone wrong, was this `arg.find("-worker-threads=") == 0` originally?
```suggestion
if (arg.starts_with("-worker-threads=")) {
```
so I guess `if (!found) {` should also be adjusted after this
---
Alternatively, instead of finding and overwriting, what if we unconditionally erased and pushed back the actual thread count, something like:
```C++
std::vector current_setup_args {args.setup_args};
if (threads > 0) {
std::erase_if(current_set
...
(https://github.com/bitcoin/bitcoin/pull/33740#discussion_r2559261336)
hmmm, this looks like a refactor gone wrong, was this `arg.find("-worker-threads=") == 0` originally?
```suggestion
if (arg.starts_with("-worker-threads=")) {
```
so I guess `if (!found) {` should also be adjusted after this
---
Alternatively, instead of finding and overwriting, what if we unconditionally erased and pushed back the actual thread count, something like:
```C++
std::vector current_setup_args {args.setup_args};
if (threads > 0) {
std::erase_if(current_set
...
💬 l0rinc commented on pull request "RFC: bench: Add multi thread benchmarking":
(https://github.com/bitcoin/bitcoin/pull/33740#discussion_r2559321265)
What's the purpose of a vector over a range? Are we planning on testing threads `2,4,8,16` instead of `1,2,3,4,5,6...`?
If not, consider making it a range, something like:
```C++
constexpr int DEFAULT_MAX_BENCH_THREADS{16};
const auto [start_threads, end_threads]{is_thread_scaling ?
std::pair{1, DEFAULT_MAX_BENCH_THREADS} : std::pair{0, 0}};
```
and maybe iterate as
```C++
for (int threads{start_threads}; threads <= end_threads; ++threads) {
...
}
```
(https://github.com/bitcoin/bitcoin/pull/33740#discussion_r2559321265)
What's the purpose of a vector over a range? Are we planning on testing threads `2,4,8,16` instead of `1,2,3,4,5,6...`?
If not, consider making it a range, something like:
```C++
constexpr int DEFAULT_MAX_BENCH_THREADS{16};
const auto [start_threads, end_threads]{is_thread_scaling ?
std::pair{1, DEFAULT_MAX_BENCH_THREADS} : std::pair{0, 0}};
```
and maybe iterate as
```C++
for (int threads{start_threads}; threads <= end_threads; ++threads) {
...
}
```
💬 l0rinc commented on pull request "RFC: bench: Add multi thread benchmarking":
(https://github.com/bitcoin/bitcoin/pull/33740#discussion_r2559283533)
What should happen when `worker_threads > MAX_SCRIPTCHECK_THREADS`?
(https://github.com/bitcoin/bitcoin/pull/33740#discussion_r2559283533)
What should happen when `worker_threads > MAX_SCRIPTCHECK_THREADS`?
💬 l0rinc commented on pull request "RFC: bench: Add multi thread benchmarking":
(https://github.com/bitcoin/bitcoin/pull/33740#discussion_r2559324072)
Might be out of scope for this PR, but instead of inlining `DEFAULT_BENCH_FILTER` here, can we move it over to `src/bench/bench.cpp`
```suggestion
bool is_thread_scaling = args.scale_threads && (args.regex_filter != DEFAULT_BENCH_FILTER);
```
(https://github.com/bitcoin/bitcoin/pull/33740#discussion_r2559324072)
Might be out of scope for this PR, but instead of inlining `DEFAULT_BENCH_FILTER` here, can we move it over to `src/bench/bench.cpp`
```suggestion
bool is_thread_scaling = args.scale_threads && (args.regex_filter != DEFAULT_BENCH_FILTER);
```
🤔 rkrux reviewed a pull request: "doc: clarify and cleanup macOS fuzzing notes"
(https://github.com/bitcoin/bitcoin/pull/33921#pullrequestreview-3504404111)
ACK c34bc01b2ff2fc91ed4020288c5fa15f0c5b075e
(https://github.com/bitcoin/bitcoin/pull/33921#pullrequestreview-3504404111)
ACK c34bc01b2ff2fc91ed4020288c5fa15f0c5b075e
⚠️ hulxv opened an issue: "RFC: Replacing `tinyformat` with `{fmt}`"
(https://github.com/bitcoin/bitcoin/issues/33942)
# Description
Bitcoin Core has used tinyformat (via its `strprintf` wrapper) for text formatting for many years, a choice that reflected the library’s simplicity and its ability to be dropped into a large C++ codebase with minimal friction. Over time, the C++ standard and the ecosystem around formatting have evolved: `{fmt}` has become the de facto modern formatting library and served as the basis for `std::format` in later C++ standards. At the same time, a security and maintenance review of T
...
(https://github.com/bitcoin/bitcoin/issues/33942)
# Description
Bitcoin Core has used tinyformat (via its `strprintf` wrapper) for text formatting for many years, a choice that reflected the library’s simplicity and its ability to be dropped into a large C++ codebase with minimal friction. Over time, the C++ standard and the ecosystem around formatting have evolved: `{fmt}` has become the de facto modern formatting library and served as the basis for `std::format` in later C++ standards. At the same time, a security and maintenance review of T
...
💬 rkrux commented on pull request "wallet: don't consider unconfirmed TRUC coins with ancestors":
(https://github.com/bitcoin/bitcoin/pull/33528#discussion_r2559474078)
I see, you might be correct. I hadn't checked the function implementation and assumed this based on the function doc that gave this impression to me.
https://github.com/bitcoin/bitcoin/blob/238c1c8933b1f7479a9bca2b7cb207d26151c39d/src/interfaces/chain.h#L224-L225
https://github.com/bitcoin/bitcoin/blob/238c1c8933b1f7479a9bca2b7cb207d26151c39d/src/txmempool.h#L592-L598
(https://github.com/bitcoin/bitcoin/pull/33528#discussion_r2559474078)
I see, you might be correct. I hadn't checked the function implementation and assumed this based on the function doc that gave this impression to me.
https://github.com/bitcoin/bitcoin/blob/238c1c8933b1f7479a9bca2b7cb207d26151c39d/src/interfaces/chain.h#L224-L225
https://github.com/bitcoin/bitcoin/blob/238c1c8933b1f7479a9bca2b7cb207d26151c39d/src/txmempool.h#L592-L598
🚀 fanquake merged a pull request: "Cluster mempool"
(https://github.com/bitcoin/bitcoin/pull/33629)
(https://github.com/bitcoin/bitcoin/pull/33629)
💬 maflcko commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2559477612)
> `[[unlikely]]`
Will this actually help in a large function without a loop, where a single if is annotated?
Generally, I have the impression that `[[likely]]`, and `[[unlikely]]` should not be used. Possibly in rare cases, where there is a really hot loop, and all release compilers agree that the attribute will help.
In other cases, if the attributes are used too liberal, it seems an accidental pessimization is plausible: It is unclear what the meaning of the attributes are, if nested scopes
...
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2559477612)
> `[[unlikely]]`
Will this actually help in a large function without a loop, where a single if is annotated?
Generally, I have the impression that `[[likely]]`, and `[[unlikely]]` should not be used. Possibly in rare cases, where there is a really hot loop, and all release compilers agree that the attribute will help.
In other cases, if the attributes are used too liberal, it seems an accidental pessimization is plausible: It is unclear what the meaning of the attributes are, if nested scopes
...
🚀 fanquake merged a pull request: "ci: Use latest Xcode that the minimum macOS version allows"
(https://github.com/bitcoin/bitcoin/pull/33932)
(https://github.com/bitcoin/bitcoin/pull/33932)
💬 maflcko commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2559487359)
nit: Could use named args consistently, like below? `/*part_offset=*/0, /*part_size=*/std::nullopt);`
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2559487359)
nit: Could use named args consistently, like below? `/*part_offset=*/0, /*part_size=*/std::nullopt);`
💬 l0rinc commented on pull request "[IBD] specialize CheckBlock's input & coinbase checks":
(https://github.com/bitcoin/bitcoin/pull/31682#issuecomment-3574992541)
Rebased after #33629, removed the conflicting [`1783fbe` (#31682)](https://github.com/bitcoin/bitcoin/pull/31682/commits/1783fbe04dd4938300dadf2553a9bb20accf2ccc) for simplicity
(https://github.com/bitcoin/bitcoin/pull/31682#issuecomment-3574992541)
Rebased after #33629, removed the conflicting [`1783fbe` (#31682)](https://github.com/bitcoin/bitcoin/pull/31682/commits/1783fbe04dd4938300dadf2553a9bb20accf2ccc) for simplicity
🚀 fanquake merged a pull request: "depends: sqlite 3.50.4; switch to autosetup"
(https://github.com/bitcoin/bitcoin/pull/32655)
(https://github.com/bitcoin/bitcoin/pull/32655)
💬 dergoegge commented on pull request "doc: clarify and cleanup macOS fuzzing notes":
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2559558868)
I'm not going to add the steps back in. It's too much of a game of whack-a-mole which invites bike-shedding (what works for one person doesn't work for someone else, it depends on macOS version, brew version etc.) and it looks like people are perfectly capable of figuring out what works for them on their own.
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2559558868)
I'm not going to add the steps back in. It's too much of a game of whack-a-mole which invites bike-shedding (what works for one person doesn't work for someone else, it depends on macOS version, brew version etc.) and it looks like people are perfectly capable of figuring out what works for them on their own.
💬 l0rinc commented on pull request "doc: clarify and cleanup macOS fuzzing notes":
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2559577489)
> it depends on macOS version, brew version etc
What if we documented a solution that would work with latest xcode and clang and whatever else is needed?
That shouldn't be too difficult and shouldn't involve "bike shedding" (I don't think it's the correct term here since it's not a "minor, easily understandable issues" at the expense of "neglecting more complex, important matters").
As mentioned, I strongly think we should have guidance for at least basic fuzzing support for Mac as well!
...
(https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2559577489)
> it depends on macOS version, brew version etc
What if we documented a solution that would work with latest xcode and clang and whatever else is needed?
That shouldn't be too difficult and shouldn't involve "bike shedding" (I don't think it's the correct term here since it's not a "minor, easily understandable issues" at the expense of "neglecting more complex, important matters").
As mentioned, I strongly think we should have guidance for at least basic fuzzing support for Mac as well!
...
💬 l0rinc commented on pull request "doc: clarify and cleanup macOS fuzzing notes":
(https://github.com/bitcoin/bitcoin/pull/33921#issuecomment-3575153013)
Concept ACK, I agree that we should clarify usage of fuzzing on a Max.
However, approach NACK, as described in https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2559577489 (which was marked as resolved for some reason, so reposting here for visibility)
(https://github.com/bitcoin/bitcoin/pull/33921#issuecomment-3575153013)
Concept ACK, I agree that we should clarify usage of fuzzing on a Max.
However, approach NACK, as described in https://github.com/bitcoin/bitcoin/pull/33921#discussion_r2559577489 (which was marked as resolved for some reason, so reposting here for visibility)
💬 diegoviola commented on pull request "Revert "gui, qt: brintToFront workaround for Wayland"":
(https://github.com/bitcoin-core/gui/pull/914#issuecomment-3575174383)
> Instead of linking to a bunch of threads, it could make sense to just say that the issue has been fixed upstream in Qt6, so that the workaround is no longer needed?
The `Qt::WindowStaysOnTopHint` was never going to work on Wayland. Clients don't get to dictate whether they stay on top or not. The compositor does, and you (the user) are supposed to control the compositor through configuration. This is a fundamental difference that a lot of people don't seem to understand.
It seems like th
...
(https://github.com/bitcoin-core/gui/pull/914#issuecomment-3575174383)
> Instead of linking to a bunch of threads, it could make sense to just say that the issue has been fixed upstream in Qt6, so that the workaround is no longer needed?
The `Qt::WindowStaysOnTopHint` was never going to work on Wayland. Clients don't get to dictate whether they stay on top or not. The compositor does, and you (the user) are supposed to control the compositor through configuration. This is a fundamental difference that a lot of people don't seem to understand.
It seems like th
...