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

💬 MarcoFalke commented on pull request "mempool / miner: regularly flush <=0-fee entries and mine everything in the mempool":
(https://github.com/bitcoin/bitcoin/pull/27018#issuecomment-1669268690)
Needs rebase if still relevant?

💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286873697)
Wrap this function in a more generic "Decode" function that doesn't have size restrictions, possibly use an ENUM for indicating what sort of error guarantees you want.

💬 MarcoFalke commented on pull request "doc: clarify that LOCK() does AssertLockNotHeld() internally":
(https://github.com/bitcoin/bitcoin/pull/27116#issuecomment-1669288503)
I don't see the point of changing this, unless you also make the stderr output the same?

💬 theStack commented on pull request "bench: add readblock benchmark":
(https://github.com/bitcoin/bitcoin/pull/26684#discussion_r1286885333)
Agree that my suggestion is non-blocking, this is obviously much less critical for benchmarks and tests as opposed to production code. Having it enforced on the CI would be very nice indeed (I might look into how well this works with clang-tidy and if it'd also catch our own assertion-variants), I personally wouldn't remove the section from the docs though.