💬 1ma commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1669092536)
Fair, I assumed they worked like v1 notifications. Then they don't have the footprint I thought (and now I understand why it's better than v1's, like you told me on Twitter), but I hope my message conveyed that I'm not specifically concerned about BIP47 v3 (which I see as legit), but more generally about leaving the door open to easily embed data in the UTXO set:

> You should have more leeway in 2023 to redesign BIP47 v3 on top of OP_RETURN than I'll have in 2050 to run a full node **if bare
...

💬 pajasevi commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1669145950)
v3 payment codes to be redesigned on top OP_RETURN would just mean switching back to v1 scheme. You would understand that if you actually understood those specifications.

I didn't come here to debate the nuances of "banning" certain transactions on P2P layer because, frankly, I don't agree with the premise. I came here to refute the nonsense that was claimed by people who should know better. And since I don't agree with the premise, the execution and the results, I totally NACK this.

💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#issuecomment-1669179392)
Anything left to do here, for a CI-only pull request with review?

💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1286809420)
I thought it would be more fun for reviewers if they could ACK the first use of the `mutable const` keyword in this codebase. Also, the value will be `nullptr` before and after the call to `HandleRequest`, so it seems *almost* `const` to me, but happy to change.

💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1286809802)
Nice, used your doc string. Thanks

💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1286811085)
> This implementation only supports being called on parameters with a RPCArg::Default fallback.

sorry, I was missing the early return `if (!arg.isNull()) return arg;`. Fixed now. Good catch!

💬 MarcoFalke commented on pull request "rpc: Add Arg() default helper":
(https://github.com/bitcoin/bitcoin/pull/28230#discussion_r1286815104)
> `std::optional`

Not sure. This requires all args to be copied again, which can be expensive, if the arg is a hex-encoded block. (https://github.com/bitcoin/bitcoin/pull/20017/files) had the same issue, btw.

I wrote some code to implement `std::string` as well (without a copy).

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1286821141)
This is wrong. The argument of `WalletLogPrintf` is never a string literal (the parameter is).

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1286821403)
Fixed in https://github.com/bitcoin/bitcoin/pull/28237

💬 MarcoFalke commented on pull request "RPC/Wallet: Access wallets via interfaces::Wallet":
(https://github.com/bitcoin/bitcoin/pull/26082#issuecomment-1669223402)
cc @luke-jr

💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286843620)
rename to TweakAdd to more closely mirror the secp function

💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286844417)
this should also be rename since we aren't hashing

💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286847829)
Should probably rename this to just "UnhashedECDH"

💬 ChrisMartl commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1669240273)
> I hate to break it to you all. Bitcoin is primary a data layer and secondary a monetary layer. The data of transactions is worth spending to bribe miners for inclusion. However if some arbitrary data you disagree with gets inclusion and attestation and still follows the rules of consensus its a valid transaction. To censor any transaction that follows consensus is censorship.
>
> Bitcoin is about uncensorable speech be it a monetary transaction or arbitrary data for someone willing to spend
...

💬 MarcoFalke commented on pull request "Bugfix: Skip tests for tools not being built":
(https://github.com/bitcoin/bitcoin/pull/23027#issuecomment-1669242771)
May want to close this, given the ongoing cmake transition obsoletes it either way?

💬 MarcoFalke commented on pull request "guix: switch from `guix environment` to `guix shell`":
(https://github.com/bitcoin/bitcoin/pull/26077#issuecomment-1669247689)
lgtm, but you'll have to document the minimum guix version to be 1.4, no?

💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286857436)
move this to the UnhashedECDH function

👍 MarcoFalke approved a pull request: "Add Signet launch shortcut for Windows"
(https://github.com/bitcoin/bitcoin/pull/26334#pullrequestreview-1566900510)
Seems fine. Left comments

💬 MarcoFalke commented on pull request "Add Signet launch shortcut for Windows":
(https://github.com/bitcoin/bitcoin/pull/26334#discussion_r1286867678)
Can just reuse the testnet icon to save 43.4kB in the git repo? Doesn't seem worth it to burden every clone forever just so that a color in a test-only picture is of a slightly different tone.

💬 MarcoFalke commented on pull request "Add Signet launch shortcut for Windows":
(https://github.com/bitcoin/bitcoin/pull/26334#discussion_r1286865806)
Can remove the `64-bit` here and above?