💬 instagibbs commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1745945489)
also seems like the assertion fails for me at that commit
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1745945489)
also seems like the assertion fails for me at that commit
💬 sipa commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1745962506)
Nice catch. Apparently the example graphs were obtained by finding 100k-1M iterations-optimal clusters *with LIMO* (so while passing in an already-good linearization as input), while the benchmarks didn't pass in an existing linearization.
@sdaftuar is going to construct new ones.
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1745962506)
Nice catch. Apparently the example graphs were obtained by finding 100k-1M iterations-optimal clusters *with LIMO* (so while passing in an already-good linearization as input), while the benchmarks didn't pass in an existing linearization.
@sdaftuar is going to construct new ones.
👍 ryanofsky approved a pull request: "test: Pin and document TEST_DIR_PATH_ELEMENT, SeedRand::FIXED_SEED"
(https://github.com/bitcoin/bitcoin/pull/30748#pullrequestreview-2283763614)
Code review ACK fa84f9decd224ea1c25dc7095bad70a48fa1a534
IMO, `TEST_DIR_PATH_ELEMENT` would be clearer as `TESTS_DIR_NAME` and `"test_common bitcoin"` would be clearer as `"bitcoin tests"` but current names seem fine too.
(https://github.com/bitcoin/bitcoin/pull/30748#pullrequestreview-2283763614)
Code review ACK fa84f9decd224ea1c25dc7095bad70a48fa1a534
IMO, `TEST_DIR_PATH_ELEMENT` would be clearer as `TESTS_DIR_NAME` and `"test_common bitcoin"` would be clearer as `"bitcoin tests"` but current names seem fine too.
💬 ryanofsky commented on pull request "refactor: Avoid wallet code writing node settings file":
(https://github.com/bitcoin/bitcoin/pull/22217#issuecomment-2332366356)
This PR caused an unintended change in behavior. Before this PR, passing `-nowallet=0`would cause a wallet "1" to be loaded. After this PR, it triggers a vague "JSON value of type bool is not of expected type string" error and uncaught exception. #30684 improves this by avoiding the exception and making the error clearer.
(https://github.com/bitcoin/bitcoin/pull/22217#issuecomment-2332366356)
This PR caused an unintended change in behavior. Before this PR, passing `-nowallet=0`would cause a wallet "1" to be loaded. After this PR, it triggers a vague "JSON value of type bool is not of expected type string" error and uncaught exception. #30684 improves this by avoiding the exception and making the error clearer.
💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745986533)
These alternatives work like a simple state machine, close to how we would look at the code: are we inside a format (i.e. started with %) or outside, switching states between the two. The second suggestion can even be used to determine is the number of params is more or if it's less than the desired one.
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745986533)
These alternatives work like a simple state machine, close to how we would look at the code: are we inside a format (i.e. started with %) or outside, switching states between the two. The second suggestion can even be used to determine is the number of params is more or if it's less than the desired one.
📝 hebasto converted_to_draft a pull request: "Reintroduce external signer support for Windows"
(https://github.com/bitcoin/bitcoin/pull/29868)
Based on upstream https://github.com/arun11299/cpp-subprocess/pull/99.
Partially reverts:
- https://github.com/bitcoin/bitcoin/pull/28967
- https://github.com/bitcoin/bitcoin/pull/29489
After this PR, we can proceed to actually remove the [unused code](https://github.com/bitcoin/bitcoin/pull/28981#pullrequestreview-1991272752) from `src/util/subprocess.hpp`.
(https://github.com/bitcoin/bitcoin/pull/29868)
Based on upstream https://github.com/arun11299/cpp-subprocess/pull/99.
Partially reverts:
- https://github.com/bitcoin/bitcoin/pull/28967
- https://github.com/bitcoin/bitcoin/pull/29489
After this PR, we can proceed to actually remove the [unused code](https://github.com/bitcoin/bitcoin/pull/28981#pullrequestreview-1991272752) from `src/util/subprocess.hpp`.
💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745999190)
While the code is slightly simpler (-4 lines, indentation), the error message in case of failure is also simpler:
<details>
<summary>with static assert</summary>
```
bitcoin/src/test/util_string_tests.cpp:16:19: error: static assertion expression is not an integral constant expression
static_assert([] {
^~~~
bitcoin/src/util/string.h:39:34: note: subexpression not valid in a constant expression
if (num_params != count) throw "Format specifier count must
...
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745999190)
While the code is slightly simpler (-4 lines, indentation), the error message in case of failure is also simpler:
<details>
<summary>with static assert</summary>
```
bitcoin/src/test/util_string_tests.cpp:16:19: error: static assertion expression is not an integral constant expression
static_assert([] {
^~~~
bitcoin/src/util/string.h:39:34: note: subexpression not valid in a constant expression
if (num_params != count) throw "Format specifier count must
...
💬 hebasto commented on pull request "Reintroduce external signer support for Windows":
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1746006077)
It is not "extra", but required. Consider this output:
```
C:\Users\hebasto>where echo
INFO: Could not find files for the given pattern(s).
C:\Users\hebasto>where cmd
C:\Windows\System32\cmd.exe
```
(https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1746006077)
It is not "extra", but required. Consider this output:
```
C:\Users\hebasto>where echo
INFO: Could not find files for the given pattern(s).
C:\Users\hebasto>where cmd
C:\Windows\System32\cmd.exe
```
💬 glozow commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2332421427)
> I'm also pretty hesitant to guess what people will find useful in general, e.g., if they're worried about third party replacement cycling, they can be more cautious. The can also include a tapscript branch for anyonecanspend cleanup, just like LN does today!
Seems reasonable to want keyed. Mostly wanted the rationale to be written up here.
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2332421427)
> I'm also pretty hesitant to guess what people will find useful in general, e.g., if they're worried about third party replacement cycling, they can be more cautious. The can also include a tapscript branch for anyonecanspend cleanup, just like LN does today!
Seems reasonable to want keyed. Mostly wanted the rationale to be written up here.
🤔 ryanofsky reviewed a pull request: "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface"
(https://github.com/bitcoin/bitcoin/pull/30697#pullrequestreview-2283809237)
Just leaving some notes in case there are more settings to the chain api later.
(https://github.com/bitcoin/bitcoin/pull/30697#pullrequestreview-2283809237)
Just leaving some notes in case there are more settings to the chain api later.
💬 ryanofsky commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1745999348)
IMO it's not great that this new API allows setting null values when previous API did not. Allowing null values to be set could encourage setting null values, which could encourage using null values, and treating them differently than missing values, which is probably not a good outcome. (Or even if it is good, is something that should be thought about more clearly, not just introduced in a bugfix.)
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1745999348)
IMO it's not great that this new API allows setting null values when previous API did not. Allowing null values to be set could encourage setting null values, which could encourage using null values, and treating them differently than missing values, which is probably not a good outcome. (Or even if it is good, is something that should be thought about more clearly, not just introduced in a bugfix.)
💬 ryanofsky commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1746016362)
Agree this would be an improvement. But I think it would be better to change behavior and avoid writing null settings (for reasons in other comment). Would probably write as:
```c++
if (auto* value = common::FindKey(settings.rw_settings, name)) {
action = update_settings_func(*value);
if (value->isNull()) settings.rw_settings.erase(name);
} else {
UniValue value;
action = update_settings_func(*value);
if (!value.isNull()) settings.rw_settings[name] = std::move(value
...
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1746016362)
Agree this would be an improvement. But I think it would be better to change behavior and avoid writing null settings (for reasons in other comment). Would probably write as:
```c++
if (auto* value = common::FindKey(settings.rw_settings, name)) {
action = update_settings_func(*value);
if (value->isNull()) settings.rw_settings.erase(name);
} else {
UniValue value;
action = update_settings_func(*value);
if (!value.isNull()) settings.rw_settings[name] = std::move(value
...
💬 ryanofsky commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1746002806)
Agree rvalue reference would be an improvement, less for the reason that it would make the function safer to use, and more for the reason that it would make the function more convenient to use, because lvalue references cannot bind to temporary or literal values.
But even better than an rvalue reference would just be a plain value like `common::SettingsValue value` because this would allow the caller to decide whether they want to move or copy, instead of forcing the caller to always move.
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1746002806)
Agree rvalue reference would be an improvement, less for the reason that it would make the function safer to use, and more for the reason that it would make the function more convenient to use, because lvalue references cannot bind to temporary or literal values.
But even better than an rvalue reference would just be a plain value like `common::SettingsValue value` because this would allow the caller to decide whether they want to move or copy, instead of forcing the caller to always move.
💬 ryanofsky commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1746018681)
Seems like as long as `SettingsAction` enum exists this function and deleteRwSettings below should probably take `SettingsAction action` parameters instead of `bool write` parameters to be more consistent and explicit.
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1746018681)
Seems like as long as `SettingsAction` enum exists this function and deleteRwSettings below should probably take `SettingsAction action` parameters instead of `bool write` parameters to be more consistent and explicit.
💬 ryanofsky commented on pull request "Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface":
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1745993344)
> Note: behaviour already existed prior to this PR, but this line (and thus also `overwriteRwSetting`) can throw if `name` is not an existing key, and would benefit from being documented in both function's docstring I think.
map::erase shouldn't actually throw anything, API was always intended to erase the setting if it exists otherwise do nothing
(https://github.com/bitcoin/bitcoin/pull/30697#discussion_r1745993344)
> Note: behaviour already existed prior to this PR, but this line (and thus also `overwriteRwSetting`) can throw if `name` is not an existing key, and would benefit from being documented in both function's docstring I think.
map::erase shouldn't actually throw anything, API was always intended to erase the setting if it exists otherwise do nothing
💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1746036271)
Instead of a functor I think we can simplify this by using a lambda instead.
And we can also simplify the comparison by using string_literals, i.e.:
```C++
#include <string>
using namespace std::string_literals;
...
auto check_throw_num_spec = [](ErrType str) { return str == "Format specifier count must match the argument count!"s; };
BOOST_CHECK_EXCEPTION(ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers(""), ErrType, check_throw_num_spec);
...
```
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1746036271)
Instead of a functor I think we can simplify this by using a lambda instead.
And we can also simplify the comparison by using string_literals, i.e.:
```C++
#include <string>
using namespace std::string_literals;
...
auto check_throw_num_spec = [](ErrType str) { return str == "Format specifier count must match the argument count!"s; };
BOOST_CHECK_EXCEPTION(ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers(""), ErrType, check_throw_num_spec);
...
```
🤔 l0rinc reviewed a pull request: "util: Use consteval checked format string in FatalErrorf"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2283882274)
Concept ACK, the remaining nits are non-blocking
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2283882274)
Concept ACK, the remaining nits are non-blocking
💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1746046371)
If you keep this version, `num_params` can likely be a `size_t` instead:
```diff
diff --git a/src/util/string.h b/src/util/string.h
--- a/src/util/string.h (revision fa72ce66421d3f90a6794b3e54e56873ae81265f)
+++ b/src/util/string.h (date 1725561949080)
@@ -18,13 +18,13 @@
namespace util {
/** Type to denote a format string that was checked at compile time */
-template <int num_params>
+template <size_t num_params>
struct ConstevalFormatString {
const char* const fmt;
...
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1746046371)
If you keep this version, `num_params` can likely be a `size_t` instead:
```diff
diff --git a/src/util/string.h b/src/util/string.h
--- a/src/util/string.h (revision fa72ce66421d3f90a6794b3e54e56873ae81265f)
+++ b/src/util/string.h (date 1725561949080)
@@ -18,13 +18,13 @@
namespace util {
/** Type to denote a format string that was checked at compile time */
-template <int num_params>
+template <size_t num_params>
struct ConstevalFormatString {
const char* const fmt;
...
💬 achow101 commented on pull request "build: work around issue with Boost <= 1.80 and Clang >= 18":
(https://github.com/bitcoin/bitcoin/pull/30821#issuecomment-2332486521)
ACK cd062d6684908d526be7423f8f1057b891254a3c
Tested against Boost 1.73 and Clang 18.1.8 and the error was no longer present.
(https://github.com/bitcoin/bitcoin/pull/30821#issuecomment-2332486521)
ACK cd062d6684908d526be7423f8f1057b891254a3c
Tested against Boost 1.73 and Clang 18.1.8 and the error was no longer present.
✅ achow101 closed an issue: "build: Boost 1.74.0 incompatible with Clang 18"
(https://github.com/bitcoin/bitcoin/issues/30751)
(https://github.com/bitcoin/bitcoin/issues/30751)