💬 TheCharlatan commented on pull request "depends: build libevent with `-D_GNU_SOURCE`":
(https://github.com/bitcoin/bitcoin/pull/30743#issuecomment-2326285628)
Guix build (aarch64):
```
27bc371752bd40a5f634cc8769c860dae2faec5f71976dba4fef0b0b22c69dc4 guix-build-556775408797/output/aarch64-linux-gnu/SHA256SUMS.part
a0e74a766c104f2e64196c6711e9eb895786847ff08a92a2c3b253f8b2611725 guix-build-556775408797/output/aarch64-linux-gnu/bitcoin-556775408797-aarch64-linux-gnu-debug.tar.gz
0c2099f20a86aff4766e6762b3a69780d72af0f5b26a8579e614085035461a90 guix-build-556775408797/output/aarch64-linux-gnu/bitcoin-556775408797-aarch64-linux-gnu.tar.gz
17c4667a2b
...

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#issuecomment-2326310300)
Force-pushed to address all review comments by @l0rinc and @hodlinator, thanks for the review!
Mostly small style nits, and also added 9ef049db5ea30f60d0b72714c42c74e2d816c820 to move uint256_tests/conversion to arith_uint256_tests [since that better reflects how the code is organized](https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741633513).

🤔 stickies-v reviewed a pull request: "Remove unsafe uint256S() and test-only uint160S()"
(https://github.com/bitcoin/bitcoin/pull/30773#pullrequestreview-2277180999)
Force-pushed to address all review comments by @l0rinc and @hodlinator, thanks for the review!
Mostly small style nits, and also added 9ef049db5ea30f60d0b72714c42c74e2d816c820 to move uint256_tests/conversion to arith_uint256_tests [since that better reflects how the code is organized](https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741633513).

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741905568)
> Thanks, I'll patch this if I have to force-push.

Done!

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741908187)
Nice, didn't quite love the docstring and this is much clearer, thanks.

💬 stickies-v commented on pull request "Remove unsafe uint256S() and test-only uint160S()":
(https://github.com/bitcoin/bitcoin/pull/30773#discussion_r1741909541)
I was considering moving it but then thought it was quite arbitrary, but you're right - tests mirroring how code is structured is good, so I've added a commit to move the `conversion` tests. Thanks!

👋 0xB10C's pull request is ready for review: "tracing: Only prepare tracepoint arguments when actually tracing"
(https://github.com/bitcoin/bitcoin/pull/26593)

💬 0xB10C commented on pull request "tracing: Only prepare tracepoint arguments when actually tracing":
(https://github.com/bitcoin/bitcoin/pull/26593#issuecomment-2326369142)
rebased after #30741 and fixed the compilation with `-DWITH_USDT=ON` (by also setting `SDT_USE_VARIADIC` in `cmake/module/FindUSDT.cmake`)

💬 jadijadi commented on pull request "test: fixing failing system_tests/run_command under some Locales":
(https://github.com/bitcoin/bitcoin/pull/30788#issuecomment-2326382391)
> Also, the commit message is wrong `echo success` is never translated, because `echo` is just a tool to echo something, not to translate a passed string.

you are right and thanks for mentioning it. fixed the message

👍 danielabrozzoni approved a pull request: "test: add check that too large txs aren't put into orphanage"
(https://github.com/bitcoin/bitcoin/pull/30784#pullrequestreview-2277309899)
tACK 30aab71063d039d4386f580a031cb5e5e742c363

💬 maflcko commented on pull request "tracing: explicitly cast block_connected duration to nanoseconds":
(https://github.com/bitcoin/bitcoin/pull/29877#issuecomment-2326417585)
re-lgtm ACK cd0edf26c07c8c615f3ae3ac040c4774dcc8e650

💬 hodlinator commented on pull request "test: increase FromUserHex FUZZ and unit testing coverage":
(https://github.com/bitcoin/bitcoin/pull/30618#issuecomment-2326433232)
> > 2. but `BOOST_CHECK` is deprecated
>
> I don't [see the deprecation](https://live.boost.org/doc/libs/1_86_0/libs/test/doc/html/boost_test/utf_reference/testing_tool_ref/assertion_boost_level.html), I think it's still useful for truthy checks (e.g. empty optionals).

It seems to be poorly documented, but if you search [the header](https://www.boost.org/doc/libs/1_86_0/boost/test/tools/interface.hpp) for "DEPRECATED TOOLS" you'll find it.

💬 jadijadi commented on pull request "test: fixing failing system_tests/run_command under some Locales":
(https://github.com/bitcoin/bitcoin/pull/30788#issuecomment-2326443447)
> The test only wants to check the stderr and exit code, so instead of using `ls`, it may be easier to use a python command ([#29868 (comment)](https://github.com/bitcoin/bitcoin/pull/29868#discussion_r1740457415)), or something similar.

Agree with your solution. Python one-liner is cleaner and more reliable. Used that one so we do not need the Locale anymore. tests pass both in PT and C locale.

by the way since you are working on this in a larger scale, I can close my PR and wait for a f
...

💬 hebasto commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#issuecomment-2326448582)
> ... as a follow up it would be good to:
>
> * Update oss-fuzz to match what is being done here? Now that they've diverged.

The OSS-Fuzz script controls all flags explicitly, including https://github.com/google/oss-fuzz/blob/5403c34ff006baf410b94cad1112a6c385d1bade/projects/bitcoin-core/build.sh#L50:
```bash
export CPPFLAGS="-D_LIBCPP_HARDENING_MODE=_LIBCPP_HARDENING_MODE_DEBUG -DBOOST_MULTI_INDEX_ENABLE_SAFE_MODE"
```

💬 maflcko commented on pull request "test: fixing failing system_tests/run_command under some Locales":
(https://github.com/bitcoin/bitcoin/pull/30788#issuecomment-2326450938)
Looks good. Make sure to adjust the pull request description, now that the approach has changed.

💬 fanquake commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#issuecomment-2326460252)
> The OSS-Fuzz script controls all flags explicitly

Sure, but you changed the CMake build type here, so why not change it in oss-fuzz? Otherwise, it'd be good to explain somewhere why they (should) differ.

💬 hebasto commented on pull request "build: Fix linking for `fuzz` target when building with MSan":
(https://github.com/bitcoin/bitcoin/pull/30778#issuecomment-2326491197)
> > The OSS-Fuzz script controls all flags explicitly
>
> Sure, but you changed the CMake build type here, so why not change it in oss-fuzz? Otherwise, it'd be good to explain somewhere why they (should) differ.

Sure. Please consider https://github.com/google/oss-fuzz/pull/12443.

💬 brunoerg commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#issuecomment-2326502478)
Rebased

💬 theStack commented on pull request "qt, build: remove unneeded `Q_IMPORT_PLUGIN` macro calls":
(https://github.com/bitcoin-core/gui/pull/834#issuecomment-2326505635)
> The log shows "No static plugins."
>
> To verify that static plugins are still being imported, you should build with depends.

Tested that on Ubuntu 23.04 LTS (since for OpenBSD, depends doesn't build QT):
```
2024-09-03T11:47:55Z Bitcoin Core version v28.99.0-7346b0109208 (release build)
2024-09-03T11:47:55Z Qt 5.15.14 (static), plugin=xcb
2024-09-03T11:47:55Z Static plugins:
2024-09-03T11:47:55Z QMinimalIntegrationPlugin, version 331520
2024-09-03T11:47:55Z QXcbIntegrationPlugin
...

👍 hodlinator approved a pull request: "Remove unsafe uint256S() and test-only uint160S()"
(https://github.com/bitcoin/bitcoin/pull/30773#pullrequestreview-2277432402)
re-ACK 9ef049db5ea30f60d0b72714c42c74e2d816c820

`git range-diff master a9d19ae 9ef049d` showed only small expected edits + new commit to move `conversion`-tests.

`ctest -j <cores> --test-dir build` passed.