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

💬 ryanofsky commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1745679987)
That would be nice and if I can figure out a way to check for that more directly I could drop the second half of this new test. It looks like there is a log message "%i block-relay-only anchors will be tried for connections" that gets printed when `m_use_addrman_outgoing` is false so maybe I could check for that when -noconnect is specified, and have better confirmation that noconnect is working correctly, than this `dnsseed_ignored` check provides.

I think the first half of the test checking
...

👍 hebasto approved a pull request: "cmake: scope Boost Test check to `vcpkg`"
(https://github.com/bitcoin/bitcoin/pull/30822#pullrequestreview-2283298003)
ACK dda17b44e7a680412a231e64a5a09120a850bb1f, I have reviewed the code and it looks OK.

💬 l0rinc commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1745687847)
Seems a bit excessive to me, could we maybe specialize the tests when those errors are actually added?

📝 fanquake opened a pull request: "cmake: add `USE_SOURCE_PERMISSIONS` to all `configure_file()` usage"
(https://github.com/bitcoin/bitcoin/pull/30823)
`USE_SOURCE_PERMISSIONS` is the default, so this should not change behaviour. However, being explicit makes it clear what we are doing.

Related to #30815.

See https://cmake.org/cmake/help/latest/command/configure_file.html#options.

💬 fanquake commented on issue "build: reproducibility issue with macOS Guix builds":
(https://github.com/bitcoin/bitcoin/issues/30815#issuecomment-2331921478)
Opened #30823 to add the explicit permissions usage. We can also use that PR to try and solve this issue, if wanted.

💬 hebasto commented on pull request "cmake: add `USE_SOURCE_PERMISSIONS` to all `configure_file()` usage":
(https://github.com/bitcoin/bitcoin/pull/30823#issuecomment-2331926727)
Could variations in the source directory permissions actually cause https://github.com/bitcoin/bitcoin/issues/30815?