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

💬 fanquake commented on pull request "build: work around issue with Boost <= 1.80 and Clang >= 18":
(https://github.com/bitcoin/bitcoin/pull/30821#discussion_r1745578011)
(depending on what GCC does, over the lifetime of the 28.x branch, at some point, it may turn in and #ifndef MSVC etc)

👍 stickies-v approved a pull request: "[28.x] rc backports"
(https://github.com/bitcoin/bitcoin/pull/30762#pullrequestreview-2283131161)
ACK b2a137929a20baed161988e24de592b1f59c0096

Verified that all backports are clean. Didn't review changes in detail but they seem sensible.

💬 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_r1745582376)
Done.
Also applied it to the `assumevalid` example:
```C++
BOOST_CHECK_EQUAL(get_valid_opts({"-assumevalid=0x12"}).assumed_valid_block, uint256{0x12});
```

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

🚀 fanquake merged a pull request: "lint: Check for release note snippets in the wrong folder"
(https://github.com/bitcoin/bitcoin/pull/30812)

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1745612258)
> You already know at the implementation

At which points are these test relying on implementation?

Thanks for providing an example. If you have a diff that shows where the "hardcoded" tests catch something that "my" tests don't, that would really help. I struggle to see how `BOOST_CHECK_EQUAL(ArithToUint256(ZeroL), uint256::ZERO);` is any different from `BOOST_CHECK_EQUAL(ArithToUint256(arith_uint256{0}), uint256{0});` (or how that substitution is relying on implementation details, which y
...

💬 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_r1745635476)
I'm hesitant to add more to the back-and-forth, but I think we may have been talking past each other. In my [latest comment](https://github.com/bitcoin/bitcoin/pull/30618#discussion_r1744124233) I suggested updating the `FromHex()` tests, where based on the [latest force-push](https://github.com/bitcoin/bitcoin/compare/6fa4c92e4c9bf5ea90e25d41b525c88f9c2d834d..2cee181a6cc00840def16c311aae69883130eebd) it seems you were talking about the `FromUserHex()` tests, which I agreed I was okay with based
...

💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745637880)
In the future more (constant) errors will be added, so I think checking what type of error was thrown is useful and doesn't seem harmful either way in the tests?

✅ achow101 closed an issue: "Testnet4 consensus failure due to timewarp related "softfork""
(https://github.com/bitcoin/bitcoin/issues/30786)

💬 achow101 commented on issue "Testnet4 consensus failure due to timewarp related "softfork"":
(https://github.com/bitcoin/bitcoin/issues/30786#issuecomment-2331823260)
My nodes' logs indicate this was resolved at 2024-09-05 02:06:40 UTC when the new chain reorg'd the timewarped one.

💬 ryanofsky commented on pull request "Introduce waitTipChanged() mining interface, replace RPCNotifyBlockChange, drop CRPCSignals & g_best_block":
(https://github.com/bitcoin/bitcoin/pull/30409#issuecomment-2331826005)
> > the change in logic is a bit tricky to follow.
>
> Any particular commit that's problematic? I could try splitting it.

I think the PR is good in it's current form, but if I wanted to be more confident it wasn't changing behavior I would not change code from using `g_best_block` and `::latestblock` directly to calling `waitTipChanged()`. Instead, I would first remove `g_best_block` and replace it with `KernelNotifications::m_tip_block`, then I would remove `::latestblock` and replace it
...

💬 achow101 commented on pull request "refactor: Testnet4 - Replace uint256S("str") -> uint256{"str"}":
(https://github.com/bitcoin/bitcoin/pull/30721#issuecomment-2331847706)
> Needs backport to 28.x branch

Unless this fixes a bug that I'm missing, refactors generally are not candidates for backport.

📝 fanquake opened a pull request: "cmake: scope Boost Test check to `vcpkg`"
(https://github.com/bitcoin/bitcoin/pull/30822)
This check was added for `vcpkg`, given how it packages Boost. However, we don't need to run the check for other platforms, and it's quite slow. So, scope it to just `vcpkg`.

On my machine, this reduces the time to run `time cmake -B build` from ~12 seconds, to ~6 seconds.

Fixes: #30787.