💬 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?
(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
...
(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.
(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).
(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.
(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.
(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
(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
```
...
(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.
(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
(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.
(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
...
(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/).
...
(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/).
...
💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#issuecomment-2383146568)
No code changes since https://github.com/bitcoin/bitcoin/commit/cb9be6729c4e27bf2d6d703c0d454a22cbcdb6e1, only commit messages (as requested) and rebased on latest master (separately).
(https://github.com/bitcoin/bitcoin/pull/30906#issuecomment-2383146568)
No code changes since https://github.com/bitcoin/bitcoin/commit/cb9be6729c4e27bf2d6d703c0d454a22cbcdb6e1, only commit messages (as requested) and rebased on latest master (separately).
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781112156)
> They are indeed copy-pasted from https://github.com/qt/qt5 to reproduce Qt's full source infrastructure,
It seems really odd that we'd need to copy-paste a bunch of code out of Qt 5, to compile Qt 6? Is this an approach supported by Qt? Is it guaranteed to keep working going forward, or just happens to work for now?
> Use the qt-configure-module tool.
> I've tried the latter approach, and it makes the code much clunkier.
Could you elaborate on "clunkier", as I assume at some point we
...
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781112156)
> They are indeed copy-pasted from https://github.com/qt/qt5 to reproduce Qt's full source infrastructure,
It seems really odd that we'd need to copy-paste a bunch of code out of Qt 5, to compile Qt 6? Is this an approach supported by Qt? Is it guaranteed to keep working going forward, or just happens to work for now?
> Use the qt-configure-module tool.
> I've tried the latter approach, and it makes the code much clunkier.
Could you elaborate on "clunkier", as I assume at some point we
...
💬 brunoerg commented on issue "docs: "Fuzzing the Bitcoin Core P2P layer using Honggfuzz NetDriver" is outdated":
(https://github.com/bitcoin/bitcoin/issues/30957#issuecomment-2383177268)
> How about using FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION for these checks? Then we wouldn't have to maintain a patch in the docs.
Sounds good, I can address it if others agree. Just a question, couldn't it affect other targets that also reaches it?
(https://github.com/bitcoin/bitcoin/issues/30957#issuecomment-2383177268)
> How about using FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION for these checks? Then we wouldn't have to maintain a patch in the docs.
Sounds good, I can address it if others agree. Just a question, couldn't it affect other targets that also reaches it?
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781132492)
> It seems really odd that we'd need to copy-paste a bunch of code out of Qt 5, to compile Qt 6?
The name of the repository is confusing. See the branches and tags for details. The `dev` branch is QT 6 :)
> Is this an approach supported by Qt? Is it guaranteed to keep working going forward, or just happens to work for now?
It is guaranteed to keep working going forward, even with additional modules for the QML GUI.
> Could you elaborate on "clunkier", as I assume at some point we are
...
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781132492)
> It seems really odd that we'd need to copy-paste a bunch of code out of Qt 5, to compile Qt 6?
The name of the repository is confusing. See the branches and tags for details. The `dev` branch is QT 6 :)
> Is this an approach supported by Qt? Is it guaranteed to keep working going forward, or just happens to work for now?
It is guaranteed to keep working going forward, even with additional modules for the QML GUI.
> Could you elaborate on "clunkier", as I assume at some point we are
...
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781138748)
> The name of the repository is confusing. See the branches and tags for details. The dev branch is QT 6 :)
Ok. I guess in that case my confusing is just around having to need to copy-paste, and maintain code out of Qt 6, into separate patches, to then be able to compile Qt 6.
> It is guaranteed to keep working going forward, even with additional modules for the QML GUI.
Can you link to where this guarantee comes from? It'd be good to document all of this, if it's a less-standard way to
...
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781138748)
> The name of the repository is confusing. See the branches and tags for details. The dev branch is QT 6 :)
Ok. I guess in that case my confusing is just around having to need to copy-paste, and maintain code out of Qt 6, into separate patches, to then be able to compile Qt 6.
> It is guaranteed to keep working going forward, even with additional modules for the QML GUI.
Can you link to where this guarantee comes from? It'd be good to document all of this, if it's a less-standard way to
...
💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2383223911)
Concept ACK
This looks great and the API in the header looks easy, thanks.
I'm in the process of cleaning up my HTTP branch for a pull request and then I can start reviewing this and rebasing on top.
One element of libevent I'm not immediately seeing here is timed events. Really the only thing HTTP needs it for is `walletpassphrase` which calls `RPCRunLater()` which interacts with `HTTPRPCTimerInterface()`. I don't think Conman has a specific mechanism for this because timed things are
...
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2383223911)
Concept ACK
This looks great and the API in the header looks easy, thanks.
I'm in the process of cleaning up my HTTP branch for a pull request and then I can start reviewing this and rebasing on top.
One element of libevent I'm not immediately seeing here is timed events. Really the only thing HTTP needs it for is `walletpassphrase` which calls `RPCRunLater()` which interacts with `HTTPRPCTimerInterface()`. I don't think Conman has a specific mechanism for this because timed things are
...
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781160385)
> Can you link to where this guarantee comes from? It'd be good to document all of this, if it's a less-standard way to build Qt.
Actually, it is the only documented way to configure Qt 6: https://doc.qt.io/qt-6/configure-options.html#configure-workflow.
Using the `qt-configure-module` tool is mentioned in the Qt's [blog](https://www.qt.io/blog/qt-6-build-system).
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781160385)
> Can you link to where this guarantee comes from? It'd be good to document all of this, if it's a less-standard way to build Qt.
Actually, it is the only documented way to configure Qt 6: https://doc.qt.io/qt-6/configure-options.html#configure-workflow.
Using the `qt-configure-module` tool is mentioned in the Qt's [blog](https://www.qt.io/blog/qt-6-build-system).