🤔 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)
⚠️ danilotg opened an issue: "Trying to run bitcoin qt on Windows and getting an AV"
(https://github.com/bitcoin/bitcoin/issues/30825)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
********* Start testing of AppTests *********
Config: Using QtTest library 5.15.10, Qt 5.15.10 (x86_64-little_endian-llp64 shared (dynamic) debug build; by MSVC 2022), windows 11
PASS : AppTests::initTestCase()
QINFO : AppTests::appTests() Backing up GUI settings to "C:\\Users\\danilog\\AppData\\Local\\Temp\\test_common_Bitcoin Core\\13cec4b43759d5d6b469fa4cd0b3ea34c2e956e50f2acca5a4
...
(https://github.com/bitcoin/bitcoin/issues/30825)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
********* Start testing of AppTests *********
Config: Using QtTest library 5.15.10, Qt 5.15.10 (x86_64-little_endian-llp64 shared (dynamic) debug build; by MSVC 2022), windows 11
PASS : AppTests::initTestCase()
QINFO : AppTests::appTests() Backing up GUI settings to "C:\\Users\\danilog\\AppData\\Local\\Temp\\test_common_Bitcoin Core\\13cec4b43759d5d6b469fa4cd0b3ea34c2e956e50f2acca5a4
...
💬 davidgumberg commented on pull request "cmake: scope Boost Test check to `vcpkg`":
(https://github.com/bitcoin/bitcoin/pull/30822#issuecomment-2332584796)
Tested ACK https://github.com/bitcoin/bitcoin/commit/a7a4e11db8722c85175bcc4d9f73a713e9e61513
This branch reduces the amount of time that `cmake -B build` takes on my setup by 52% from 26.749 seconds to 12.649 seconds:
```console
$ $ git switch 30822
Switched to branch '30822'
$ hyperfine --warmup 3 --runs 10 --prepare "git clean -dfx" "cmake -B build"
Benchmark 1: cmake -B build
Time (mean ± σ): 12.649 s ± 0.113 s [User: 6.748 s, System: 5.765 s]
Range (min … max):
...
(https://github.com/bitcoin/bitcoin/pull/30822#issuecomment-2332584796)
Tested ACK https://github.com/bitcoin/bitcoin/commit/a7a4e11db8722c85175bcc4d9f73a713e9e61513
This branch reduces the amount of time that `cmake -B build` takes on my setup by 52% from 26.749 seconds to 12.649 seconds:
```console
$ $ git switch 30822
Switched to branch '30822'
$ hyperfine --warmup 3 --runs 10 --prepare "git clean -dfx" "cmake -B build"
Benchmark 1: cmake -B build
Time (mean ± σ): 12.649 s ± 0.113 s [User: 6.748 s, System: 5.765 s]
Range (min … max):
...
💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1746138999)
> Sorry if I'm being dim, I'm really trying to understand your concern.
I can see that and I appreciate it.
> I think BOOST_CHECK_EQUAL(UintToArith256(ArithToUint256(arith)), arith); is the only roundtrip?
Aren't these roundtrip as well because of `GetHex`/`FromHex`?
```C++
const auto u256{uint256::FromHex(arith.GetHex()).value()};
BOOST_CHECK_EQUAL(UintToArith256(u256), arith);
BOOST_CHECK_EQUAL(u256, ArithToUint256(arith));
BOOST_CHECK_EQUAL(ArithToUint256(arith).GetHex(), UintTo
...
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1746138999)
> Sorry if I'm being dim, I'm really trying to understand your concern.
I can see that and I appreciate it.
> I think BOOST_CHECK_EQUAL(UintToArith256(ArithToUint256(arith)), arith); is the only roundtrip?
Aren't these roundtrip as well because of `GetHex`/`FromHex`?
```C++
const auto u256{uint256::FromHex(arith.GetHex()).value()};
BOOST_CHECK_EQUAL(UintToArith256(u256), arith);
BOOST_CHECK_EQUAL(u256, ArithToUint256(arith));
BOOST_CHECK_EQUAL(ArithToUint256(arith).GetHex(), UintTo
...
📝 brunoerg opened a pull request: "fuzz: reduce number of iterations in `crypto_aeadchacha20poly1305` target"
(https://github.com/bitcoin/bitcoin/pull/30826)
By reducing the number of iterations we improve the performance of this target and may increase coverage.
Running with `-runs=100000` from qa-assets I noticed a significant performance improvement and an increase on cov:
master:
```
#100000 DONE cov: 567 ft: 4078 corp: 124/33Kb lim: 4096 exec/s: 793 rss: 499Mb
```
PR:
```
#100000 DONE cov: 568 ft: 3833 corp: 113/15188b lim: 1746 exec/s: 1250 rss: 544Mb
```
(https://github.com/bitcoin/bitcoin/pull/30826)
By reducing the number of iterations we improve the performance of this target and may increase coverage.
Running with `-runs=100000` from qa-assets I noticed a significant performance improvement and an increase on cov:
master:
```
#100000 DONE cov: 567 ft: 4078 corp: 124/33Kb lim: 4096 exec/s: 793 rss: 499Mb
```
PR:
```
#100000 DONE cov: 568 ft: 3833 corp: 113/15188b lim: 1746 exec/s: 1250 rss: 544Mb
```
💬 willcl-ark commented on pull request "[28.x] rc backports":
(https://github.com/bitcoin/bitcoin/pull/30762#issuecomment-2332619781)
ACK b2a137929a20baed161988e24de592b1f59c0096
Backports all look good to me too. I saw two (spurious) functional test errors, but these went away on individual runs and aren't related to these backports:
<details>
<summary>feature_minchainwork.py</summary>
```
255/310 - feature_minchainwork.py failed, Duration: 11 s
stdout:
2024-09-05T15:40:42.055000Z TestFramework (INFO): PRNG seed is: 2091274345751474524
2024-09-05T15:40:42.056000Z TestFramework (INFO): Initializing test directo
...
(https://github.com/bitcoin/bitcoin/pull/30762#issuecomment-2332619781)
ACK b2a137929a20baed161988e24de592b1f59c0096
Backports all look good to me too. I saw two (spurious) functional test errors, but these went away on individual runs and aren't related to these backports:
<details>
<summary>feature_minchainwork.py</summary>
```
255/310 - feature_minchainwork.py failed, Duration: 11 s
stdout:
2024-09-05T15:40:42.055000Z TestFramework (INFO): PRNG seed is: 2091274345751474524
2024-09-05T15:40:42.056000Z TestFramework (INFO): Initializing test directo
...