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

💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781298290)
Probably something that should be mentioned in the commit message? Also in the PR description, if you're adding the requirement that building for a certain platform will now require the very latest release of Qt.

💬 instagibbs commented on pull request "refactor: ensure type safety for txid and wtxid in `RelayTransaction`":
(https://github.com/bitcoin/bitcoin/pull/31001#issuecomment-2383456576)
@dergoegge that's an argument for bigger bites then?

Not against these or more changes.

💬 marcofleon commented on pull request "refactor: ensure type safety for txid and wtxid in `RelayTransaction`":
(https://github.com/bitcoin/bitcoin/pull/31001#issuecomment-2383468176)
I'll look to expand this one a bit then. I also think the suggestion to add more context is a good one. That way I can refer back to this PR on future ones as a way to track progress.

💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1781330901)
In 9ad6801174298241cfeff80c5ba9214b4bb9981f (ci: Update for Qt 6):
I was under the impression that these kinds of GUI-only warnings should be suppressed in the build system, i.e:
https://github.com/bitcoin/bitcoin/blob/f3c74c4a7e120ea363abe4d4aa034b85c1d71919/src/qt/CMakeLists.txt#L126 ? I think we should do one or the other, but not spread supression between the build system and the CI.

👍 instagibbs approved a pull request: "refactor: TxDownloadManager + fuzzing"
(https://github.com/bitcoin/bitcoin/pull/30110#pullrequestreview-2337805760)
reACK ce205abe10ebccfdbea60adf4c568a8ba61390c3

via `git range-diff master 1d4e33e7f88162cf6fcd6aee0b63973015853591 ce205abe10ebccfdbea60adf4c568a8ba61390c3`

new unit test coverage looks superior, and catches the move error previously encountered.