🤔 promag reviewed a pull request: "qt6, test: Handle deprecated code"
(https://github.com/bitcoin-core/gui/pull/839#pullrequestreview-2346403885)
Concept ACK

💬 promag commented on pull request "qt6, test: Handle deprecated code":
(https://github.com/bitcoin-core/gui/pull/839#discussion_r1786676537)
2b5fd8fd24026ecf2fac7af6657e2363bfe3706c

An alternative to reduce duplicate code and the diff is to define `QVERIFY_EXCEPTION_THROWN`, something like?

```cpp
#if (QT_VERSION >= QT_VERSION_CHECK(6, 3, 0))
#define QVERIFY_EXCEPTION_THROWN(a, b) QVERIFY_THROWS_EXCEPTION(b, a)
#endif
```

🤔 promag reviewed a pull request: "qt6: Handle deprecated `QLocale::nativeCountryName`"
(https://github.com/bitcoin-core/gui/pull/838#pullrequestreview-2346424221)
Code review ACK 9123a286e9743196307d38dd66ae0cfaf3805029.

💬 JayBitron commented on issue "Unable to cross compile on linux for macos (28.x branch)":
(https://github.com/bitcoin/bitcoin/issues/31027#issuecomment-2392074733)
@hebasto Yes, it's a fresh new intstalled linux, I don't use docker, I've been compiling bitcoin for macos since version 25.x had no problem, yes, I've the macos SDK 15.0

🤔 promag reviewed a pull request: "qt6: Fix linking when configured with `-DENABLE_WALLET=OFF`"
(https://github.com/bitcoin-core/gui/pull/837#pullrequestreview-2346444309)
I'll try a different approach.

👍 jarolrod approved a pull request: "depends: Print ready-to-use `--toolchain` option for CMake invocation"
(https://github.com/bitcoin/bitcoin/pull/31008#pullrequestreview-2346464174)
ACK 605926da0ab4ac7ae4e9aed55a059f37c31c15b5

👍 jarolrod approved a pull request: "qt6: Handle deprecated `QLocale::nativeCountryName`"
(https://github.com/bitcoin-core/gui/pull/838#pullrequestreview-2346481059)
ACK 9123a286e9743196307d38dd66ae0cfaf3805029

Built this on macOS with qt5.15 and Qt6.7, as well as on Linux Ubuntu with Qt6.4 and Qt6.7, and tested GUI functions related to translations.

💬 hebasto commented on pull request "qt6, test: Handle deprecated code":
(https://github.com/bitcoin-core/gui/pull/839#discussion_r1786715136)
Thanks! Reworked per your feedback.

💬 hebasto commented on issue "Unable to cross compile on linux for macos (28.x branch)":
(https://github.com/bitcoin/bitcoin/issues/31027#issuecomment-2392146216)
@JayBitron

> -- The C compiler identification is Clang 14.0.6

Please ensure the minimum required Clang version, which is [16.0](https://github.com/bitcoin/bitcoin/pull/30263) for v28.0.

Additionally, you may need the [`lld`](https://packages.debian.org/bookworm/lld) package installed (the same version as llvm/clang).

💬 JayBitron commented on issue "Unable to cross compile on linux for macos (28.x branch)":
(https://github.com/bitcoin/bitcoin/issues/31027#issuecomment-2392151361)
@hebasto Altho the stable version of debian provides the clang 14, I have done also the clang 16 and even upper version, they throw different errors

💬 furszy commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1786696368)
Done. Added the jump line. I don't feel strong about the capitalization neither but it looks incorrect to introduce it now when all other functions aren't using it.

💬 furszy commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1786729786)
> Why not make `SetupUnitTestArgs` available and call it here instead? Delegates the responsibility of documenting the default value to that part of the code.
>
> (Could maybe rename `SetupUnitTestArgs` -> `SetupTestArgs` or `static BasicTestingSetup::RegisterArgs` to make it less unit test-specific and communicate how broadly it is used).

I think we should be careful and explicit about the features we add to other binaries. One of them could end up clashing with another functionality or b
...

💬 furszy commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1786698951)
Have reworked this function in a more general manner just so we can add new args in the future with just one line of parsing code.

💬 furszy commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#discussion_r1786733103)
done.

🚀 hebasto merged a pull request: "qt6: Handle deprecated `QLocale::nativeCountryName`"
(https://github.com/bitcoin-core/gui/pull/838)

💬 furszy commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2392158642)
Thanks for the review @hodlinator. Updated per feedback.

> Good to see these globals implemented for bench. As already mentioned, I've been working on a partially overlapping change in https://github.com/bitcoin/bitcoin/pull/30737.

Cool. It wasn't on my radar. Adding it now.

💬 JayBitron commented on issue "Unable to cross compile on linux for macos (28.x branch)":
(https://github.com/bitcoin/bitcoin/issues/31027#issuecomment-2392168251)
@hebasto lld is installed
```
root@localhost:~/.build/bitcoin-macos# make HOST=x86_64-apple-darwin -C depends -j32
make: Entering directory '/root/.build/bitcoin-macos/depends'
Configuring libevent...
-- The C compiler identification is Clang 20.0.0
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - failed
-- Check for working C compiler: /usr/bin/clang
-- Check for working C compiler: /usr/bin/clang - broken
CMake Error at /usr/share/cmake-3.30/Modules/CMakeTestCComp
...

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1786755937)
True. I'll leave this for a potential follow-up

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1786756748)
I'll leave this one for follow-up as well.

💬 tdb3 commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1786766139)
We seem to be limited/selective in testing for hidden RPCs in functional tests (i.e. there are more hidden RPCs than `addpeeraddress` and `getrawaddrman`). Maybe that sort of check would/could be a larger scope than just `getorphantxs`?

I'm on the fence about testing that RPCs are hidden. Changing from hidden to another category would be noticed in a PR review, and having a functional test for it means that more code needs to be maintained.