💬 theStack commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1285210302)
(in commit 077783d24b7f1e3d3312e6cc12aee3f178400725)
Looking at the "override calculated size with coin control specified size" branch removed below, seems like a `GetVirtualTransactionSize` call is missing here (in the case that `GetInputWeight` doesn't return std::nullopt), as we want vbytes as `input_bytes` unit, not WU?
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1285210302)
(in commit 077783d24b7f1e3d3312e6cc12aee3f178400725)
Looking at the "override calculated size with coin control specified size" branch removed below, seems like a `GetVirtualTransactionSize` call is missing here (in the case that `GetInputWeight` doesn't return std::nullopt), as we want vbytes as `input_bytes` unit, not WU?
💬 theStack commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1285213341)
(in commit 6fcbf9f44358f85aab4c0b6617c113b4c37da5a8)
nit: could be written as a one-liner
```suggestion
return (it != m_selected.end() && it->second.HasScripts());
```
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1285213341)
(in commit 6fcbf9f44358f85aab4c0b6617c113b4c37da5a8)
nit: could be written as a one-liner
```suggestion
return (it != m_selected.end() && it->second.HasScripts());
```
💬 theStack commented on pull request "wallet: Pass through transaction locktime and preset input sequences and scripts to CreateTransaction":
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1285211710)
(in commit 705568146d4399ad8089adc4943786e2a0c0a62e)
nit: could be written as a one-liner
```suggestion
return (it != m_selected.end()) ? it->second.GetSequence() : std::nullopt;
```
(https://github.com/bitcoin/bitcoin/pull/25273#discussion_r1285211710)
(in commit 705568146d4399ad8089adc4943786e2a0c0a62e)
nit: could be written as a one-liner
```suggestion
return (it != m_selected.end()) ? it->second.GetSequence() : std::nullopt;
```
💬 Semisol commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666868952)
Concept ACK
I do not see the negative impact of this change, as there is a heavy incentive to use P2WSH or P2SH due to the cost savings and/or wallets not being able to send to bare multisigs easily.
This also has the additional benefit of encouraging adoption of more widely supported standards and preventing spam.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666868952)
Concept ACK
I do not see the negative impact of this change, as there is a heavy incentive to use P2WSH or P2SH due to the cost savings and/or wallets not being able to send to bare multisigs easily.
This also has the additional benefit of encouraging adoption of more widely supported standards and preventing spam.
💬 ns-xvrn commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666885693)
Concept ACK
For the reasoning mentioned by achow101 and semisol. Avoiding spam is healthy for my full node.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666885693)
Concept ACK
For the reasoning mentioned by achow101 and semisol. Avoiding spam is healthy for my full node.
💬 josibake commented on pull request "Silent Payments: Implement BIP352":
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1285232819)
Thanks for the suggestion, I'll take a look at `EvalCheckSig`!
(https://github.com/bitcoin/bitcoin/pull/28122#discussion_r1285232819)
Thanks for the suggestion, I'll take a look at `EvalCheckSig`!
💬 fanquake commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1666916558)
@willcl-ark note that you ACK'd the wrong commit here.
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1666916558)
@willcl-ark note that you ACK'd the wrong commit here.
🚀 fanquake merged a pull request: "p2p: Diversify automatic outbound connections with respect to networks"
(https://github.com/bitcoin/bitcoin/pull/27213)
(https://github.com/bitcoin/bitcoin/pull/27213)
💬 zkfrio commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666921966)
> @zkfrio your statement is contradictory. If bare multisig inscribers can easily route around mempool policy then there's no reason to oppose the change. If you ACK this proposal everyone can have their slice of pie.
It is not contradictory if you understand how bitcoin works. This will create different mempool and P2P or incentivize a system that allows all transactions that follow consensus rules. It's a slippery slope.
If you think bare multisig is bad for multisig for bitcoin. Share o
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666921966)
> @zkfrio your statement is contradictory. If bare multisig inscribers can easily route around mempool policy then there's no reason to oppose the change. If you ACK this proposal everyone can have their slice of pie.
It is not contradictory if you understand how bitcoin works. This will create different mempool and P2P or incentivize a system that allows all transactions that follow consensus rules. It's a slippery slope.
If you think bare multisig is bad for multisig for bitcoin. Share o
...
💬 whycorn commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666923443)
> > @zkfrio your statement is contradictory. If bare multisig inscribers can easily route around mempool policy then there's no reason to oppose the change. If you ACK this proposal everyone can have their slice of pie.
>
> It is not contradictory if you understand how bitcoin works. This will create different mempool and P2P or incentivize a system that allows all transactions that follow consensus rules. It's a slippery slope.
>
> If you think bare multisig is bad for multisig for bitcoi
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666923443)
> > @zkfrio your statement is contradictory. If bare multisig inscribers can easily route around mempool policy then there's no reason to oppose the change. If you ACK this proposal everyone can have their slice of pie.
>
> It is not contradictory if you understand how bitcoin works. This will create different mempool and P2P or incentivize a system that allows all transactions that follow consensus rules. It's a slippery slope.
>
> If you think bare multisig is bad for multisig for bitcoi
...
💬 zkfrio commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666923869)
>cry harder ordilabs spammer, you can't tell me what's spam to my node - your monkey jpeggery is certainly spam to me.
Try this ad hominem on twitter
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666923869)
>cry harder ordilabs spammer, you can't tell me what's spam to my node - your monkey jpeggery is certainly spam to me.
Try this ad hominem on twitter
💬 Davidson-Souza commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666927056)
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. 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.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1666927056)
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. 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.
📝 theStack opened a pull request: "test: check for specific bip157 disconnect reasons, add test coverage"
(https://github.com/bitcoin/bitcoin/pull/28227)
This PR checks for specific disconnect reasons using `assert_debug_log` in the functional test `p2p_blockfilters.py`. With that we ensure that the disconnect happens for the expected reason and also makes it easier to navigate between implementation and test code, i.e. both the questions "do we have test coverage for this disconnect cause?" (from an implementation reader's perspective) and "where is the code handling this disconnect cause?" (from a test reader's perspective) can be answered simp
...
(https://github.com/bitcoin/bitcoin/pull/28227)
This PR checks for specific disconnect reasons using `assert_debug_log` in the functional test `p2p_blockfilters.py`. With that we ensure that the disconnect happens for the expected reason and also makes it easier to navigate between implementation and test code, i.e. both the questions "do we have test coverage for this disconnect cause?" (from an implementation reader's perspective) and "where is the code handling this disconnect cause?" (from a test reader's perspective) can be answered simp
...
💬 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.