💬 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).
💬 maflcko commented on pull request "log: Enforce trailing newline":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1781165062)
> I think the best thing would be go to ignore trailing \n characters, stop using them going forward, and not require them.
Thanks, pushed the diff from https://github.com/bitcoin/bitcoin/pull/30929#issuecomment-2382851754
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1781165062)
> I think the best thing would be go to ignore trailing \n characters, stop using them going forward, and not require them.
Thanks, pushed the diff from https://github.com/bitcoin/bitcoin/pull/30929#issuecomment-2382851754
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781173354)
Ok. This is interesting, because `(@available(macOS` is used in more places in the Qt codebase (and I assume this will only increase in future) than what is patched out here, and, was also present in Qt5, but we didn't have to patch it out there? So maybe there is some other build-related issue here?
This approach of essentially hardcoding our oldest supported version, as also the newest supported version, also means any users of the newer OS's, may be missing out on new features/other improv
...
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781173354)
Ok. This is interesting, because `(@available(macOS` is used in more places in the Qt codebase (and I assume this will only increase in future) than what is patched out here, and, was also present in Qt5, but we didn't have to patch it out there? So maybe there is some other build-related issue here?
This approach of essentially hardcoding our oldest supported version, as also the newest supported version, also means any users of the newer OS's, may be missing out on new features/other improv
...
💬 dergoegge commented on issue "docs: "Fuzzing the Bitcoin Core P2P layer using Honggfuzz NetDriver" is outdated":
(https://github.com/bitcoin/bitcoin/issues/30957#issuecomment-2383287515)
> Just a question, couldn't it affect other targets that also reaches it?
Yes I think so but similar to the PoW check it never makes sense to have this check in the way. The `p2p_transport_serialization` harness currently has [a bunch of extra logic](https://github.com/bitcoin/bitcoin/blob/d812cf11896a2214467b6fa72d7b763bac6077c5/src/test/fuzz/p2p_transport_serialization.cpp#L43-L44) for reaching the unhappy/happy paths of these checks. We can achieve the same with the macro (without having t
...
(https://github.com/bitcoin/bitcoin/issues/30957#issuecomment-2383287515)
> Just a question, couldn't it affect other targets that also reaches it?
Yes I think so but similar to the PoW check it never makes sense to have this check in the way. The `p2p_transport_serialization` harness currently has [a bunch of extra logic](https://github.com/bitcoin/bitcoin/blob/d812cf11896a2214467b6fa72d7b763bac6077c5/src/test/fuzz/p2p_transport_serialization.cpp#L43-L44) for reaching the unhappy/happy paths of these checks. We can achieve the same with the macro (without having t
...
💬 hodlinator commented on pull request "DO NOT MERGE: Windows bitcoind stall debugging":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2383304639)
> Still remains to see if it loads nicely in the debugger.
It debugs!
Requires .DMP, .PDB and .EXE (last one currently luckily added just as an extra test).
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2383304639)
> Still remains to see if it loads nicely in the debugger.
It debugs!
Requires .DMP, .PDB and .EXE (last one currently luckily added just as an extra test).
💬 maflcko commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2383313791)
> I accomplish this with a map of timestamps and callback functions in my event loop
I wonder why the existing scheduler can't be used for re-locking the wallet? I know there is https://github.com/bitcoin/bitcoin/issues/18488 and https://github.com/bitcoin/bitcoin/issues/14289, but the thread is already filled with random stuff such as `BerkeleyDatabase::PeriodicFlush()`, and relocking the wallet seems(?) fast (I haven't benchmarked), so should be fine to put in there as well, at least from t
...
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2383313791)
> I accomplish this with a map of timestamps and callback functions in my event loop
I wonder why the existing scheduler can't be used for re-locking the wallet? I know there is https://github.com/bitcoin/bitcoin/issues/18488 and https://github.com/bitcoin/bitcoin/issues/14289, but the thread is already filled with random stuff such as `BerkeleyDatabase::PeriodicFlush()`, and relocking the wallet seems(?) fast (I haven't benchmarked), so should be fine to put in there as well, at least from t
...
👍 fanquake approved a pull request: "guix: Drop no longer needed `PATH` modification"
(https://github.com/bitcoin/bitcoin/pull/30989#pullrequestreview-2337684777)
ACK f1daa80521eccebe86af6ee6fa594edf40eaa676
```bash
015b853d60c742120b88f1501ce241c8b7b3e874eca9ab150ba2ec282ecb9572 guix-build-f1daa80521ec/output/aarch64-linux-gnu/SHA256SUMS.part
2a8ed51f02046a73dc9a391b8939528c2e506d545274c934202a5643f26b102b guix-build-f1daa80521ec/output/aarch64-linux-gnu/bitcoin-f1daa80521ec-aarch64-linux-gnu-debug.tar.gz
0ce7a6c81b657cfcbd2edf1e18cca8f66bd7bbe15a12b90dd60ddb1218b72254 guix-build-f1daa80521ec/output/aarch64-linux-gnu/bitcoin-f1daa80521ec-aarch64-l
...
(https://github.com/bitcoin/bitcoin/pull/30989#pullrequestreview-2337684777)
ACK f1daa80521eccebe86af6ee6fa594edf40eaa676
```bash
015b853d60c742120b88f1501ce241c8b7b3e874eca9ab150ba2ec282ecb9572 guix-build-f1daa80521ec/output/aarch64-linux-gnu/SHA256SUMS.part
2a8ed51f02046a73dc9a391b8939528c2e506d545274c934202a5643f26b102b guix-build-f1daa80521ec/output/aarch64-linux-gnu/bitcoin-f1daa80521ec-aarch64-linux-gnu-debug.tar.gz
0ce7a6c81b657cfcbd2edf1e18cca8f66bd7bbe15a12b90dd60ddb1218b72254 guix-build-f1daa80521ec/output/aarch64-linux-gnu/bitcoin-f1daa80521ec-aarch64-l
...
🚀 fanquake merged a pull request: "guix: Drop no longer needed `PATH` modification"
(https://github.com/bitcoin/bitcoin/pull/30989)
(https://github.com/bitcoin/bitcoin/pull/30989)
💬 instagibbs commented on pull request "refactor: ensure type safety for txid and wtxid in `RelayTransaction`":
(https://github.com/bitcoin/bitcoin/pull/31001#issuecomment-2383339305)
is this in view of some follow-on work or a one-off? Easier to swallow these useful pills if the code is going to have churn anyways.
(https://github.com/bitcoin/bitcoin/pull/31001#issuecomment-2383339305)
is this in view of some follow-on work or a one-off? Easier to swallow these useful pills if the code is going to have churn anyways.
💬 instagibbs commented on pull request "test: switch MiniWallet padding unit from weight to vsize":
(https://github.com/bitcoin/bitcoin/pull/30718#issuecomment-2383349236)
reACK 940edd6ac24e921f639b1376fe7d35dc14c1887d
(https://github.com/bitcoin/bitcoin/pull/30718#issuecomment-2383349236)
reACK 940edd6ac24e921f639b1376fe7d35dc14c1887d
💬 maflcko commented on pull request "refactor: ensure type safety for txid and wtxid in `RelayTransaction`":
(https://github.com/bitcoin/bitcoin/pull/31001#issuecomment-2383381912)
It may be good to add some context to the pull request description. Something like: Currently there are still about `$N` places with unsafe types and this fixes a good chunk (`$nn%`) of them. If the standalone chunks are too small and this is split-up too much, the review overhead may be higher than needed to make meaningful progress here?
(https://github.com/bitcoin/bitcoin/pull/31001#issuecomment-2383381912)
It may be good to add some context to the pull request description. Something like: Currently there are still about `$N` places with unsafe types and this fixes a good chunk (`$nn%`) of them. If the standalone chunks are too small and this is split-up too much, the review overhead may be higher than needed to make meaningful progress here?