💬 maflcko commented on pull request "Cover remaining tinyformat usages in CheckFormatSpecifiers":
(https://github.com/bitcoin/bitcoin/pull/30999#discussion_r1780979165)
Those are dynamic width fields, so I still don't understand why you remove that from the comment.

💬 maflcko commented on pull request "Cover remaining tinyformat usages in CheckFormatSpecifiers":
(https://github.com/bitcoin/bitcoin/pull/30999#discussion_r1780976817)
What is the benefit of `ValidFormatSpecifiers` over the existing `PassFmt`, other than dropping the code coverage stats?

Seems fine to update the comment below to say `// Execute compile-time check again at run-time to get code coverage stats.`, but not sure about dropping it.

🤔 fanquake reviewed a pull request: "build: Switch to Qt 6"
(https://github.com/bitcoin/bitcoin/pull/30997#pullrequestreview-2337118352)
Seems like this will have a number of prerequisite PRs; 1 per platform for trying to change the runtime requirements of our binaries, and another to actually make it possible to have different runtime requirements for the gui compared to the other binaries, if that is going to be the approach.

It'd be good if 49b025636be266fa17cbb4cdb9d541c0e71f2ef8 explained why bumping to 12 is needed, as the Qt 6 supported platforms for Linux claims anything down to GCC 9 should be supported: https://doc.q
...

💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1780881370)
Where does this patch come from? If upstream, can you link to the issue/commits? If not upstream/ it's a bug in Qt, has an issue been opened upstream?

💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1780880968)
Where does this patch come from? If upstream, can you link to the issue/commits? If not upstream/ it's a bug in Qt, has an issue been opened upstream?

💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1780923077)
Is LLVM only needed for building the Qt documentation? I'd have thought there'd be a flag for disabling/this can't be a hard requirement, otherwise you'd also need a Clang/LLVM installation to build Qt, even when using GCC? I see there are some flags here for disabling related things: https://github.com/bitcoin/bitcoin/pull/30997/commits/2c7ae0dc91b005218b0e1e79bfbc729e8d14c71e#diff-0d7e256978e78897b774581dad22102138ce4ba46aa07faaef1ae6fc6178aad4R98, but I'm guessing they mustn't quite work prop
...

💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1780879026)
This code still exists in qt 6.7.2, so why is this ok to drop?

💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1780897149)
Can you explain these patches? Seems like a lot of code copy-pasted out of Qt itself? Is this "normal" to have to do when trying to build Qt?

⚠️ Sjors opened an issue: "rfc: DATUM mining interface requirements"
(https://github.com/bitcoin/bitcoin/issues/31002)
The Mining interface is designed with Stratum v2 in mind, and can likely also be used to incrementally improve Statum "v1" application.

Ocean recently announced [DATUM](https://ocean.xyz/docs/datum). So far it's proprietary and undocumented, but hopefully that changes. It could be a good test for the generality of the Mining interface to see if the DATUM gateway requires any additional methods.

Here's the current interface:

https://github.com/bitcoin/bitcoin/blob/d812cf11896a2214467b6fa
...

💬 l0rinc commented on pull request "Cover remaining tinyformat usages in CheckFormatSpecifiers":
(https://github.com/bitcoin/bitcoin/pull/30999#discussion_r1780998335)
I've added the comment back, that's indeed important context.
Compared to `PassFmt` I found the `ValidFormatSpecifiers` to be more specific (I'm not a fan of abbrvs and // comments).
Don't have strong preference here, I can be convinced to rename it back, if you do.

💬 l0rinc commented on pull request "Cover remaining tinyformat usages in CheckFormatSpecifiers":
(https://github.com/bitcoin/bitcoin/pull/30999#discussion_r1780998380)
Because we do have some dynamic width support for the values that were used in the codebase.
But I've reverted the original (deleting `%*c` which is a compile-time failure now).

💬 ryanofsky commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1781004019)
It seems a little crazy for logging code to use `\n` in format strings when logger doesn't support multiline log strings, and more crazy to add a compile time check enforcing presence of `\n` characters which serve no purpose, add noise, and give misleading impression that the logger supports multiline strings when it doesn't. I think the best thing would be go to ignore trailing \n characters, stop using them going forward, and not require them.

👍 ryanofsky approved a pull request: "refactor: Replace g_genesis_wait_cv with m_tip_block_cv"
(https://github.com/bitcoin/bitcoin/pull/30967#pullrequestreview-2337359384)
Code review ACK fa73a1d45d91fe289554e1f9f572917af5ba9d91, adding fix for potential log wakeup bug and some new comments.

💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1781021643)
you should probably update these tests as well

💬 stickies-v commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1781022698)
Ah, good catch, thanks. Updated to remove the quotes and revert to existing behaviour, to avoid messing up the logs like e.g. the below:

With quotes:
```
2024-09-30T12:22:36Z Warning: Error "tinyformat: Cannot convert from argument type to integer for use as variable width or precision" while formatting log message: "Warning: %*s Support for testnet3 is deprecated and will be removed in an upcoming release. Consider switching to testnet4.
"Script verification uses 7 additional threads
```
...

💬 stickies-v commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2383057475)
Force-pushed to [revert the changed behaviour that wraps the format string into quotes](https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1780895517), which doesn't play nice with newlines.

💬 l0rinc commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2383057763)
ACK 4f33e9a85c3a8f546deddc2f78cf74bceff30315

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781050754)
> Where does this patch come from? If upstream, can you link to the issue/commits? If not upstream/ it's a bug in Qt, has an issue been opened upstream?

Not upstream. Qt does not officially support cross-compiling from non-macOS build platforms to macOS. I don't think any related issue upstream will be considered.

💬 ryanofsky commented on pull request "[RFC] Switch and/or align debugging flags (back) to `-Og`":
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2383121313)
If there disadvantages to setting -Og everywhere, as achow is suggesting, maybe it makes sense for depends build to be able to pass flags to the bitcoin build, but let the bitcoin build be free to override them, and keep using -O0 for itself on linux but switch to -Og if needed to avoid problems on other platforms.

I think this would be compatible with approach I suggested in #30813, where depends build provides flags that the main build can use to choose default flag values, and users can fr
...

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781087385)
Can you explain these patches? Seems like a lot of code copy-pasted out of Qt itself?

All `top_level_*` files are not "patches". They are indeed copy-pasted from https://github.com/qt/qt5 to reproduce Qt's full source infrastructure, which is required to configure multiple separate modules at once.

> Is this "normal" to have to do when trying to build Qt?

There are other alternatives:
1. Download the full source [archive](https://download.qt.io/official_releases/qt/6.7/6.7.2/single/).
...