💬 l0rinc commented on pull request "Cover remaining tinyformat usages in CheckFormatSpecifiers":
(https://github.com/bitcoin/bitcoin/pull/30999#discussion_r1780907365)
Good idea, thanks

💬 l0rinc commented on pull request "Cover remaining tinyformat usages in CheckFormatSpecifiers":
(https://github.com/bitcoin/bitcoin/pull/30999#discussion_r1780907498)
[Added back](https://github.com/bitcoin/bitcoin/compare/23f2887af3d6d088433d25cf682575955b5bccd8..44aa62e98b113e118a620d9cf0cee92d8684c8fb) the part that I think is relevant, let me know if you'd like me to rewrite it.

💬 l0rinc 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_r1780912027)
Yes, your review is welcome there as well

💬 l0rinc commented on pull request "doc/build-osx.md:brew relinking note":
(https://github.com/bitcoin/bitcoin/pull/30993#discussion_r1780919060)
Is this related to https://github.com/bitcoin/bitcoin/pull/30454/files#r1709145204?
> [l0rinc](https://github.com/l0rinc) [on Aug 8](https://github.com/bitcoin/bitcoin/pull/30454/files#r1709145204)
👍 GUI is working with cmake, but I had to execute a `brew link qt5 --force` as well after `brew install qt@5` (applies to Autotools as well)

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2382905543)
ACK 5c7cacf649a6b474b876a7d219c7dc683a25e33d

Just a documentation squash since my last test and review.

💬 Sjors commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2382907372)
@hebasto this was on macOS, not a cross-compile. `$HOST` is `my-computer.localdomain`

Your patch seems to work, it's building now. Will let you know if I find other issues.

📝 marcofleon opened a pull request: "refactor: ensure type safety for txid and wtxid in `RelayTransaction`"
(https://github.com/bitcoin/bitcoin/pull/31001)
This PR updates the `RelayTransaction` function to replace `uint256` with the `Txid` and `Wtxid` types, improving type safety and helping with the gradual transition to using these types throughout the codebase.

💬 marcofleon commented on pull request "fuzz: fix bug in p2p_headers_presync harness":
(https://github.com/bitcoin/bitcoin/pull/30980#discussion_r1780938489)
If I add the first block to `all_headers` then I could do this. I do like the variable for clarity, and I'd say this assertion is mainly to make the test easier to read. So I'll keep it as is.

💬 stickies-v 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_r1780954370)
Thanks for providing the diff. There are probably plenty of opportunities to clean up the bitcoin-cli string printing, but this line is not touched and it does not relate to the goal of this PR, so it seems best to me to leave this as is.

💬 l0rinc commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2382959425)
@davidgumberg mentioned that it's possible flushing the block index is causing the regression. This could also explain why there isn't a regression when the blocksdir is on an SSD and the coinsdb is not.

💬 l0rinc 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_r1780966189)
Ok, thanks for checking

💬 maflcko commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1780966877)
To clarify, with "code-golf on effectively dead code" I meant adding the newline while keeping the compile-time check. Either the newline is appended going forward, dropping the compile-time checks, or the compile-time checks are kept and the newline is appended at runtime. But doing both doesn't make sense.

💬 l0rinc 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#issuecomment-2382966945)
Most of my observations should rather be addressed in a [separate PR](https://github.com/bitcoin/bitcoin/pull/30999) (can be merged before or after, I don't mind rebasing, would appreciate a review there).
I'm fine with merging this one as is, thanks for the improvement.

ACK 2e33dbadcf69838c8ea591dccdc666f809c3134a

💬 maflcko commented on pull request "Cover remaining tinyformat usages in CheckFormatSpecifiers":
(https://github.com/bitcoin/bitcoin/pull/30999#discussion_r1780979165)
Those are dynamic width fields, so I still don't understand why you remove that from the comment.

💬 maflcko commented on pull request "Cover remaining tinyformat usages in CheckFormatSpecifiers":
(https://github.com/bitcoin/bitcoin/pull/30999#discussion_r1780976817)
What is the benefit of `ValidFormatSpecifiers` over the existing `PassFmt`, other than dropping the code coverage stats?

Seems fine to update the comment below to say `// Execute compile-time check again at run-time to get code coverage stats.`, but not sure about dropping it.

🤔 fanquake reviewed a pull request: "build: Switch to Qt 6"
(https://github.com/bitcoin/bitcoin/pull/30997#pullrequestreview-2337118352)
Seems like this will have a number of prerequisite PRs; 1 per platform for trying to change the runtime requirements of our binaries, and another to actually make it possible to have different runtime requirements for the gui compared to the other binaries, if that is going to be the approach.

It'd be good if 49b025636be266fa17cbb4cdb9d541c0e71f2ef8 explained why bumping to 12 is needed, as the Qt 6 supported platforms for Linux claims anything down to GCC 9 should be supported: https://doc.q
...

💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1780881370)
Where does this patch come from? If upstream, can you link to the issue/commits? If not upstream/ it's a bug in Qt, has an issue been opened upstream?

💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1780880968)
Where does this patch come from? If upstream, can you link to the issue/commits? If not upstream/ it's a bug in Qt, has an issue been opened upstream?

💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1780923077)
Is LLVM only needed for building the Qt documentation? I'd have thought there'd be a flag for disabling/this can't be a hard requirement, otherwise you'd also need a Clang/LLVM installation to build Qt, even when using GCC? I see there are some flags here for disabling related things: https://github.com/bitcoin/bitcoin/pull/30997/commits/2c7ae0dc91b005218b0e1e79bfbc729e8d14c71e#diff-0d7e256978e78897b774581dad22102138ce4ba46aa07faaef1ae6fc6178aad4R98, but I'm guessing they mustn't quite work prop
...

💬 fanquake commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1780879026)
This code still exists in qt 6.7.2, so why is this ok to drop?