💬 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)
🚀 achow101 merged a pull request: "build: work around issue with Boost <= 1.80 and Clang >= 18"
(https://github.com/bitcoin/bitcoin/pull/30821)
(https://github.com/bitcoin/bitcoin/pull/30821)
💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1746079204)
By counting the excess we could give more fine-grained errors, e.g:
```C++
constexpr static void Detail_CheckNumFormatSpecifiers(std::string_view str)
{
int excess{num_params};
bool in_format{false};
for (char c: str) {
if (c == '%') in_format = !in_format;
else if (in_format) {
--excess;
in_format = false;
}
}
if (in_format) throw "Format specifier incorrectly terminated by end of string!";
if (excess > 0) thr
...
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1746079204)
By counting the excess we could give more fine-grained errors, e.g:
```C++
constexpr static void Detail_CheckNumFormatSpecifiers(std::string_view str)
{
int excess{num_params};
bool in_format{false};
for (char c: str) {
if (c == '%') in_format = !in_format;
else if (in_format) {
--excess;
in_format = false;
}
}
if (in_format) throw "Format specifier incorrectly terminated by end of string!";
if (excess > 0) thr
...
💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1746095482)
> The test would also pass for a simple re-cast roundtrip:
I'm a bit confused by the example `_mock` functions. I think reinterpret casting a `std::array<uint8_t, 32>` <-> `uint32_t[8]` can actually be a (unreliable and unsafe but depending on memory alignment sometimes correct) way of implementing the conversion functions, since both types are 256 contiguous bits of uint data? So depending on your platform, I think this passing the tests doesn't really provide much information?
Sorry if I
...
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1746095482)
> The test would also pass for a simple re-cast roundtrip:
I'm a bit confused by the example `_mock` functions. I think reinterpret casting a `std::array<uint8_t, 32>` <-> `uint32_t[8]` can actually be a (unreliable and unsafe but depending on memory alignment sometimes correct) way of implementing the conversion functions, since both types are 256 contiguous bits of uint data? So depending on your platform, I think this passing the tests doesn't really provide much information?
Sorry if I
...
💬 l0rinc commented on pull request "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1746097208)
> I'm hesitant to add more to the back-and-forth
I don't mind jousting until we find something that is better than what either of us had in mind. Happens often, keep insisting on parts that you feel strongly about!
Let's see what you think of my latest push:
* FromHex correctness assumes FromUserHex parseability
* FromUserHex correctness assumes that if reconverted to hex, it should be accepted by FromHex
Pushed (first a separate rebase, and a change)
(https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1746097208)
> I'm hesitant to add more to the back-and-forth
I don't mind jousting until we find something that is better than what either of us had in mind. Happens often, keep insisting on parts that you feel strongly about!
Let's see what you think of my latest push:
* FromHex correctness assumes FromUserHex parseability
* FromUserHex correctness assumes that if reconverted to hex, it should be accepted by FromHex
Pushed (first a separate rebase, and a change)