💬 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.
💬 Empact commented on pull request "refactor: Remove unused MessageStartChars parameters from BlockManager methods":
(https://github.com/bitcoin/bitcoin/pull/28191#issuecomment-1667267999)
utACK fa69e3a95c452c2ba3221b17c19fba5993b5d073
(https://github.com/bitcoin/bitcoin/pull/28191#issuecomment-1667267999)
utACK fa69e3a95c452c2ba3221b17c19fba5993b5d073
💬 MarcoFalke commented on pull request "test: check for specific bip157 disconnect reasons, add test coverage":
(https://github.com/bitcoin/bitcoin/pull/28227#discussion_r1285434896)
```suggestion
peer_1.wait_for_disconnect()
```
Generally I am not a fan of using `assert_debug_log` to sync. Especially when it is trivial to avoid by adding 4 spaces
(https://github.com/bitcoin/bitcoin/pull/28227#discussion_r1285434896)
```suggestion
peer_1.wait_for_disconnect()
```
Generally I am not a fan of using `assert_debug_log` to sync. Especially when it is trivial to avoid by adding 4 spaces
💬 MarcoFalke commented on pull request "test: check for specific bip157 disconnect reasons, add test coverage":
(https://github.com/bitcoin/bitcoin/pull/28227#discussion_r1285435526)
same
(https://github.com/bitcoin/bitcoin/pull/28227#discussion_r1285435526)
same
💬 vasild commented on pull request "Use shared_ptr for CNode inside CConnman":
(https://github.com/bitcoin/bitcoin/pull/28222#issuecomment-1667275661)
> Also see #10738
:+1:
I guess it must be possible to do that without "reinventing the wheel aka introducing custom smart pointer types", like mentioned in the last comment.
(https://github.com/bitcoin/bitcoin/pull/28222#issuecomment-1667275661)
> Also see #10738
:+1:
I guess it must be possible to do that without "reinventing the wheel aka introducing custom smart pointer types", like mentioned in the last comment.
💬 Empact commented on pull request "refactor: Remove unused includes from wallet.cpp":
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1667285417)
To be clear as to what I'm referring to, note your commit messages both end with "Also ...". This alludes to the fact that you're doing a variety of things in each, which is a burden (in this case a minor one) on review.
Feel free to disregard as a nit, but I think this is a good practice to keep in mind, that we may maintain discipline in how we structure our changes for the benefit of review.
(https://github.com/bitcoin/bitcoin/pull/28200#issuecomment-1667285417)
To be clear as to what I'm referring to, note your commit messages both end with "Also ...". This alludes to the fact that you're doing a variety of things in each, which is a burden (in this case a minor one) on review.
Feel free to disregard as a nit, but I think this is a good practice to keep in mind, that we may maintain discipline in how we structure our changes for the benefit of review.
💬 MarcoFalke commented on pull request "util/blockstorage: Add `TRACE_RAII`, slightly faster -reindex-chainstate with CBufferedFile":
(https://github.com/bitcoin/bitcoin/pull/28226#issuecomment-1667287962)
It may be better to modify `CAutoFile` in that case. The file handle in the auto-file should[1] always be unique and pinned to a single thread, so I using `fread_unlocked` seems better, as it improves performance everywhere. (Though, it looks like `std::fread_unlocked` does not exist in C++, so I wonder if it exists on Windows and macOS?)
[1] I may submit a related pull to better enforce this at compile-time.
(https://github.com/bitcoin/bitcoin/pull/28226#issuecomment-1667287962)
It may be better to modify `CAutoFile` in that case. The file handle in the auto-file should[1] always be unique and pinned to a single thread, so I using `fread_unlocked` seems better, as it improves performance everywhere. (Though, it looks like `std::fread_unlocked` does not exist in C++, so I wonder if it exists on Windows and macOS?)
[1] I may submit a related pull to better enforce this at compile-time.
💬 M-BTC commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1667335410)
Concept ACK. Bloating the UTXO set should be more costly, especially when the incentives are perfectly clear.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1667335410)
Concept ACK. Bloating the UTXO set should be more costly, especially when the incentives are perfectly clear.
💬 Sun0fABeach commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1667370041)
> 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).
Since you're saying these changes won't cause any issues for you, we can agree to move forward with the PR.
> We will continue to use the permissionless Bitcoin protocol as we see fit. #FREEDOM
I'm sure you don't have any spam filters on your e-mail inbox since that would be cens
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1667370041)
> 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).
Since you're saying these changes won't cause any issues for you, we can agree to move forward with the PR.
> We will continue to use the permissionless Bitcoin protocol as we see fit. #FREEDOM
I'm sure you don't have any spam filters on your e-mail inbox since that would be cens
...
📝 TheCharlatan opened a pull request: "kernel: Run sanity checks on context construction"
(https://github.com/bitcoin/bitcoin/pull/28228)
Moving the kernel sanity check code to kernel context instantiation time ensures that it is impossible to create an invalid kernel context. This is based on a suggestion made in a [comment](https://github.com/bitcoin/bitcoin/pull/25065#discussion_r882182549) by @theuni in the [original pull request](https://github.com/bitcoin/bitcoin/pull/25065) first introducing the `kernel::Context`.
Since this touches the daemon initialization logic, also fix a bug where it would be possible for a terminat
...
(https://github.com/bitcoin/bitcoin/pull/28228)
Moving the kernel sanity check code to kernel context instantiation time ensures that it is impossible to create an invalid kernel context. This is based on a suggestion made in a [comment](https://github.com/bitcoin/bitcoin/pull/25065#discussion_r882182549) by @theuni in the [original pull request](https://github.com/bitcoin/bitcoin/pull/25065) first introducing the `kernel::Context`.
Since this touches the daemon initialization logic, also fix a bug where it would be possible for a terminat
...
💬 viresinnumeris-1 commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1667376865)
NACK
Clearly, some are going to be adversely affected by this PR if it gets merged.
Bitcoin Core does not belong to any single group. It should not be used to push personal agendas.
Let the node runners decide. Please don't assume they don't know what they're doing and push what _you_ think is best onto others.
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1667376865)
NACK
Clearly, some are going to be adversely affected by this PR if it gets merged.
Bitcoin Core does not belong to any single group. It should not be used to push personal agendas.
Let the node runners decide. Please don't assume they don't know what they're doing and push what _you_ think is best onto others.
💬 russeree commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1667380163)
> 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.
What about P2PK then? It could still be used for the exact same purpose to bloat the UTXO set.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1667380163)
> 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.
What about P2PK then? It could still be used for the exact same purpose to bloat the UTXO set.
💬 RobinLinus commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1667394446)
> used for the exact same purpose to bloat the UTXO set
It is more costly to bloat the UTXO set with P2PK, which is exactly the intended purpose of this spam mitigation.
Still, you're right that it makes sense to open a second PR to make p2pk non-standard as well when it gets abused.
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1667394446)
> used for the exact same purpose to bloat the UTXO set
It is more costly to bloat the UTXO set with P2PK, which is exactly the intended purpose of this spam mitigation.
Still, you're right that it makes sense to open a second PR to make p2pk non-standard as well when it gets abused.