💬 zkfrio commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666938033)
> PSA (please don't kill the messenger, I'm just adding context to the pr) this pr will likely break BIP47 payment codes, at [least version 3](https://github.com/OpenBitcoinPrivacyProject/rfc/blob/master/obpp-05.mediawiki#user-content-Notification_Output). I'm not saying this is good or bad, but I think at least a "heads-up" will be a good practice if this goes through.
BIP 47 uses OP_RETURN
If anything else, we need their developers to comment else nobody cares about a project who is mak
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666938033)
> PSA (please don't kill the messenger, I'm just adding context to the pr) this pr will likely break BIP47 payment codes, at [least version 3](https://github.com/OpenBitcoinPrivacyProject/rfc/blob/master/obpp-05.mediawiki#user-content-Notification_Output). I'm not saying this is good or bad, but I think at least a "heads-up" will be a good practice if this goes through.
BIP 47 uses OP_RETURN
If anything else, we need their developers to comment else nobody cares about a project who is mak
...
💬 1ma commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666938634)
> PSA (please don't kill the messenger, I'm just adding context to the pr) this pr will likely break BIP47 payment codes, at [least version 3](https://github.com/OpenBitcoinPrivacyProject/rfc/blob/master/obpp-05.mediawiki#user-content-Notification_Output). I'm not saying this is good or bad, but I think at least a "heads-up" will be a good practice if this goes through.
This is IMO a valid objection. But to be precise BIP47 payments code are not broken as they use OP_RETURN, and v3 has not be
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666938634)
> PSA (please don't kill the messenger, I'm just adding context to the pr) this pr will likely break BIP47 payment codes, at [least version 3](https://github.com/OpenBitcoinPrivacyProject/rfc/blob/master/obpp-05.mediawiki#user-content-Notification_Output). I'm not saying this is good or bad, but I think at least a "heads-up" will be a good practice if this goes through.
This is IMO a valid objection. But to be precise BIP47 payments code are not broken as they use OP_RETURN, and v3 has not be
...
💬 Davidson-Souza commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666943944)
> BIP 47 uses OP_RETURN
Version one does. Version two uses a 1 of 2 MS in a p2sh and version 3 uses p2ms.
> This is IMO a valid objection. But to be precise BIP47 payment codes are not broken as they use OP_RETURN, and v3 has not been rolled out yet (?). Is there any reason the draft couldn't be amended to build v3 on OP_RETURN, too?
This is not a draft, this is the final spec implemented by Samurai (I think Blue Wallet and Sparrow also does). They didn't update BIP047 because of some d
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666943944)
> BIP 47 uses OP_RETURN
Version one does. Version two uses a 1 of 2 MS in a p2sh and version 3 uses p2ms.
> This is IMO a valid objection. But to be precise BIP47 payment codes are not broken as they use OP_RETURN, and v3 has not been rolled out yet (?). Is there any reason the draft couldn't be amended to build v3 on OP_RETURN, too?
This is not a draft, this is the final spec implemented by Samurai (I think Blue Wallet and Sparrow also does). They didn't update BIP047 because of some d
...
💬 ajtowns commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1268944205)
The default for `ImportMempoolOptions::mockable_fopen_function` isn't actually used anywhere since `LoadMempool` overrides it. Maybe drop `LoadMempool` entirely and call `kernel::ImportMempool` directly from init.cpp and the fuzz test?
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1268944205)
The default for `ImportMempoolOptions::mockable_fopen_function` isn't actually used anywhere since `LoadMempool` overrides it. Maybe drop `LoadMempool` entirely and call `kernel::ImportMempool` directly from init.cpp and the fuzz test?
💬 ajtowns commented on pull request "rpc: Add importmempool RPC":
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1268950369)
Might be worth adding a helper for the `isNull ? default : get_native()` pattern?
```c++
template<typename T>
T RPCDefaultArg(const UniValue&, T default);
template<>
bool RPCDefaultArg(const UniValue& uv, bool default) { return uv.isNull() ? default : uv.get_bool(); }
const auto use_current_time = RPCDefaultArg<bool>(request.params[1]["use_current_time"], true);
```
(https://github.com/bitcoin/bitcoin/pull/27460#discussion_r1268950369)
Might be worth adding a helper for the `isNull ? default : get_native()` pattern?
```c++
template<typename T>
T RPCDefaultArg(const UniValue&, T default);
template<>
bool RPCDefaultArg(const UniValue& uv, bool default) { return uv.isNull() ? default : uv.get_bool(); }
const auto use_current_time = RPCDefaultArg<bool>(request.params[1]["use_current_time"], true);
```
💬 zkfrio commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666967075)
>This is not a draft, this is the final spec implemented by Samurai (I think Blue Wallet and Sparrow also does). They didn't update BIP047 because of some disagreements between the developers and the BIPs repo. But this is not relevant here, the latest version, to my knowledge is v3 that uses p2ms.
1. Samourai- nobody cares about that project who is making money from others efforts.
2. None of them use P2MS, do some research.
Still NACK
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666967075)
>This is not a draft, this is the final spec implemented by Samurai (I think Blue Wallet and Sparrow also does). They didn't update BIP047 because of some disagreements between the developers and the BIPs repo. But this is not relevant here, the latest version, to my knowledge is v3 that uses p2ms.
1. Samourai- nobody cares about that project who is making money from others efforts.
2. None of them use P2MS, do some research.
Still NACK
💬 printer-jam commented on pull request "Remove arbitrary restrictions on OP_RETURN by default":
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1666976142)
> The individuals opposing this PR, believing they protect Bitcoin from centralization, ironically desire to dictate to all other people how they should and should not use Bitcoin.
Why create the PR to change Bitcoin? You are not stopped from using it within the current rules, yet you want to dictate to node runners that they must now store more arbitrary data, and if they don't agree they are the ones 'dictating' by opposing this PRs demands? Classic projection.
(https://github.com/bitcoin/bitcoin/pull/28130#issuecomment-1666976142)
> The individuals opposing this PR, believing they protect Bitcoin from centralization, ironically desire to dictate to all other people how they should and should not use Bitcoin.
Why create the PR to change Bitcoin? You are not stopped from using it within the current rules, yet you want to dictate to node runners that they must now store more arbitrary data, and if they don't agree they are the ones 'dictating' by opposing this PRs demands? Classic projection.
💬 samouraiwallet commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666981146)
Ignoring the unwarranted ad hominem.
I have no desire to wade into what appears to be a bunch of meddling with the code base for no good reason (bare multsig outputs are not the cause of the increased spam, as I am sure you know).
I will simply confirm that Samourai Wallet implements Version 1 of BIP47 which doesn't use bare multsig but rather OP_RETURN. There are at least 200,000 BIP47 payment codes (that users have opted in to uploading to a public web directory) in the wild across thre
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666981146)
Ignoring the unwarranted ad hominem.
I have no desire to wade into what appears to be a bunch of meddling with the code base for no good reason (bare multsig outputs are not the cause of the increased spam, as I am sure you know).
I will simply confirm that Samourai Wallet implements Version 1 of BIP47 which doesn't use bare multsig but rather OP_RETURN. There are at least 200,000 BIP47 payment codes (that users have opted in to uploading to a public web directory) in the wild across thre
...
💬 Davidson-Souza commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666991867)
@samouraiwallet Thank you for your clarifications. I retract myself in claiming Samurai uses v3.
However, as noted:
> I don't think it is wise to assume that no one is using BIP47 Version 3
My comment still stands on, this is a valid additional context to reviewers.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666991867)
@samouraiwallet Thank you for your clarifications. I retract myself in claiming Samurai uses v3.
However, as noted:
> I don't think it is wise to assume that no one is using BIP47 Version 3
My comment still stands on, this is a valid additional context to reviewers.
💬 furszy commented on pull request "test: create wallet specific for test_locked_wallet case":
(https://github.com/bitcoin/bitcoin/pull/28139#discussion_r1285273580)
> I wonder why implicit fee estimation passed in the first place. Previously there were two inputs and a fee of 550sat. Now there is a fee of 2200 sat with one input.
That is because the default wallets have a custom tx fee (set [here](https://github.com/bitcoin/bitcoin/blob/b7138252ace6d21476964774e094ed1143cd7a1c/test/functional/wallet_fundrawtransaction.py#L100)), which is much lower than the one used in the wallet created by this test case (which runs the default fee estimation).
Side
...
(https://github.com/bitcoin/bitcoin/pull/28139#discussion_r1285273580)
> I wonder why implicit fee estimation passed in the first place. Previously there were two inputs and a fee of 550sat. Now there is a fee of 2200 sat with one input.
That is because the default wallets have a custom tx fee (set [here](https://github.com/bitcoin/bitcoin/blob/b7138252ace6d21476964774e094ed1143cd7a1c/test/functional/wallet_fundrawtransaction.py#L100)), which is much lower than the one used in the wallet created by this test case (which runs the default fee estimation).
Side
...
💬 ariard commented on pull request "Libstandardness (edition 2023)":
(https://github.com/bitcoin/bitcoin/pull/28220#issuecomment-1667028032)
> This is not and should never become a thing. Every node has its own policies, and relying on any specific policy is broken by design.
While each node can set its own transaction relay policy, economically rational nodes are expected to follow the set of rules yielding the highest feerate traffic, at the very least for the ones partaking in block template construction if they wish to stay competitive mining entities.
If the default set of transaction relay rules do not yield the highest f
...
(https://github.com/bitcoin/bitcoin/pull/28220#issuecomment-1667028032)
> This is not and should never become a thing. Every node has its own policies, and relying on any specific policy is broken by design.
While each node can set its own transaction relay policy, economically rational nodes are expected to follow the set of rules yielding the highest feerate traffic, at the very least for the ones partaking in block template construction if they wish to stay competitive mining entities.
If the default set of transaction relay rules do not yield the highest f
...
💬 russeree commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1667063615)
Concept NACK
1. **This PR won't stop utxo set bloat.** This would not stop bloat of the UTXO set because P2PK still exists. P2MS is just P2PK with the ability to bake up to 2 more pubkeys to the chain even if those pubkeys don't actually have known private keys.
2. **Long term storage** because of the nature of P2SH and P2WSH scripts for multisig. The user is responsible for not only their private keys but also the public keys to recreate the script to spend their funds. P2MS is an incredi
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1667063615)
Concept NACK
1. **This PR won't stop utxo set bloat.** This would not stop bloat of the UTXO set because P2PK still exists. P2MS is just P2PK with the ability to bake up to 2 more pubkeys to the chain even if those pubkeys don't actually have known private keys.
2. **Long term storage** because of the nature of P2SH and P2WSH scripts for multisig. The user is responsible for not only their private keys but also the public keys to recreate the script to spend their funds. P2MS is an incredi
...
💬 mikeinspace commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1667066451)
Concept NACK but I would be very amused to see this get merged. We will engineer around it, of course.
We will go straight to mining pools if we have to (who LOVE to include our high fee transactions).
We will continue to use the permissionless Bitcoin protocol how we see fit. #FREEDOM
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1667066451)
Concept NACK but I would be very amused to see this get merged. We will engineer around it, of course.
We will go straight to mining pools if we have to (who LOVE to include our high fee transactions).
We will continue to use the permissionless Bitcoin protocol how we see fit. #FREEDOM
💬 Fiach-Dubh commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1667112926)
> Concept NACK
>
> 1. **This PR won't stop utxo set bloat.** This would not stop bloat of the UTXO set because P2PK still exists. P2MS is just P2PK with the ability to bake up to 2 more pubkeys to the chain even if those pubkeys don't actually have known private keys.
> 2. **Long term storage** because of the nature of P2SH and P2WSH scripts for multisig. The user is responsible for not only their private keys but also the public keys to recreate the script to spend their funds. P2MS is an i
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1667112926)
> Concept NACK
>
> 1. **This PR won't stop utxo set bloat.** This would not stop bloat of the UTXO set because P2PK still exists. P2MS is just P2PK with the ability to bake up to 2 more pubkeys to the chain even if those pubkeys don't actually have known private keys.
> 2. **Long term storage** because of the nature of P2SH and P2WSH scripts for multisig. The user is responsible for not only their private keys but also the public keys to recreate the script to spend their funds. P2MS is an i
...
💬 russeree commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1667128259)
> There's something really ironic about ruseree claiming this would be censorship, with the stamp creators comment afterwards saying his degens will just pay out of band as a work around.
>
> Which is fair, but also demonstrates this is not censorship.
I would make the point that it is a form of censorship. My analogy would be this is akin to the same way that social media platforms can make certain context and conversations incredibly difficult to propagate across their platforms. Instead
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1667128259)
> There's something really ironic about ruseree claiming this would be censorship, with the stamp creators comment afterwards saying his degens will just pay out of band as a work around.
>
> Which is fair, but also demonstrates this is not censorship.
I would make the point that it is a form of censorship. My analogy would be this is akin to the same way that social media platforms can make certain context and conversations incredibly difficult to propagate across their platforms. Instead
...
💬 RobinLinus commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1667208040)
> We will go straight to mining pools [...] who include our high fee transactions
Making the spam attacks more costly is the rationale behind this change. And the guy who runs the attack is trying to stop this spam mitigation, which is a strong indicator that he believes this change will be effective.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1667208040)
> We will go straight to mining pools [...] who include our high fee transactions
Making the spam attacks more costly is the rationale behind this change. And the guy who runs the attack is trying to stop this spam mitigation, which is a strong indicator that he believes this change will be effective.
💬 darosior commented on pull request "mempool: Persist with XOR":
(https://github.com/bitcoin/bitcoin/pull/28207#discussion_r1285419172)
Right, overlooked.
(https://github.com/bitcoin/bitcoin/pull/28207#discussion_r1285419172)
Right, overlooked.
💬 Empact commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1667258580)
Concept ACK
nit: IMO better to split the changes from the iwyu adds/removals in the spirit of the "[Pull Request Philosophy](https://github.com/bitcoin/bitcoin/blob/b7138252ace6d21476964774e094ed1143cd7a1c/CONTRIBUTING.md#pull-request-philosophy)"
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1667258580)
Concept ACK
nit: IMO better to split the changes from the iwyu adds/removals in the spirit of the "[Pull Request Philosophy](https://github.com/bitcoin/bitcoin/blob/b7138252ace6d21476964774e094ed1143cd7a1c/CONTRIBUTING.md#pull-request-philosophy)"
💬 MarcoFalke commented on pull request "test: create wallet specific for test_locked_wallet case":
(https://github.com/bitcoin/bitcoin/pull/28139#discussion_r1285427110)
yeah, if the fee was hardcoded before, it makes sense to keep it hardcoded. Especially if removing the hard code makes the test fail.
(https://github.com/bitcoin/bitcoin/pull/28139#discussion_r1285427110)
yeah, if the fee was hardcoded before, it makes sense to keep it hardcoded. Especially if removing the hard code makes the test fail.
💬 MarcoFalke commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1667263372)
The second commit is basically iwyu on `src/blockfilter.h` and `wallet.cpp/h`, so I don't think it can be split further.
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1667263372)
The second commit is basically iwyu on `src/blockfilter.h` and `wallet.cpp/h`, so I don't think it can be split further.