💬 maflcko commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1780802968)
Added commit fa3ad1da70 bugfix: Mark m_tip_block_cv as guarded by m_tip_block_mutex

💬 maflcko commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1780804362)
> documentation

Updated the `m_tip_block` docs, but let me know if this should be moved to the `waitTipChanged` docs instead.

💬 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#issuecomment-2382717634)
Force-pushed to address two nits from @l0rinc - thanks for the review!

💬 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_r1780827733)
The comment states:
```
// Avoid triggering the following crash bug:
// * strprintf("%.1s", (char*)nullptr);
```
but we're not calling `strprintf` directly anymore, hence my comment

💬 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_r1780832127)
wasn't aware of `BOOST_TEST_CONTEXT`, would indeed be an improvement here

💬 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_r1780833680)
Not sure why it's less clear than before, but it's just a nit from my part anyway

💬 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_r1780834719)
> but we're not calling `strprintf` directly anymore, hence my comment

`strprintf` is an alias for `tfm::format`, introduced by and for Bitcoin Core. This pull request does not touch the alias and does not change it. It exists before an after this pull request.

💬 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_r1780837147)
Can you expand on that? We've changed other `strprintf` instance here to enable compile-time validation, how come this one didn't make the cut?

💬 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_r1780838413)
Ok, it's not that important

💬 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#issuecomment-2382746995)
Only changes are some minor style-only test-only fixups.

re-ACK 2e33dbadcf69838c8ea591dccdc666f809c3134a 🔰

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpw
...

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