💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1791789613)
There's probably enough GUI-only stuff here, i.e `bison, ninja-build, python3, xz-utils`, that this could be moved to it's own `#### Gui` section.

💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1791790452)
Could you better explain this patch? The description says that using the `QT_NEEDS_QMAIN` macro is required, but using it breaks things, so you're just getting rid of it? So how do things continue to work, if you're removing the required macro?

💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1791797379)
Given that this is introducing a second copy of a patch we already have, it'd be good if there was someone we could avoid having to maintain two different copies of the same thing.

💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1791799818)
I guess same thought here, given that this PR is introducing two copies of this same patch.

🤔 vasild reviewed a pull request: "net, net_processing: additional and consistent disconnect logging"
(https://github.com/bitcoin/bitcoin/pull/28521#pullrequestreview-2354306033)
Approach ACK 9967e8feb6

I noticed the repeating pattern `fLogIPs ? strprintf(", peeraddr=%s", pfrom.addr.ToStringAddrPort()) : ""` while working on https://github.com/bitcoin/bitcoin/pull/29415 and I repeated it a lot of times in the new messages I added :disappointed:. It has been on my TODO to introduce some function to deduplicate that. Thanks for the `CNode::LogIP()` method!

💬 vasild commented on pull request "net, net_processing: additional and consistent disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1791797716)
_(suggestion at random place in the code)_

Consider to also adjust the `CConnman::DisconnectNode()` functions.

💬 vasild commented on pull request "net, net_processing: additional and consistent disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1791761963)
```suggestion
LogDebug(BCLog::NET,
"socket receive timeout: %is, %s\n", count_seconds(now - last_recv),
node.DisconnectMsg(fLogIPs)
```

💬 vasild commented on pull request "net, net_processing: additional and consistent disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1791784114)
This log shouldn't contain "disconnecting" because we do not disconnect.

💬 vasild commented on pull request "net, net_processing: additional and consistent disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#discussion_r1791764812)
This `%d %d`, `last_recv.count() != 0, last_send.count() != 0` is super obscure. It was like that before, so it is fine wrt this PR.

But would be nice to fix it, since this is changing the same log message already. I am not sure how though. Maybe change that `%d` to something like "has never received from the peer" + "has never sent to the peer".

⚠️ dergoegge opened an issue: "Disallow building fuzz binary without `-DBUILD_FOR_FUZZING`"
(https://github.com/bitcoin/bitcoin/issues/31057)
Without `-DBUILD_FOR_FUZZING` the fuzz binary is less suited for testing puporses, because it won't crash on `Assume` and it won't work around fuzz blockers with `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION`. Afaict, the only place where the fuzz binary is currently used without `-DBUILD_FOR_FUZZING` is in the macOS and windows CI jobs.

I would like to propose that we split the CI jobs into separate fuzz-only jobs that use `-DBUILD_FOR_FUZZING` and that we remove support for building the fuzz bi
...

💬 dergoegge commented on issue "Disallow building fuzz binary without `-DBUILD_FOR_FUZZING`":
(https://github.com/bitcoin/bitcoin/issues/31057#issuecomment-2399758280)
cc @maflcko @marcofleon

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2399781582)
> > Ninja is required to build the `qt` package in depends. It is mentioned in `depends/README.md`. However, `doc/build-osx.md` does not cover building with depends, so I don't think it is necessary to mention Ninja there.
>
> Could you please explain why this is the case? Ninja seems like an odd requirement here.

Sure!

From the Qt's [source code](https://github.com/qt/qtbase/blob/92b685784960eea6eb353688cf0edeb94d69c6cd/cmake/QtBuildHelpers.cmake#L12-L16):
```cmake
message(WA
...

👍 dergoegge approved a pull request: "net: fuzz: bypass network magic and checksum validation"
(https://github.com/bitcoin/bitcoin/pull/31012#pullrequestreview-2354435611)
ACK d43966603bdefb0402a2dc93ff990cb254cdf2df

I think we could consider removing the honggfuzz netdriver instructions in a follow up but the changes look good to me anyway. I tested this by running honggfuzz in netdriver mode.

💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1791846656)
Looks like this was dropped entirely. Any particualr reason? It was previously was suppressing:
```bash
CMake Warning at qtbase/cmake/QtPublicAppleHelpers.cmake:920 (message):
Qt requires at least version 14 of Xcode, you're building against version .
Please upgrade. You can turn this warning into an error by configuring
with -DQT_FORCE_FATAL_APPLE_SDK_AND_XCODE_CHECK=ON.
Call Stack (most recent call first):
qtbase/cmake/QtBuildHelpers.cmake:443 (_qt_internal_check_apple_sdk_and_
...

💬 hebasto commented on pull request "build: Rename `PACKAGE_*` variables to `CLIENT_*`":
(https://github.com/bitcoin/bitcoin/pull/31042#issuecomment-2399840931)
Friendly ping @ryanofsky @theuni :)

💬 naumenkogs commented on pull request "net, net_processing: additional and consistent disconnect logging":
(https://github.com/bitcoin/bitcoin/pull/28521#issuecomment-2399877567)
Would be great to address @vasild feedbacks :)

💬 marcofleon commented on issue "Disallow building fuzz binary without `-DBUILD_FOR_FUZZING`":
(https://github.com/bitcoin/bitcoin/issues/31057#issuecomment-2399886750)
This makes sense to me. Could be missing something, but I don't see a strong reason for building a fuzz binary without the specfic behaviors and optimizations we would like for fuzzing. If there's support for this, I could close https://github.com/bitcoin/bitcoin/pull/31028 and help out with this instead.

🤔 marcofleon reviewed a pull request: "net: fuzz: bypass network magic and checksum validation"
(https://github.com/bitcoin/bitcoin/pull/31012#pullrequestreview-2354553239)
Tested ACK d43966603bdefb0402a2dc93ff990cb254cdf2df

🚀 fanquake merged a pull request: "test: remove unused code from `script_tests`"
(https://github.com/bitcoin/bitcoin/pull/31051)

🚀 fanquake merged a pull request: "depends: For mingw cross compile use -gcc-posix to prevent library conflict"
(https://github.com/bitcoin/bitcoin/pull/31013)