💬 ryanofsky commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1828424828)
re: https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1822780992

> Should this be removed?

I think it's useful to document the format of the specifier, and it's still true that the remainder of the specifier (length and type) is not checked. Happy to update this if there's a specific suggestion, but I think it's still helpful and accurate so would not want to remove it.

💬 ryanofsky commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1828424763)
re: https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1822782509

> (One could expect that at least one precision-digit was required after '.' but it is not in tinyformat so this behavior is consistent).

That's good to know, I was just trying to make the parsing as simple as possible, but this syntax does seem to be commonly accepted (it works in python too) and I added test cases for it following your other suggestion

💬 ryanofsky commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1828424528)
re: https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1823488811

> ```c++
> PassFmt<1>("%.f");
> ```

Nice suggestion, added two cases: `%5.f` and `%.f` below `%5.2f`

💬 ryanofsky commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1828424659)
re: https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1822308585

Thanks, updated

💬 ryanofsky commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1828438077)
re: https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1823481424

> Why not use `FormatStringCheck<sizeof...(Args)>` in `printf` and `printfln` directly below as well?

No reason, added now.

🤔 achow101 reviewed a pull request: "test: Add `leaf_version` parameter to `taproot_tree_helper()`"
(https://github.com/bitcoin/bitcoin/pull/29371#pullrequestreview-2414278216)
Concept ACK

Please cleanup your commit message, it contains message fragments from squashed commits.

💬 achow101 commented on pull request "test: Add `leaf_version` parameter to `taproot_tree_helper()`":
(https://github.com/bitcoin/bitcoin/pull/29371#discussion_r1828489955)
nit:

add a space after the comma.

💬 ismaelsadeeq commented on pull request "test: enable running independent functional test sub-tests":
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1828498975)
Done, thanks for reviewing

💬 ismaelsadeeq commented on pull request "test: enable running independent functional test sub-tests":
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1828502796)
Done, thanks

💬 ismaelsadeeq commented on pull request "test: enable running independent functional test sub-tests":
(https://github.com/bitcoin/bitcoin/pull/30991#discussion_r1828505297)
Some tests might not have the test prefix, though. I don’t think we can use the `test_` prefix to catch all sub-tests; rather, the goal is just to enable running individual methods that can be sub-tests.

👍 ryanofsky approved a pull request: "test: Prove+document ConstevalFormatString/tinyformat parity"
(https://github.com/bitcoin/bitcoin/pull/30933#pullrequestreview-2414300187)
Code review ACK a911efc6e868ab1a23d05ee8ab3e94f7ecb89e2b. I didn't dig into previous discussion and it seems possible another approach to extending the tests might have advantages over this one. But the code change looks good and seems like an easy way of leveraging existing test cases to check for compatibility with tinyformat.

💬 ryanofsky commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1828504699)
In commit "test: Prove+document ConstevalFormatString/tinyformat parity" (a911efc6e868ab1a23d05ee8ab3e94f7ecb89e2b)

The name `TfmF` doesn't have any clear meaning to me, and usage of the function is also complicated by the need to use tuple_cat everywhere. I think the code would be clearer if the function were less generic and did not require passing a tuple. Maybe would suggest:

<details><summary>diff</summary>
<p>

```diff
--- a/src/test/util_string_tests.cpp
+++ b/src/test/util_str
...

💬 ryanofsky commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#discussion_r1828510180)
In commit "test: Prove+document ConstevalFormatString/tinyformat parity" (a911efc6e868ab1a23d05ee8ab3e94f7ecb89e2b)

Maybe drop WrongNumArgs and just pass CorrectArgs? Unless I'm missing something it seems like PassFmtIncorrect could choose the wrong number of args arbitrarily like PassFmt does.

💬 achow101 commented on pull request "optimization: Speed up Base58 encoding/decoding by 400%/200% via preliminary byte packing":
(https://github.com/bitcoin/bitcoin/pull/29473#issuecomment-2455881092)
As this is basically a complete rewrite of the base58 encoding and decoding code, I don't think the review effort required to review this is worth it relative to the unimportance of these functions.

💬 faisalazuredev commented on pull request "Update secp256k1 subtree to v0.6.0":
(https://github.com/bitcoin/bitcoin/pull/31216#issuecomment-2455983357)
merge it

👍 theStack approved a pull request: "Ephemeral Dust"
(https://github.com/bitcoin/bitcoin/pull/30239#pullrequestreview-2414414598)
ACK 131bed19bdfc922328cad9781fa9122b6810a8fd

💬 theStack commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1828578790)
nit: should ideally be already part of the commit that introduces this function for minimal diff (here and in header)

💬 theStack commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1828577696)
nit: missing closing double quote

📝 secp512k2 opened a pull request: "doc: Fix word order in developer-notes.md"
(https://github.com/bitcoin/bitcoin/pull/31220)
This pull request fixes a word order error in developer-notes.md.

Before:

"In cases where do you call .c_str(), you might want to additionally check that the string does not contain embedded '\0' characters..."

After:

"In cases where you do call .c_str(), you might want to additionally check that the string does not contain embedded '\0' characters..."

Explanation:

The sentence had incorrect word order, making it grammatically incorrect. Rearranging "do you" to "you do" correct
...

👍 rkrux approved a pull request: "test: enable running independent functional test sub-tests"
(https://github.com/bitcoin/bitcoin/pull/30991#pullrequestreview-2414999263)
ACK e96823bdcc2317fbf0bed64e53a1b3e5bb8f00f6

Thanks for quickly addressing the feedback.