💬 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?

💬 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
...

💬 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
...

💬 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
...

💬 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).

💬 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

💬 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
...

💬 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
...

💬 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).

💬 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
...

👍 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
...

🚀 fanquake merged a pull request: "guix: Drop no longer needed `PATH` modification"
(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.

💬 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

💬 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?

💬 marcofleon commented on pull request "refactor: ensure type safety for txid and wtxid in `RelayTransaction`":
(https://github.com/bitcoin/bitcoin/pull/31001#issuecomment-2383391992)
> is this in view of some follow-on work or a one-off?

I noticed it while doing a bit of review on Erlay (https://github.com/bitcoin/bitcoin/pull/30116).

> 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?

Agreed that it may be too small a change to warrant a PR. I was trying to keep the scope within `RelayTransaction` only. But I can do some more investigation and see if it makes sense
...

💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781281974)
In 1b95937fb70b5f5ec7462d48018b7180cc37c1e1 (Fix compiling for Windows):

From what I can tell, `QModernWindowsStylePlugin` was [introduced in Qt `6.7.0`](https://github.com/qt/qtbase/commit/65d58e6c41e3c549c89ea4f05a8e467466e79ca3), which I assume means this change drags the minimum required Qt for Windows up to `6.7.0` (the current latest release)? Is that correct?

💬 dergoegge commented on pull request "refactor: ensure type safety for txid and wtxid in `RelayTransaction`":
(https://github.com/bitcoin/bitcoin/pull/31001#issuecomment-2383422769)
> Easier to swallow these useful pills if the code is going to have churn anyways

I understand where this is coming from but if we only do these refactors if we're already touching the code, then I don't think we'll ever finish the migration (there is simply too much code to be touched). I'd prefer for us to move to the new types as soon as possible and remove the `const uint256&` conversion function, so that we end up with actual type-safety.

I suggested to Marco to work on this, so that
...

💬 ryanofsky commented on pull request "log: Enforce trailing newline":
(https://github.com/bitcoin/bitcoin/pull/30929#issuecomment-2383429828)
Approach ACK fadce8893d766a9ead7493c1a00a885066156b00. Two things:

- Would be good to clarify in the description that the reason why "This refactor does not change behavior" is that the only unterminated log prints were removed in #30485, so there is no code relying on the old behavior. (Assuming this is the case)

- I wonder if it would be possible to simplify the implementation by just making the newline part of log formatting:

```diff
--- a/src/logging.cpp
+++ b/src/logging.cpp
@@
...

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781294860)
Correct.