💬 TheCharlatan commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2331380598)
> Any particular commit that's problematic? I could try splitting it.

I don't think so. The logic in the changed lines is relatively easy to understand, but it takes a bit of time to check where the notifications are issued and what the differences between the two might be. I don't think dividing anything here might help with that.

🚀 fanquake merged a pull request: "test: Use std::span and std::string_view for raw data"
(https://github.com/bitcoin/bitcoin/pull/30796)

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1745457490)
good point :+1:

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745462272)
Maybe in a follow-up, given that the existing linter doesn't warn either?

💬 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)