⚠️ Sjors opened an issue: "Qt build fails on macOS 15.0"
(https://github.com/bitcoin/bitcoin/issues/31009)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Building bitcoin-qt produces a flood of errors:
```
In file included from /Users/sjors/dev/bitcoin/src/qt/csvmodelwriter.cpp:5:
In file included from /Users/sjors/dev/bitcoin/src/qt/csvmodelwriter.h:8:
In file included from /usr/local/opt/qt@5/lib/QtCore.framework/Headers/QList:1:
In file included from /usr/local/opt/qt@5/lib/QtCore.framework/Headers/qlist.h:48:
/usr/local/include/
...
(https://github.com/bitcoin/bitcoin/issues/31009)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Building bitcoin-qt produces a flood of errors:
```
In file included from /Users/sjors/dev/bitcoin/src/qt/csvmodelwriter.cpp:5:
In file included from /Users/sjors/dev/bitcoin/src/qt/csvmodelwriter.h:8:
In file included from /usr/local/opt/qt@5/lib/QtCore.framework/Headers/QList:1:
In file included from /usr/local/opt/qt@5/lib/QtCore.framework/Headers/qlist.h:48:
/usr/local/include/
...
💬 Sjors commented on issue "Qt build fails on macOS 15.0 and 13.7":
(https://github.com/bitcoin/bitcoin/issues/31009#issuecomment-2385679198)
Oh, the problem is the presence of `qt` (6) in combination with `qt@5`, which I installed for testing #30997.
`brew remove qt` "fixes" it.
(https://github.com/bitcoin/bitcoin/issues/31009#issuecomment-2385679198)
Oh, the problem is the presence of `qt` (6) in combination with `qt@5`, which I installed for testing #30997.
`brew remove qt` "fixes" it.
📝 hebasto opened a pull request: "cmake: Avoid hardcoding Qt's major version in Find module"
(https://github.com/bitcoin/bitcoin/pull/31010)
This PR facilitates future migration to Qt 6 and is a prerequisite for https://github.com/bitcoin/bitcoin/pull/30997.
(https://github.com/bitcoin/bitcoin/pull/31010)
This PR facilitates future migration to Qt 6 and is a prerequisite for https://github.com/bitcoin/bitcoin/pull/30997.
👍 ryanofsky approved a pull request: "log: Enforce trailing newline"
(https://github.com/bitcoin/bitcoin/pull/30929#pullrequestreview-2340113273)
Code review ACK fa2b7d8d6b3f8d53199921e1e542072441b26fab. Just comment and test cleanup since last review
(https://github.com/bitcoin/bitcoin/pull/30929#pullrequestreview-2340113273)
Code review ACK fa2b7d8d6b3f8d53199921e1e542072441b26fab. Just comment and test cleanup since last review
💬 Sjors commented on pull request "doc: add testnet4 section header for config file":
(https://github.com/bitcoin/bitcoin/pull/31007#issuecomment-2385709480)
utACK 605926da0ab4ac7ae4e9aed55a059f37c31c15b5
(https://github.com/bitcoin/bitcoin/pull/31007#issuecomment-2385709480)
utACK 605926da0ab4ac7ae4e9aed55a059f37c31c15b5
💬 fanquake commented on issue "Having qt(@6) breaks build for qt@5 on macOS 15.0 and 13.7":
(https://github.com/bitcoin/bitcoin/issues/31009#issuecomment-2385712586)
I assume this is related to [`AUTOMOC`](https://cmake.org/cmake/help/latest/manual/cmake-qt.7.html#automoc), and CMake getting confused and picking the wrong tooling to use.
(https://github.com/bitcoin/bitcoin/issues/31009#issuecomment-2385712586)
I assume this is related to [`AUTOMOC`](https://cmake.org/cmake/help/latest/manual/cmake-qt.7.html#automoc), and CMake getting confused and picking the wrong tooling to use.
💬 fanquake commented on pull request "build: Mark `x86_64-linux-gnu` release binaries as CET-enabled":
(https://github.com/bitcoin/bitcoin/pull/30685#issuecomment-2385714543)
What is the status of this?
(https://github.com/bitcoin/bitcoin/pull/30685#issuecomment-2385714543)
What is the status of this?
💬 fanquake commented on pull request "refactor: Avoid unsigned integer overflow in `script/interpreter.cpp`":
(https://github.com/bitcoin/bitcoin/pull/29543#issuecomment-2385715734)
Closing in favour of the other one.
(https://github.com/bitcoin/bitcoin/pull/29543#issuecomment-2385715734)
Closing in favour of the other one.
✅ fanquake closed a pull request: "refactor: Avoid unsigned integer overflow in `script/interpreter.cpp`"
(https://github.com/bitcoin/bitcoin/pull/29543)
(https://github.com/bitcoin/bitcoin/pull/29543)
💬 maflcko commented on pull request "doc: add testnet4 section header for config file":
(https://github.com/bitcoin/bitcoin/pull/31007#discussion_r1782730746)
```suggestion
- placed into sections with headers `[main]` (not `[mainnet]`), `[test]` (not `[testnet]`, for testnet3), `[testnet4]`, `[signet]` or `[regtest]`;
```
It would be `[testnet4]`, not `testnet4`.
(https://github.com/bitcoin/bitcoin/pull/31007#discussion_r1782730746)
```suggestion
- placed into sections with headers `[main]` (not `[mainnet]`), `[test]` (not `[testnet]`, for testnet3), `[testnet4]`, `[signet]` or `[regtest]`;
```
It would be `[testnet4]`, not `testnet4`.
💬 Sjors commented on pull request "depends: Print ready-to-use `--toolchain` option for CMake invocation":
(https://github.com/bitcoin/bitcoin/pull/31008#issuecomment-2385746639)
tACK 605926da0ab4ac7ae4e9aed55a059f37c31c15b5
(https://github.com/bitcoin/bitcoin/pull/31008#issuecomment-2385746639)
tACK 605926da0ab4ac7ae4e9aed55a059f37c31c15b5
💬 pablomartin4btc commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1782780671)
My bad, my suggestion was more for discussion as I did it on top of @achow101's one. The tests can be updated but I see we could be breaking things on users end if they pass named args (`filename=`) as in `wallet_startup,py`.
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1782780671)
My bad, my suggestion was more for discussion as I did it on top of @achow101's one. The tests can be updated but I see we could be breaking things on users end if they pass named args (`filename=`) as in `wallet_startup,py`.
💬 MarnixCroes commented on pull request "doc: add testnet4 section header for config file":
(https://github.com/bitcoin/bitcoin/pull/31007#discussion_r1782784699)
fixed, my bad...
(https://github.com/bitcoin/bitcoin/pull/31007#discussion_r1782784699)
fixed, my bad...
📝 ryanofsky opened a pull request: "refactor: move util/pcp and util/netif to common/"
(https://github.com/bitcoin/bitcoin/pull/31011)
Move util/pcp.cpp and util/netif.cpp to common/ because they depend on netaddress.cpp which is part of the common library. This was causing check-deps.sh script to fail as reported by _fanquake_ in https://github.com/bitcoin/bitcoin/pull/30415#issuecomment-2385475097.
Also fix CI to fail when the `check-deps.sh` script fails. Previously it would output errors but not cause the job to fail (which was unintentional).
(https://github.com/bitcoin/bitcoin/pull/31011)
Move util/pcp.cpp and util/netif.cpp to common/ because they depend on netaddress.cpp which is part of the common library. This was causing check-deps.sh script to fail as reported by _fanquake_ in https://github.com/bitcoin/bitcoin/pull/30415#issuecomment-2385475097.
Also fix CI to fail when the `check-deps.sh` script fails. Previously it would output errors but not cause the job to fail (which was unintentional).
💬 ryanofsky commented on pull request "contrib: fix check-deps.sh to check for weak symbols":
(https://github.com/bitcoin/bitcoin/pull/30415#issuecomment-2385820890)
> Looks like this doesn't fail when it detects a problem? i.e the tidy job is passing (most recent run: https://api.cirrus-ci.com/v1/task/5757501226876928/logs/ci.log), but is currently outputting:
Nice catch. Opened #31011 to address this
(https://github.com/bitcoin/bitcoin/pull/30415#issuecomment-2385820890)
> Looks like this doesn't fail when it detects a problem? i.e the tidy job is passing (most recent run: https://api.cirrus-ci.com/v1/task/5757501226876928/logs/ci.log), but is currently outputting:
Nice catch. Opened #31011 to address this
🤔 danielabrozzoni reviewed a pull request: "Add waitFeesChanged() to Mining interface"
(https://github.com/bitcoin/bitcoin/pull/31003#pullrequestreview-2340337601)
Code review ACK 9a723c784ad5170b1e01e91f018e5794a51dd038 - code looks good to me, I don't have an opinion on merging both commits vs waiting for cluster mempool for the second one.
(https://github.com/bitcoin/bitcoin/pull/31003#pullrequestreview-2340337601)
Code review ACK 9a723c784ad5170b1e01e91f018e5794a51dd038 - code looks good to me, I don't have an opinion on merging both commits vs waiting for cluster mempool for the second one.
🤔 hodlinator reviewed a pull request: "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error"
(https://github.com/bitcoin/bitcoin/pull/30928#pullrequestreview-2339765974)
Concept ACK 4f33e9a85c3a8f546deddc2f78cf74bceff30315
It would be cleaner *in the implementation* to use `util::Result` instead of overwriting the returned value with the error message, but it would mess up the call sites too much probably. Current approach in this PR seems one of the least worse ones.
Passed locally: `FUZZ=str_printf src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/str_printf/*`.
### PR summary nit
> rename the throwing `tfm::format` functions to `tfm::format_raw`
T
...
(https://github.com/bitcoin/bitcoin/pull/30928#pullrequestreview-2339765974)
Concept ACK 4f33e9a85c3a8f546deddc2f78cf74bceff30315
It would be cleaner *in the implementation* to use `util::Result` instead of overwriting the returned value with the error message, but it would mess up the call sites too much probably. Current approach in this PR seems one of the least worse ones.
Passed locally: `FUZZ=str_printf src/test/fuzz/fuzz ../qa-assets/fuzz_seed_corpus/str_printf/*`.
### PR summary nit
> rename the throwing `tfm::format` functions to `tfm::format_raw`
T
...
💬 hodlinator 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_r1782505271)
Agree the current message is confusing. The distinction between format string and parameters to the format string should ideally be made clearer, as well as the actual string type.
```suggestion
- *Rationale*: Tinyformat handles `std::string` parameters. Format strings
on the other hand should be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.
```
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1782505271)
Agree the current message is confusing. The distinction between format string and parameters to the format string should ideally be made clearer, as well as the actual string type.
```suggestion
- *Rationale*: Tinyformat handles `std::string` parameters. Format strings
on the other hand should be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.
```
💬 instagibbs commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1782953943)
done
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1782953943)
done
💬 instagibbs commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2386103070)
@maflcko done, thanks
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2386103070)
@maflcko done, thanks