💬 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_r1780845253)
Hmm, then I don't think I understand what you mean. Would you be able to provide a diff/example?

👍 l0rinc approved a pull request: "log: Enforce trailing newline at compile time"
(https://github.com/bitcoin/bitcoin/pull/30929#pullrequestreview-2337059996)
ACK facfdcef4a512d4249c63134e632f6154652f022

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2382848929)
`abec00bb80...4101c7ac8d`: rebase due to conflicts

💬 maflcko commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#issuecomment-2382851754)
It would be possible to drop the first commit completely and just replace it with a patch that appends a newline if it is missing. This means that no behavior is enforced at compile-time and the newline placement in code could be inconsistent, but the thing that matters is fixed: A trailing newline is ensured to be always present.

```diff
diff --git a/src/logging.h b/src/logging.h
index f465622d0b..9998f85c35 100644
--- a/src/logging.h
+++ b/src/logging.h
@@ -256,7 +256,7 @@ inline void
...

💬 maflcko 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_r1780895517)
last commit: Not that it matters, but my preference would be to not change behavior here and keep the previous formatting. If `fmt` has a trailing newline, it would be "removed" by this patch.

💬 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_r1780898391)
My mistake, rereading it it's not clear what I was asking.
So I think we could simplify the nested formats by pulling up the internal pattern:
Having `%2s` in the main pattern and having a `%s%s` inside could be simplified to having `%s%s` at the parent and layout out the values in the parameters:
```patch
Index: src/bitcoin-cli.cpp
<+>UTF-8
===================================================================
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
--- a/src/bitcoin-cli.cpp
...

💬 vasild commented on pull request "rpc: provide per message stats for global traffic via new RPC 'getnetmsgstats'":
(https://github.com/bitcoin/bitcoin/pull/29418#issuecomment-2382864737)
`f85814052d...a8e531f92f`: rebase due to conflicts

👍 vasild approved a pull request: "net: Replace libnatpmp with built-in PCP+NATPMP implementation"
(https://github.com/bitcoin/bitcoin/pull/30043#pullrequestreview-2337161558)
ACK 5c7cacf649a6b474b876a7d219c7dc683a25e33d

💬 l0rinc commented on pull request "log: Enforce trailing newline at compile time":
(https://github.com/bitcoin/bitcoin/pull/30929#issuecomment-2382870857)
> replace it with a patch that appends a newline if it is missing

Didn't you call this code-golf in https://github.com/bitcoin/bitcoin/pull/30929#discussion_r1778763759?
I'd prefer keeping this explicit (as it i now) - or renaming the method to end with `ln` if we're appending.

💬 l0rinc commented on pull request "Cover remaining tinyformat usages in CheckFormatSpecifiers":
(https://github.com/bitcoin/bitcoin/pull/30999#discussion_r1780907250)
Valid complaint, I've [added](https://github.com/bitcoin/bitcoin/compare/23f2887af3d6d088433d25cf682575955b5bccd8..44aa62e98b113e118a620d9cf0cee92d8684c8fb) a `consteval ValidFormatSpecifiers` local delegate and split the valid tests from the `BOOST_CHECK_EXCEPTION`s.

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