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

💬 MarcoFalke commented on pull request "bench: add readblock benchmark":
(https://github.com/bitcoin/bitcoin/pull/26684#discussion_r1286889279)
>if it'd also catch our own assertion-variants)

They are *required* to have side-effects, as they must return the value that was passed in. So checking them is not possible and it seems better to consistently check nothing than to special case one function.

💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286904504)
Similar to "Decode", there should be a higher-level function that first decodes the string and then passes it to the relevant decoding function

💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286912005)
Even better, should probably first add a new destination type to CTxDestination

💬 BitcoinErrorLog commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1669339848)
I have not lost interest I simply do not have the technical expertise to maintain this, nor the patience to respond to newer gaslighting comments that were already addressed in earlier conversation.

My prediction that this feature would later be used to sneak in an "on" by default is already true too...

Not sure how all of you can be so comfortable with this kind of distortion of status quo, particularly when it serves no one but Peter Todd, some misguided wannabe Core Wizards, and whiche
...