🚀 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
...
💬 hodlinator commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2332633451)
#### Invalid chars check
Here follows an implementation of the low bar of what I stated in https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2273568357. It's implemented on top of fa72ce66421d3f90a6794b3e54e56873ae81265f.
<details>
<summary>
##### Diff
</summary>
I've confirmed that "aAcdfeEfFgGinopsuxX" matches what I linked to in cppreference and also the Python version being replaced.
```diff
diff --git a/src/test/util_string_tests.cpp b/src/test/util_strin
...
(https://github.com/bitcoin/bitcoin/pull/30546#issuecomment-2332633451)
#### Invalid chars check
Here follows an implementation of the low bar of what I stated in https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2273568357. It's implemented on top of fa72ce66421d3f90a6794b3e54e56873ae81265f.
<details>
<summary>
##### Diff
</summary>
I've confirmed that "aAcdfeEfFgGinopsuxX" matches what I linked to in cppreference and also the Python version being replaced.
```diff
diff --git a/src/test/util_string_tests.cpp b/src/test/util_strin
...
👍 hodlinator approved a pull request: "Remove unsafe uint256S() and test-only uint160S()"
(https://github.com/bitcoin/bitcoin/pull/30773#pullrequestreview-2284131255)
re-ACK b60013ed71b697bab98884aa475fb64ae7736c2e
`git range-diff master 9ef049d b60013`
Collapsed `conversion` test change into one commit.
New `ParseHashV` commit is more user-friendly.
(https://github.com/bitcoin/bitcoin/pull/30773#pullrequestreview-2284131255)
re-ACK b60013ed71b697bab98884aa475fb64ae7736c2e
`git range-diff master 9ef049d b60013`
Collapsed `conversion` test change into one commit.
New `ParseHashV` commit is more user-friendly.
💬 hodlinator commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1746191005)
nit: The old `conversion` test compared against integer literals here. The `methods` test in *arith_uint256_tests.cpp* compares against `0`, so this is covered in part.
Still, what do you think about adding something like:
```diff
diff --git a/src/test/arith_uint256_tests.cpp b/src/test/arith_uint256_tests.cpp
index f596951dac..d437b6a54b 100644
--- a/src/test/arith_uint256_tests.cpp
+++ b/src/test/arith_uint256_tests.cpp
@@ -565,6 +565,8 @@ BOOST_AUTO_TEST_CASE(conversion)
for (u
...
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1746191005)
nit: The old `conversion` test compared against integer literals here. The `methods` test in *arith_uint256_tests.cpp* compares against `0`, so this is covered in part.
Still, what do you think about adding something like:
```diff
diff --git a/src/test/arith_uint256_tests.cpp b/src/test/arith_uint256_tests.cpp
index f596951dac..d437b6a54b 100644
--- a/src/test/arith_uint256_tests.cpp
+++ b/src/test/arith_uint256_tests.cpp
@@ -565,6 +565,8 @@ BOOST_AUTO_TEST_CASE(conversion)
for (u
...
💬 instagibbs commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1745984816)
do we have functional test coverage that it *is* allowed elsewhere? Just check that it doesn't fail rather than rip out?
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1745984816)
do we have functional test coverage that it *is* allowed elsewhere? Just check that it doesn't fail rather than rip out?
💬 instagibbs commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1745994360)
I wonder if we want to change this string to "insufficient fee, does not improve feerate diagram" to give a hint at what to do to user
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1745994360)
I wonder if we want to change this string to "insufficient fee, does not improve feerate diagram" to give a hint at what to do to user
🤔 instagibbs reviewed a pull request: "[WIP] Cluster mempool implementation"
(https://github.com/bitcoin/bitcoin/pull/28676#pullrequestreview-2283792936)
poked at the function tests a bit
(https://github.com/bitcoin/bitcoin/pull/28676#pullrequestreview-2283792936)
poked at the function tests a bit
💬 instagibbs commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1745982880)
nothing fails when I remove this
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1745982880)
nothing fails when I remove this
💬 instagibbs commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1746041322)
Here's a diff to clean up some of the test language and double-checking the right number of clusters
```
diff --git a/test/functional/mempool_package_rbf.py b/test/functional/mempool_package_rbf.py
index f75b13a571..201f303525 100755
--- a/test/functional/mempool_package_rbf.py
+++ b/test/functional/mempool_package_rbf.py
@@ -176,95 +176,103 @@ class PackageRBFTest(BitcoinTestFramework):
assert_equal(f"package RBF failed: insufficient anti-DoS fees, rejecting replacement {failure
...
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1746041322)
Here's a diff to clean up some of the test language and double-checking the right number of clusters
```
diff --git a/test/functional/mempool_package_rbf.py b/test/functional/mempool_package_rbf.py
index f75b13a571..201f303525 100755
--- a/test/functional/mempool_package_rbf.py
+++ b/test/functional/mempool_package_rbf.py
@@ -176,95 +176,103 @@ class PackageRBFTest(BitcoinTestFramework):
assert_equal(f"package RBF failed: insufficient anti-DoS fees, rejecting replacement {failure
...
💬 instagibbs commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1745997228)
I think `test_rbf_carveout_disallowed` is meaningless post-cluster mempool, we should be relying on other tests to ensure we never go above cluster count limit
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1745997228)
I think `test_rbf_carveout_disallowed` is meaningless post-cluster mempool, we should be relying on other tests to ensure we never go above cluster count limit
💬 instagibbs commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1746092665)
this test is pretty ancient and tbh not sure worth holding around. But in case we want to hold onto it, modifications to make things more sensible in cluster mempool world
```
diff --git a/test/functional/mempool_packages.py b/test/functional/mempool_packages.py
index f7fe381f43..9cf1aa01f5 100755
--- a/test/functional/mempool_packages.py
+++ b/test/functional/mempool_packages.py
@@ -1,92 +1,91 @@
#!/usr/bin/env python3
# Copyright (c) 2014-2022 The Bitcoin Core developers
# Distri
...
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1746092665)
this test is pretty ancient and tbh not sure worth holding around. But in case we want to hold onto it, modifications to make things more sensible in cluster mempool world
```
diff --git a/test/functional/mempool_packages.py b/test/functional/mempool_packages.py
index f7fe381f43..9cf1aa01f5 100755
--- a/test/functional/mempool_packages.py
+++ b/test/functional/mempool_packages.py
@@ -1,92 +1,91 @@
#!/usr/bin/env python3
# Copyright (c) 2014-2022 The Bitcoin Core developers
# Distri
...
💬 instagibbs commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1746201647)
heh, if you try 99 cluster conflicts + sibling eviction, that implies the new transaction has a vsize cap of 1kvB since it's a child, and since it will be north of 5kvB it will be rejected. I don't think we can have sibling eviction count meaningfully towards exceeding max cluster conflicts. Worth the coverage anyways?
```
diff --git a/test/functional/mempool_truc.py b/test/functional/mempool_truc.py
index e3fcc45059..4e8e5943fc 100755
--- a/test/functional/mempool_truc.py
+++ b/test/func
...
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1746201647)
heh, if you try 99 cluster conflicts + sibling eviction, that implies the new transaction has a vsize cap of 1kvB since it's a child, and since it will be north of 5kvB it will be rejected. I don't think we can have sibling eviction count meaningfully towards exceeding max cluster conflicts. Worth the coverage anyways?
```
diff --git a/test/functional/mempool_truc.py b/test/functional/mempool_truc.py
index e3fcc45059..4e8e5943fc 100755
--- a/test/functional/mempool_truc.py
+++ b/test/func
...