💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1669341194)
> > As I and others have explained above in the linked resources, eg https://petertodd.org/2023/fullrbf-multiparty-protocols and https://petertodd.org/2023/why-you-should-run-mempoolfullrbf, it is impossible to eliminate the need for transaction replacement.
>
> After reading your blog posts and thinking about it for the day, I've got some questions about it if you don't mind. One of the issues you write about is when a CoinJoin input is spent before the round is complete causing nodes to see
...
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1669341194)
> > As I and others have explained above in the linked resources, eg https://petertodd.org/2023/fullrbf-multiparty-protocols and https://petertodd.org/2023/why-you-should-run-mempoolfullrbf, it is impossible to eliminate the need for transaction replacement.
>
> After reading your blog posts and thinking about it for the day, I've got some questions about it if you don't mind. One of the issues you write about is when a CoinJoin input is spent before the round is complete causing nodes to see
...
💬 petertodd commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1669342850)
> NACK
>
> Clearly, some are going to be adversely affected by this PR if it gets merged.
Citation needed.
You need to provide actual examples of services that are going to be negatively affected. Simply claiming that is not enough. No-one has provided such examples in this pull-req or on the bitcoin-dev mailing list.
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1669342850)
> NACK
>
> Clearly, some are going to be adversely affected by this PR if it gets merged.
Citation needed.
You need to provide actual examples of services that are going to be negatively affected. Simply claiming that is not enough. No-one has provided such examples in this pull-req or on the bitcoin-dev mailing list.
💬 RicYashiroLee commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1669350250)
> 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
...
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1669350250)
> 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
...
💬 josibake commented on pull request "Silent Payments: sending":
(https://github.com/bitcoin/bitcoin/pull/28201#discussion_r1286925630)
This does not need to be a struct, it can just be a function.
(https://github.com/bitcoin/bitcoin/pull/28201#discussion_r1286925630)
This does not need to be a struct, it can just be a function.
💬 dergoegge commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1286929692)
I'm OK with going either way but disallowing uint256 comparisons in this PR causes a lot of churn, i.e. going around casting all the uint256 to the right type. Most of these casts would later be reverted in follow up PRs that convert more things to use the new types (e.g. mempool, net processing, wallet).
I think I'd prefer doing the switch to the new type system first and then disallowing uint256 comparisons afterwards. What do you think?
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1286929692)
I'm OK with going either way but disallowing uint256 comparisons in this PR causes a lot of churn, i.e. going around casting all the uint256 to the right type. Most of these casts would later be reverted in follow up PRs that convert more things to use the new types (e.g. mempool, net processing, wallet).
I think I'd prefer doing the switch to the new type system first and then disallowing uint256 comparisons afterwards. What do you think?
💬 MarcoFalke commented on pull request "wallet: Implement independent BDB parser":
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1669359482)
Can you give more context? What are the steps to reproduce? My understanding is that the fuzz CI tasks don't install and use bdb. And you can put a comment in the fuzz test to document that running this with bdb can be done at the users own risk. Outside of that, my recommendations would be to use bdb-5.3, which fixed a few bugs (or maybe it is possible to use an even later version, just for fuzzing?).
(https://github.com/bitcoin/bitcoin/pull/26606#issuecomment-1669359482)
Can you give more context? What are the steps to reproduce? My understanding is that the fuzz CI tasks don't install and use bdb. And you can put a comment in the fuzz test to document that running this with bdb can be done at the users own risk. Outside of that, my recommendations would be to use bdb-5.3, which fixed a few bugs (or maybe it is possible to use an even later version, just for fuzzing?).
💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286930916)
This doesn't need to be a struct, could just be functions which the DescScriptPubKeyMan calls as needed
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286930916)
This doesn't need to be a struct, could just be functions which the DescScriptPubKeyMan calls as needed
💬 MarcoFalke commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1286931397)
No opinion. I just wanted to mention that this is not type safe and maybe other reviewers would want to know too?
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1286931397)
No opinion. I just wanted to mention that this is not type safe and maybe other reviewers would want to know too?
💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286935217)
this function should be "label aware," meaning if the wallet implements labels later on, this function does not need to change.
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286935217)
this function should be "label aware," meaning if the wallet implements labels later on, this function does not need to change.
💬 dergoegge commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1286935288)
> The "opposite" type would be any type other than the type itself.
I meant opposite w.r.t `has_witness`, if you have a better name in mind, let me know.
> I just wanted to mention that this is not type safe and maybe other reviewers would want to know too?
I'll just add a todo comment about uint256 comparisons.
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1286935288)
> The "opposite" type would be any type other than the type itself.
I meant opposite w.r.t `has_witness`, if you have a better name in mind, let me know.
> I just wanted to mention that this is not type safe and maybe other reviewers would want to know too?
I'll just add a todo comment about uint256 comparisons.
💬 MarcoFalke commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1286938038)
Maybe something like this: (no idea if it compiles):
```cpp
template<typename Other>
constexpr int Compare(const Other& other) const {
if contexpr (std::is_same_v<Other, uint256> || \\ TODO remove this
std::is_save_v<Other, transaction_identifier<has_witness>>
){
return reinterpret_cast<const uint256&>(*this).Compare(other);
}else{
static_assert(ALWAYS_FALSE<Other>, "Forbidden comparison type");
}
}
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1286938038)
Maybe something like this: (no idea if it compiles):
```cpp
template<typename Other>
constexpr int Compare(const Other& other) const {
if contexpr (std::is_same_v<Other, uint256> || \\ TODO remove this
std::is_save_v<Other, transaction_identifier<has_witness>>
){
return reinterpret_cast<const uint256&>(*this).Compare(other);
}else{
static_assert(ALWAYS_FALSE<Other>, "Forbidden comparison type");
}
}
💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286941048)
This shouldn't return private keys, it should only return the tweak portion of the private key. This also means `Recipient` doesn't need to have the spend private key.
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286941048)
This shouldn't return private keys, it should only return the tweak portion of the private key. This also means `Recipient` doesn't need to have the spend private key.
💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286941460)
this should be its own commit
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286941460)
this should be its own commit
💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286941957)
this should be its own commit, might not be needed if we instead V0SilentPaymentDestination as a CTxDestination type
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1286941957)
this should be its own commit, might not be needed if we instead V0SilentPaymentDestination as a CTxDestination type
📝 Crypt-iQ converted_to_draft a pull request: "p2p: ensure mapBlockSource is removed from in ProcessBlock"
(https://github.com/bitcoin/bitcoin/pull/28235)
If we receive a valid block that passes the anti-DoS threshold but doesn't advance the tip, the BlockChecked callback won't be called. This is because ActivateBestChain will return early since pindexMostWork is equal to m_chain.Tip(). Since the BlockChecked callback isn't called, mapBlockSource won't be removed from. Fix that by always removing from mapBlockSource in ProcessBlock.
(https://github.com/bitcoin/bitcoin/pull/28235)
If we receive a valid block that passes the anti-DoS threshold but doesn't advance the tip, the BlockChecked callback won't be called. This is because ActivateBestChain will return early since pindexMostWork is equal to m_chain.Tip(). Since the BlockChecked callback isn't called, mapBlockSource won't be removed from. Fix that by always removing from mapBlockSource in ProcessBlock.
💬 RicYashiroLee commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1669394875)
> Bitcoin is purely a layer for storage of data that, typically, represents financial transactions. We have witnessed for years that there is no reasonable way to prevent the storage of abitrary data (any system that allows user input of 'random' values should accept the inevitability of data storage, "the signal is the noise").
>
> The internet became the ubiquitous system that it is today not because some weak-willed soul decided for the users that they needed to be protected by limiting th
...
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1669394875)
> Bitcoin is purely a layer for storage of data that, typically, represents financial transactions. We have witnessed for years that there is no reasonable way to prevent the storage of abitrary data (any system that allows user input of 'random' values should accept the inevitability of data storage, "the signal is the noise").
>
> The internet became the ubiquitous system that it is today not because some weak-willed soul decided for the users that they needed to be protected by limiting th
...
💬 petertodd commented on pull request "Remove -mempoolfullrbf option":
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1669412860)
> 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 whichever miners Peter can persuade to run code they dont really care about anyway.
FYI excluding Luxor, I haven't had any significant communications with any mining pools. I of course attempted to contact every mining pool I could find contact details for. But other than two I didn't get any responses back, and
...
(https://github.com/bitcoin/bitcoin/pull/26525#issuecomment-1669412860)
> 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 whichever miners Peter can persuade to run code they dont really care about anyway.
FYI excluding Luxor, I haven't had any significant communications with any mining pools. I of course attempted to contact every mining pool I could find contact details for. But other than two I didn't get any responses back, and
...
👍 MarcoFalke approved a pull request: "crypto: more `Span<std::byte>` modernization & follow-ups"
(https://github.com/bitcoin/bitcoin/pull/28100#pullrequestreview-1535733421)
nice ACK 153a6745fa523a3fd0ccce1304ceb75d4511df20 🔖
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: nice ACK 153a6745fa523a3f
...
(https://github.com/bitcoin/bitcoin/pull/28100#pullrequestreview-1535733421)
nice ACK 153a6745fa523a3fd0ccce1304ceb75d4511df20 🔖
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: nice ACK 153a6745fa523a3f
...
💬 MarcoFalke commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1267199359)
nit in 9e1199c7ce5d9a29c6ddf3dc28d8814bb1190a64: Can replace `std::byte{}` with `{}`, if it compiles, same below?
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1267199359)
nit in 9e1199c7ce5d9a29c6ddf3dc28d8814bb1190a64: Can replace `std::byte{}` with `{}`, if it compiles, same below?
💬 MarcoFalke commented on pull request "crypto: more `Span<std::byte>` modernization & follow-ups":
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1267206399)
9e1199c7ce5d9a29c6ddf3dc28d8814bb1190a64: Could replace this whole function with:
```cpp
fillrand(MakeWritableByteSpan(ret));
(https://github.com/bitcoin/bitcoin/pull/28100#discussion_r1267206399)
9e1199c7ce5d9a29c6ddf3dc28d8814bb1190a64: Could replace this whole function with:
```cpp
fillrand(MakeWritableByteSpan(ret));