💬 TheCharlatan commented on pull request "refactor: Remove unused boost signals2 from torcontrol":
(https://github.com/bitcoin/bitcoin/pull/28240#discussion_r1287296853)
Maybe change this to "These messages could be used to dispatch async notifications to an async handler"?

💬 MarcoFalke commented on pull request "refactor: Remove unused boost signals2 from torcontrol":
(https://github.com/bitcoin/bitcoin/pull/28240#discussion_r1287321938)
I wasn't sure if the includes must come in a specific order. I've pushed a new commit to re-order them, which can be easily reverted, if there are issues.

💬 MarcoFalke commented on pull request "Avoid lock order inversion in `Chainstate::ConnectTip` function":
(https://github.com/bitcoin/bitcoin/pull/27684#issuecomment-1669917794)
Maybe `m_tx_inventory_mutex` can be released before `pool.cs` to break that link instead?

💬 theStack commented on pull request "bench: add readblock benchmark":
(https://github.com/bitcoin/bitcoin/pull/26684#discussion_r1287370102)
Okay, that makes sense. My blind guess was that clang-tidy (or any other similar tool) would only look at the passed expression and not care about the effect of the assert as a whole. Seems like even the passed expression is not side-effect free at several places in the code, e.g.
https://github.com/bitcoin/bitcoin/blob/b565485c24c0feacae559a7f6f7b83d7516ca58d/src/init.cpp#L1428

so it indeed might be better to remove recommendations that we don't even follow ourselves.

(Also, I sadly can'
...

💬 theuni commented on pull request "refactor: Enforce C-str fmt strings in WalletLogPrintf()":
(https://github.com/bitcoin/bitcoin/pull/28237#discussion_r1287382437)
Hmm, this one wasn't being caught before?

🤔 theuni reviewed a pull request: "refactor: Enforce C-str fmt strings in WalletLogPrintf()"
(https://github.com/bitcoin/bitcoin/pull/28237#pullrequestreview-1567718554)
Makes sense to me. I almost did the same thing while poking at `WalletLogPrintf` for the sake of easier static analysis.

Am I missing some obvious reason though, other than "might as well"?

💬 theuni commented on pull request "refactor: Enforce C-str fmt strings in WalletLogPrintf()":
(https://github.com/bitcoin/bitcoin/pull/28237#discussion_r1287387071)
I was going to suggest the `nonnull` attribute here, but I believe our tidy check actually enforces that :)

💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1287394228)
As long as testnet and signet are the same, and different from mainnet, I'm happy. When testing with e.g. hardware wallets it lets you use the testnet app / firmware instead of signet-specific stuff.

💬 MajesticBank commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1669963434)
#1 At this point I will assume Daniel600 is not just spreading lies but obviously pressured / paid to promote lower privacy on bitcoin blockchain.

There is no service on internet that accept 0-conf bitcoin transaction, end.

I have 10 years experience in this industry and running crypto-instant swap exchange for 2+ years https://majesticbank.sc/

Made over 500,000+ transactions using Coinpayments, Coinspaid and Changelly + fixedfloat, changenow and many more instant-swap sites, no servi
...

💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1287398956)
37b0ca1c3d753cc61e0debf8ea59160bdf4127ec: alternatively you could scan for the exact pubkey length. That would tolerate future versions to add extra stuff to the address that's safe to ignore for older versions.

💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1287401010)
37b0ca1c3d753cc61e0debf8ea59160bdf4127ec: could use `version == 0 && silent_payment_data.size() < SILENT_PAYMENT_V0_DATA_SIZE` (see comment above)

💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1287404996)
328951d056e939b6db573043fc71d8bd6ae68dff: this should be in some bech32 helper function.

💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1287404315)
328951d056e939b6db573043fc71d8bd6ae68dff: if cheap, could assert/assume both ` CPubKey` are valid.

💬 Sjors commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1287402078)
37b0ca1c3d753cc61e0debf8ea59160bdf4127ec `assert data.size() >= SILENT_PAYMENT_V0_DATA_SIZE` (or early return if you don't expect the caller to have already checked this)

💬 MarcoFalke commented on pull request "refactor: Enforce C-str fmt strings in WalletLogPrintf()":
(https://github.com/bitcoin/bitcoin/pull/28237#discussion_r1287411898)
I hope that the tests (and reviewers) will catch a `LogPrint(nullptr, 1, 2, 3)`. But adding `nonnull` can be done in a follow-up, if needed, for all log-functions.

💬 MarcoFalke commented on pull request "refactor: Enforce C-str fmt strings in WalletLogPrintf()":
(https://github.com/bitcoin/bitcoin/pull/28237#discussion_r1287412409)
Yes, see https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1286821141

💬 TheCharlatan commented on pull request "refactor: Remove unused boost signals2 from torcontrol":
(https://github.com/bitcoin/bitcoin/pull/28240#issuecomment-1669986445)
Re-ACK faaba770e11352ddf6414b9855f4baa46a967580

💬 MarcoFalke commented on pull request "refactor: Enforce C-str fmt strings in WalletLogPrintf()":
(https://github.com/bitcoin/bitcoin/pull/28237#issuecomment-1669987229)
> Am I missing some obvious reason though, other than "might as well"?

Edited OP to list both reasons.

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1287425096)
This is wrong, too. `ScriptPubKeyMan` has a log statement, too.

My recommendation would be to just remove this line completely, unless there is a reason to have it. The other lines should be exact enough to match everything that is needed, without under- or over-matching.

💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1287432683)
Big fan of this rewrite, makes it pretty extensible without too much code duplication. Nice!

Building on your new approach, the non-default parameters could potentially be handled with pointers, to avoid unnecessary copy operations. For example, I think these 2 lines: https://github.com/bitcoin/bitcoin/blob/b565485c24c0feacae559a7f6f7b83d7516ca58d/src/wallet/rpc/spend.cpp#L274-L275

can be rewritten as:
```cpp
if (auto comment{self.Arg<const std::string*>(2)}; comment && !(comment->
...