💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1786141210)
The `QMAKE_*` variables are used by the legacy `qmake` build tools, which we neither build nor use.
From https://doc.qt.io/qt-6/qt6-buildsystem.html:
> The Qt 5 build system was built on top of [qmake](https://doc.qt.io/qt-6/qmake-manual.html). In Qt 6, we ported the build system to CMake.
>
> **Note:** This only affects users that want to build Qt from sources. You can still use qmake as a build tool for your applications.
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1786141210)
The `QMAKE_*` variables are used by the legacy `qmake` build tools, which we neither build nor use.
From https://doc.qt.io/qt-6/qt6-buildsystem.html:
> The Qt 5 build system was built on top of [qmake](https://doc.qt.io/qt-6/qmake-manual.html). In Qt 6, we ported the build system to CMake.
>
> **Note:** This only affects users that want to build Qt from sources. You can still use qmake as a build tool for your applications.
👍 tdb3 approved a pull request: "build: have "make test" depend on "make all""
(https://github.com/bitcoin/bitcoin/pull/31015#pullrequestreview-2345481436)
ACK 2957ca9611916efb570d157b9c7a0b188161660d
Not sure how many people this would affect in the near term (e.g. I've experienced Debian 12 (with apt) and Fedora 40 (with dnf) default to make versions under 3.29), but seems like a useful improvement nonetheless.
Tested with Fedora 40 manually install a more recent cmake version (3.30.4).
Saw that `make -C build test` force recompile of transaction.cpp.
(https://github.com/bitcoin/bitcoin/pull/31015#pullrequestreview-2345481436)
ACK 2957ca9611916efb570d157b9c7a0b188161660d
Not sure how many people this would affect in the near term (e.g. I've experienced Debian 12 (with apt) and Fedora 40 (with dnf) default to make versions under 3.29), but seems like a useful improvement nonetheless.
Tested with Fedora 40 manually install a more recent cmake version (3.30.4).
Saw that `make -C build test` force recompile of transaction.cpp.
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1786143393)
> 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.
These files are fetched from the Qt repository as extra sources now.
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1786143393)
> 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.
These files are fetched from the Qt repository as extra sources now.
📝 AgusR7 opened a pull request: "mempool_accept.py changes"
(https://github.com/bitcoin/bitcoin/pull/31025)
refactoring check_mempool_result to check if the key doesn't exists.
max_vin precalculated directly.
some changes on outputs of logs.
Cleaned up imports and code formatting for readability, refactored methods (like check_mempool_result) for clarity, improved error handling, added comments, and ensured adherence to PEP 8 standards for better maintainability.
I hope it helps!
(https://github.com/bitcoin/bitcoin/pull/31025)
refactoring check_mempool_result to check if the key doesn't exists.
max_vin precalculated directly.
some changes on outputs of logs.
Cleaned up imports and code formatting for readability, refactored methods (like check_mempool_result) for clarity, improved error handling, added comments, and ensured adherence to PEP 8 standards for better maintainability.
I hope it helps!
💬 RandyMcMillan commented on pull request "doc/build-osx.md:brew relinking note":
(https://github.com/bitcoin/bitcoin/pull/30993#discussion_r1786165857)
In scenarios like this - the build just fails - a few extra lines for someone actually using the build instructions "may" find it useful.
Additionally a similar note "may" be useful in a scenarios where linuxbrew is being used.
I dont see much downside in trying to make build instructions a little more friendly to users - with minimal experience - building from source.
I'd like to think it is encouraged.
(https://github.com/bitcoin/bitcoin/pull/30993#discussion_r1786165857)
In scenarios like this - the build just fails - a few extra lines for someone actually using the build instructions "may" find it useful.
Additionally a similar note "may" be useful in a scenarios where linuxbrew is being used.
I dont see much downside in trying to make build instructions a little more friendly to users - with minimal experience - building from source.
I'd like to think it is encouraged.
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1786175592)
> Is LLVM only needed for building the Qt documentation?
Additionally, it allows building a Clang-based `lupdate` parser. We explicitly disabled this feature in:https://github.com/bitcoin/bitcoin/blob/d663b154ba05ec93514125936d92a0c37a291444/depends/packages/native_qt.mk#L80
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1786175592)
> Is LLVM only needed for building the Qt documentation?
Additionally, it allows building a Clang-based `lupdate` parser. We explicitly disabled this feature in:https://github.com/bitcoin/bitcoin/blob/d663b154ba05ec93514125936d92a0c37a291444/depends/packages/native_qt.mk#L80
💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1786179009)
So there's a flag to disable that tool, but no flag to disable needing LLVM for the docs? So in the normal case, building QT, requires LLVM, even if building with GCC?
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1786179009)
So there's a flag to disable that tool, but no flag to disable needing LLVM for the docs? So in the normal case, building QT, requires LLVM, even if building with GCC?
📝 fanquake opened a pull request: "ci: set a ctest test timeout of 1200 (20 minutes)"
(https://github.com/bitcoin/bitcoin/pull/31026)
This should be long enough (with headroom) for our longest running tests, which even under MSAN, TSAN, Valgrind, etc max out at about 800s.
i.e under Valgrind I see the longer runtimes as:
```bash
135/136 Test #8: bench_sanity_check_high_priority ..... Passed 371.19 sec
136/136 Test #122: coinselector_tests ................... Passed 343.39 sec
```
In the CI `tests` [under TSAN](https://cirrus-ci.com/task/6321297691508736?logs=ci#L2520):
```bash
tests ........................
...
(https://github.com/bitcoin/bitcoin/pull/31026)
This should be long enough (with headroom) for our longest running tests, which even under MSAN, TSAN, Valgrind, etc max out at about 800s.
i.e under Valgrind I see the longer runtimes as:
```bash
135/136 Test #8: bench_sanity_check_high_priority ..... Passed 371.19 sec
136/136 Test #122: coinselector_tests ................... Passed 343.39 sec
```
In the CI `tests` [under TSAN](https://cirrus-ci.com/task/6321297691508736?logs=ci#L2520):
```bash
tests ........................
...
✅ RandyMcMillan closed a pull request: "doc/build-osx.md:brew relinking note"
(https://github.com/bitcoin/bitcoin/pull/30993)
(https://github.com/bitcoin/bitcoin/pull/30993)
💬 stickies-v commented on pull request "Add -pausebackgroundsync startup option":
(https://github.com/bitcoin/bitcoin/pull/31023#issuecomment-2391498224)
Is there a strong use case for this? I'm hesitant to add any more AssumeUTXO complexity than there already is. When a user is on battery / limited bandwidth, they can just shut down Bitcoin Core until they're ready to use it again?
(https://github.com/bitcoin/bitcoin/pull/31023#issuecomment-2391498224)
Is there a strong use case for this? I'm hesitant to add any more AssumeUTXO complexity than there already is. When a user is on battery / limited bandwidth, they can just shut down Bitcoin Core until they're ready to use it again?
✅ fanquake closed an issue: "28.0 RC Testing Guide Feedback"
(https://github.com/bitcoin/bitcoin/issues/30854)
(https://github.com/bitcoin/bitcoin/issues/30854)
💬 fanquake commented on issue "28.0 RC Testing Guide Feedback":
(https://github.com/bitcoin/bitcoin/issues/30854#issuecomment-2391502087)
Closing, given v28.0 is tagged.
(https://github.com/bitcoin/bitcoin/issues/30854#issuecomment-2391502087)
Closing, given v28.0 is tagged.
💬 laanwj commented on pull request "ci: set a ctest test timeout of 1200 (20 minutes)":
(https://github.com/bitcoin/bitcoin/pull/31026#issuecomment-2391505343)
ACK 93dda4c70254750a5b5c0e7f7d2d89032519b999
Yes, we've had some ridiculously overloaded CI VPSes in the past, but one'd say 20 minutes should be enough for the unit tests even under the most adverse conditions.
(https://github.com/bitcoin/bitcoin/pull/31026#issuecomment-2391505343)
ACK 93dda4c70254750a5b5c0e7f7d2d89032519b999
Yes, we've had some ridiculously overloaded CI VPSes in the past, but one'd say 20 minutes should be enough for the unit tests even under the most adverse conditions.
💬 Sjors commented on pull request "Add -pausebackgroundsync startup option":
(https://github.com/bitcoin/bitcoin/pull/31023#issuecomment-2391514147)
@stickies-v the reason I opened it was because someone needed an easier way to benchmark IBD without going all the way from genesis.
In general this is useful if you don't want to download and process things before the snapshot. Whether that's wise is a different question.
(https://github.com/bitcoin/bitcoin/pull/31023#issuecomment-2391514147)
@stickies-v the reason I opened it was because someone needed an easier way to benchmark IBD without going all the way from genesis.
In general this is useful if you don't want to download and process things before the snapshot. Whether that's wise is a different question.
💬 willcl-ark commented on pull request "ci: set a ctest test timeout of 1200 (20 minutes)":
(https://github.com/bitcoin/bitcoin/pull/31026#issuecomment-2391540348)
ACK 93dda4c70254750a5b5c0e7f7d2d89032519b999
I tried for quite some time to get this to work using `CTEST_TEST_TIMEOUT` from within *CMakeLists.txt* but it does not appear to be possible. This therefore seems like the next best option to limit this test time.
I am curious to see if this actually manages to curtail the long-running/hanging test, or whether the system is locking up in some way that this will have no effect.
(https://github.com/bitcoin/bitcoin/pull/31026#issuecomment-2391540348)
ACK 93dda4c70254750a5b5c0e7f7d2d89032519b999
I tried for quite some time to get this to work using `CTEST_TEST_TIMEOUT` from within *CMakeLists.txt* but it does not appear to be possible. This therefore seems like the next best option to limit this test time.
I am curious to see if this actually manages to curtail the long-running/hanging test, or whether the system is locking up in some way that this will have no effect.
💬 stickies-v commented on pull request "Add -pausebackgroundsync startup option":
(https://github.com/bitcoin/bitcoin/pull/31023#issuecomment-2391564048)
> the reason I opened it was because someone needed an easier way to benchmark IBD without going all the way from genesis.
Wait, how does pausing IDB help with benchmarking IBD?
> In general this is useful if you don't want to download and process things before the snapshot
Right, I can see the use case for that, but that discussion feels more about disabling rather than pausing background validation. Adding a third option seems unnecessarily confusing?
(https://github.com/bitcoin/bitcoin/pull/31023#issuecomment-2391564048)
> the reason I opened it was because someone needed an easier way to benchmark IBD without going all the way from genesis.
Wait, how does pausing IDB help with benchmarking IBD?
> In general this is useful if you don't want to download and process things before the snapshot
Right, I can see the use case for that, but that discussion feels more about disabling rather than pausing background validation. Adding a third option seems unnecessarily confusing?
💬 Sjors commented on pull request "Add -pausebackgroundsync startup option":
(https://github.com/bitcoin/bitcoin/pull/31023#issuecomment-2391574034)
> disabling rather than pausing background validation
I think it's good to maintain the option to finish background validation. E.g. maybe because you want to enable an index.
(https://github.com/bitcoin/bitcoin/pull/31023#issuecomment-2391574034)
> disabling rather than pausing background validation
I think it's good to maintain the option to finish background validation. E.g. maybe because you want to enable an index.
💬 jonatack commented on pull request "doc: update IBD requirements in doc/README.md":
(https://github.com/bitcoin/bitcoin/pull/30992#discussion_r1786342327)
I think I prefer "several hundred or more", as given the current size required, it conveys the floor to expect a bit more, and when the requirement breaks past a terabyte, "or more" allows more time before updating again, whereas "hundreds" might be construed as or implies "not thousands."
(https://github.com/bitcoin/bitcoin/pull/30992#discussion_r1786342327)
I think I prefer "several hundred or more", as given the current size required, it conveys the floor to expect a bit more, and when the requirement breaks past a terabyte, "or more" allows more time before updating again, whereas "hundreds" might be construed as or implies "not thousands."
💬 mzumsande commented on issue "ci: macOS 14 CI failure `Invalid value detected for '-wallet' or '-nowallet'`":
(https://github.com/bitcoin/bitcoin/issues/31019#issuecomment-2391596799)
@tdb3: CI runs always rebase the PR with respect to master, so that is expected (it helps find conflicts). Also, after CI finished, there are re-runs scheduled every now and then that will rebase on new master again.
(https://github.com/bitcoin/bitcoin/issues/31019#issuecomment-2391596799)
@tdb3: CI runs always rebase the PR with respect to master, so that is expected (it helps find conflicts). Also, after CI finished, there are re-runs scheduled every now and then that will rebase on new master again.
💬 josibake commented on pull request "Add -pausebackgroundsync startup option":
(https://github.com/bitcoin/bitcoin/pull/31023#issuecomment-2391626041)
> Wait, how does pausing IDB help with benchmarking IBD?
I had a chat with @Sjors recently regarding a benchmarking use case I have, where I want to benchmark doing IBD on more recent blocks to test flush behaviour. AssumeUTXO seemed useful here in that I could start from, e.g., 800,000 and sync to tip. For this, background validation would likely interfere with the results so having a `-pausebackgroundvalidation` option seemed useful. Worth mentioning, this is not meant to be a representativ
...
(https://github.com/bitcoin/bitcoin/pull/31023#issuecomment-2391626041)
> Wait, how does pausing IDB help with benchmarking IBD?
I had a chat with @Sjors recently regarding a benchmarking use case I have, where I want to benchmark doing IBD on more recent blocks to test flush behaviour. AssumeUTXO seemed useful here in that I could start from, e.g., 800,000 and sync to tip. For this, background validation would likely interfere with the results so having a `-pausebackgroundvalidation` option seemed useful. Worth mentioning, this is not meant to be a representativ
...