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

💬 Subhra264 commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#discussion_r1287435261)
According to the BIP,
> Inputs not specifying a lock time field can take both types of lock times, as can those that specify both.

so if I am not missing anything here and both the `time_lock` and `height_lock` of `psbtin` are `std::nullopt`, shouldn't it be accepted irrespective of `input` lock times?

👍 stickies-v approved a pull request: "rpc: Add Arg() default helper"
(https://github.com/bitcoin/bitcoin/pull/28230#pullrequestreview-1567815521)
ACK fa5468dcf0109d11ddaeb4b3591e6e04b4ea6125, really like this!

💬 stickies-v commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1287446839)
nit: using `.at(i)` here and `[i]` just a few lines up, when I think they both have the same preconditions, so staying consistent is probably better

💬 glozow commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1287458787)
`std::vector<PackageEntry&>` isn't possible, but will do reference wrappers