💬 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

👍 rkrux approved a pull request: "test: enhance p2p_orphan_handling"
(https://github.com/bitcoin/bitcoin/pull/31037#pullrequestreview-2400959381)
tACK 9de9c858d5aa412e1c6086e564fbe85984a4f2d5

Successful make and all functional tests. This is my preference as well to use the `getorphantxs` RPC for asserting over searching in the debug logs.

💬 laanwj commented on pull request "Windows bitcoind stall debugging [NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2443481677)
> isn't really something we need to investigate further / work on reintroducing.

Agree, it's fine, it's good to get rid of the perfmon query.

💬 maflcko commented on pull request "wallet, assumeutxo: Don't Assume m_chain_tx_count, Improve wallet RPC errors":
(https://github.com/bitcoin/bitcoin/pull/30909#discussion_r1820289265)
nit: For new code it would be good to use the alias `chainman().GetMutex()` member instead, when `chainman()` members are accessed.

💬 jarolrod commented on pull request "cmake: Add `FindQRencode` module and enable `libqrencode` package for MSVC":
(https://github.com/bitcoin/bitcoin/pull/31173#issuecomment-2443594716)
Concept ACK

💬 jarolrod commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#issuecomment-2443600588)
Concept ACK

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

🤔 jarolrod reviewed a pull request: "Decouple WalletModel from RPCExecutor"
(https://github.com/bitcoin-core/gui/pull/841#pullrequestreview-2401161662)
strong concept ack