💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745464467)
> NACK

Ok, moved the tests to a unit test file. I hope this is better.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745472652)
> BOOST_CHECK_THROW

I am not using `BOOST_CHECK_THROW`, so I'll leave this as-is for now.

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745475010)
> We could change this to a `constexpr`, leaving the constructor as `consteval`, keeping the usage failures during runtime and be able to properly test it via positive and negative cases.

Ok, done

📝 glozow opened a pull request: "TxDownloadManager refactor followups"
(https://github.com/bitcoin/bitcoin/pull/30820)
Followups to #30110
- https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1742665719 + a few more doc fixes
- https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1742884245

💬 maflcko commented on pull request "Embed default ASMap as binary dump header file":
(https://github.com/bitcoin/bitcoin/pull/28792#discussion_r1745491541)
Could rebase and use the existing `generate_header_from_raw`, which should do the same thing, basically?

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2331616132)
Thanks for the reviews 🙏

I've created a followups PR #30820

After that will be the second part of #28031 ("try multiple peers for orphan resolution"). I'll open a separate PR, since it has 140 comments. After that, I plan to open a PR that moves `m_tx_download_mutex` inside `TxDownloadManagerImpl` (i.e. make it internally thread-safe), which I mention in the PR description here. I had considered making it the next step, but I think waiting a little more is better because
(1) It gives th
...

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1745515725)
> and the tests run so fast that in the case where they are redundant, no one should notice.

Absolutely, the cleanup is to avoid having tests that have unclear purpose, making them easier to maintain going forward. Performance is not the goal here.

I think it's impossible for this PR to not have significant changes to the unit tests, so I feel like since the review is happening anyway the test improvements I've introduced here make sense and add little overhead?

That said, I'm unsure if
...

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745505000)
the errors are really ugly this way:
> util_string_tests.cpp:16:19: error: static assertion expression is not an integral constant expression
static_assert([] {
^~~~

Would there be any disadvantage in inlining them?
```C++
BOOST_AUTO_TEST_CASE(ConstevalFormatString_NumSpec)
{
ConstevalFormatString<0>::Detail_CheckNumFormatSpecifiers("");
ConstevalFormatString<0>::Detail_CheckNumFormatSpecifiers("%");
...
ConstevalFormatString<1>::Detail_CheckNumForm
...

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745513285)
is this a leftover?

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745500013)
How come? Wouldn't that be a simpler way to check that we're throwing in this case (instead of the constant err and err_types which cannot even change)
```C++
BOOST_CHECK_THROW(ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers(""), const char*);
```

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745509491)
As mentioned in another comment, wouldn't this be simpler:
```C++
BOOST_CHECK_THROW(ConstevalFormatString<1>::Detail_CheckNumFormatSpecifiers(""), const char*);
BOOST_CHECK_THROW(ConstevalFormatString<0>::Detail_CheckNumFormatSpecifiers("%_"), const char*);
BOOST_CHECK_THROW(ConstevalFormatString<2>::Detail_CheckNumFormatSpecifiers("%_"), const char*);
```
?

📝 fanquake opened a pull request: "build: work around issue with Boost <= 1.80 and Clang >= 18"
(https://github.com/bitcoin/bitcoin/pull/30821)
Our current minimum supported Boost is `1.73.0`. However, when compiling with Boost `1.74.0` (Debian Stable), using Clang `18`, compilation fails with:
```bash
In file included from /usr/include/boost/mpl/integral_c.hpp:32:
/usr/include/boost/mpl/aux_/integral_wrapper.hpp:73:31: error: integer value -1 is outside the valid range of values [0, 3] for the enumeration type 'udt_builtin_mixture_enum' [-Wenum-constexpr-conversion]
73 | typedef AUX_WRAPPER_INST( BOOST_MPL_AUX_STATIC_CAST(AU
...

💬 stickies-v 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_r1745535351)
I agree, we shouldn't overoptimize. My last suggestion was not about performance, just about making the test more maintainable by removing tests we know don't add anything. We seem to disagree, but I'm okay with keeping the duplication if you think it's better, there's no big cost to it. You can mark this as resolved.

💬 fanquake commented on issue "build: Boost 1.74.0 incompatible with Clang 18":
(https://github.com/bitcoin/bitcoin/issues/30751#issuecomment-2331677969)
> The change to Boost Numeric in 1.81.0 may be enough to avoid the issue.

I think this is the case, PR'd a change in #30821.

👍 hebasto approved a pull request: "qt, build: remove unneeded `Q_IMPORT_PLUGIN` macro calls"
(https://github.com/bitcoin-core/gui/pull/834#pullrequestreview-2283058900)
ACK 7346b0109208c67798bdbd451e509048308eff5d.

Guix-built binaries have been tested on:
- Ubuntu 23.10:
```
2024-09-05T13:22:12Z Bitcoin Core version v28.99.0-g7346b0109208c67798bdbd451e509048308eff5d (release build)
2024-09-05T13:22:12Z Qt 5.15.14 (static), plugin=xcb
2024-09-05T13:22:12Z Static plugins:
2024-09-05T13:22:12Z QMinimalIntegrationPlugin, version 331520
2024-09-05T13:22:12Z QXcbIntegrationPlugin, version 331520
2024-09-05T13:22:12Z Style: fusion / QFusionStyle
2024-09-
...

🚀 hebasto merged a pull request: "qt, build: remove unneeded `Q_IMPORT_PLUGIN` macro calls"
(https://github.com/bitcoin-core/gui/pull/834)

💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1745557271)
You know at the implementation, that's why it seems the same.
The test would also pass for a simple re-cast roundtrip:
```C++
auto ArithToUint256_mock(const arith_uint256 &a) { return *reinterpret_cast<const uint256*>(&a); }
auto UintToArith256_mock(const uint256 &a) { return *reinterpret_cast<const arith_uint256*>(&a); }
BOOST_AUTO_TEST_CASE(conversion)
{
for (const arith_uint256& arith : {ZeroL, OneL, R1L, R2L}) {
const auto u256{uint256::FromHex(arith.GetHex()).value()};

...

💬 l0rinc commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1745564600)
> so I feel like since the review is happening anyway the test improvements I've introduced here make sense and add little overhead?

Agree, let's leave the campground cleaner than we found it.

💬 TheCharlatan commented on pull request "build: work around issue with Boost <= 1.80 and Clang >= 18":
(https://github.com/bitcoin/bitcoin/pull/30821#discussion_r1745569113)
Should this be in a `#if defined(__clang__)` block?

💬 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_r1745569348)
> We seem to disagree

We don't have to, if the above discussion didn't change your mind I'll gladly remove them.

💬 fanquake commented on pull request "build: work around issue with Boost <= 1.80 and Clang >= 18":
(https://github.com/bitcoin/bitcoin/pull/30821#discussion_r1745570403)
A bit annoying, but yea, looks like we'll need something like that to work around the windows compiler in any case.