💬 jonatack commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1819693843)
> Please format affected code, don't leave it unformatted, that's why the current code is so confusing.

The indentation wasn't wrong, and in the past, long-term contributors generally or often didn't wish to reformat code in their pulls that wasn't wrong in some dangerous way when newer contributors asked for reformatting in reviews. There has been extensive bikeshedding over what the clang-format setup here should even do. Having these arguments on the same line read left-to-right, which was
...

🤔 mzumsande reviewed a pull request: "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior"
(https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2400135259)
Concept ACK

Does this PR have the aspiration to fix all instances where it makes sense?

If so, I think that the pattern from 40ecf91068545ba55a6af5e2536a61c459334925 could also be applied to `-port` and `-rpcport` in `CheckHostPortOptions()` or `-asmap`, which all fail init in master when negated and which could be used to negate earlier options if changed.

💬 l0rinc commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1819707701)
There was no bikeshedding about this before on https://github.com/bitcoin/bitcoin/pull/31149, it just started.

💬 jonatack commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1819737341)
> By manual testing do you mean automated testing of print statements?

No, manual testing by building and running the CLI commands.

There is no automated test coverage of `-netinfo` (the touched code above), nor, for instance, of `-addrinfo`.

👍 l0rinc approved a pull request: "tinyformat: enforce compile-time checks for format string literals"
(https://github.com/bitcoin/bitcoin/pull/31149#pullrequestreview-2400193814)
Compared to https://github.com/bitcoin/bitcoin/pull/30928 I see the following changes:
* clientversion.cpp: Merge conflict in clientversion at CopyrightHolders -> Seems to be resolved correctly
* logging.h: old LogPrintFormatInternal is retained
* run-lint-format-strings.py: FALSE_POSITIVES not removed
* util_string_tests.cpp: FALSE_POSITIVES not moved to tests
* strprintf.cpp: fuzz_fmt added to avoid renames, try-catch added around fuzz_fmt calls (any reason for not try-catching fuzz_fmt i
...

💬 l0rinc commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1819746061)
By doing some of the validation at compile time, there **is** now some automated 'test coverage' on it - i.e. it will bite back if it's very wrong without any additional manual check.

💬 jonatack commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1819751107)
Generally unneeded changes can be avoided, but that was just a suggestion. Further historical context to get started:

- https://github.com/bitcoin/bitcoin/issues/18651
- https://github.com/bitcoin/bitcoin/issues/15465

💬 l0rinc commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1819774471)
Useful context, thanks!
We must however keep our kitchen clean, we can't just be cooking all the time.
This change by @stickies-v was a small, low-risk change, making the code slightly more maintainable (or at least slightly less unmaintainable).
We should encourage these.

💬 hodlinator commented on pull request "Windows bitcoind stall debugging [NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2442699537)
> Can this be closed, or is it waiting on something?

I'd like to keep it open as I explore less nuclear solutions. Pushed an initial attempt to here and my personal *master*: https://github.com/hodlinator/bitcoin/compare/84cd6478c422bc296589ab031f5c76e7bab2704d...f69a238bcec12eb9fc2d8a13264fa471e06a4d20

💬 TheCharlatan commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#issuecomment-2442701324)
Rebased f55d36a19c61e0ec8c06a6e5a0bbae455c8a1d06 -> f55d36a19c61e0ec8c06a6e5a0bbae455c8a1d06 ([blockmanDB](https://github.com/TheCharlatan/bitcoin/tree/blockmanDB) -> [blockmanDB](https://github.com/TheCharlatan/bitcoin/tree/blockmanDB), [compare](https://github.com/TheCharlatan/bitcoin/compare/blockmanDB..blockmanDB))

💬 sipsorcery commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2442705654)
Concept ACK.

tACK 49994fadb3b71552fd99acd8671c52e08284b5e9 on Win11 23H2 msvc (via cmake).

👍 tdb3 approved a pull request: "key: clear out secret data in `DecodeExtKey`"
(https://github.com/bitcoin/bitcoin/pull/31166#pullrequestreview-2400342243)
cr ACK 559a8dd9c0aafcecf00f9ccd9aabe5720bcebe8c

Good catch

💬 darosior commented on pull request "Cleanups to port mapping module post UPnP drop":
(https://github.com/bitcoin/bitcoin/pull/31157#issuecomment-2442767981)
Rebased after #31130 got merged.

👋 darosior's pull request is ready for review: "Cleanups to port mapping module post UPnP drop"
(https://github.com/bitcoin/bitcoin/pull/31157)

💬 fanquake commented on pull request "Windows bitcoind stall debugging [NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2442781213)
> I'd like to keep it open as I explore less nuclear solutions. Pushed an initial attempt to here and my personal master: hodlinator/bitcoin@84cd647...f69a238

In my opinion your (already merged) change to drop the offending code from the RNG is fine, and isn't really something we need to investigate further / work on reintroducing.

💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1819836232)
I'm afraid `FuzzedDataProvider::remaining_bytes_` becomes zero after this line in the current version, meaning `provider.ConsumeIntegralInRange<int>(0, decoded.size() - 1)` below will always return the minimum value if I'm reading the `ConsumeIntegralInRange`-implementation correctly.

```suggestion
const std::string random_string{provider.ConsumeRandomLengthString(101)};
```
---

Could maybe (in another PR) add something like `bool FuzzedDataProvider::consume_remaining_called_{false}
...

💬 cobratbq commented on issue "Docs: "External signer" documentation is outdated. (plz close if unwanted request)":
(https://github.com/bitcoin/bitcoin/issues/31005#issuecomment-2442820613)
When using `--stdin`, there is `1` command, and `2` variations of input (so far):

- `signtx` (space-separated)
- _Bitcoin Core v27.x_: `"<base64-encoded (binary) PSBT>"` (with double-quotes literally present and tag between `<` and `>` replaced with its representative content)
- _Bitcoin Core v28.x_: `<base64-encoded (binary) PSBT>` (without double-quotes, same PSBT representation)

📝 ryanofsky opened a pull request: "tinyformat: Add compile-time checking for literal format strings"
(https://github.com/bitcoin/bitcoin/pull/31174)
This PR is an alternative to #31149 that **only** adds compile-time checking for literal format strings and does not make other changes in the codebase.

💬 ryanofsky commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#issuecomment-2442899726)
Sorry for causing unnecessary churn with my earlier comments. After reading Marco's comment in https://github.com/bitcoin/bitcoin/pull/31072#discussion_r1801085975 I finally understand what this PR is really trying to do, and why all the changes are being made here. Before reading that comment I thought this PR was trying not just to add compile-time checking for tinyformat format strings, but also to force calls that don't use compile-time checking to use different formatting functions like `fo
...

💬 maflcko commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1820218726)
> Could maybe (in another PR)

In theory it could also be caught at compile time with https://clang.llvm.org/docs/AttributeReference.html#consumed-annotation-checking