💬 sdaftuar commented on issue "Cluster mempool, CPFP carveout, and V3 transaction policy":
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1911205244)
@ariard @petertodd The discussion you are having is not related to this topic, can you please take it to another thread?
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1911205244)
@ariard @petertodd The discussion you are having is not related to this topic, can you please take it to another thread?
💬 petertodd commented on issue "Cluster mempool, CPFP carveout, and V3 transaction policy":
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1911215494)
@sdaftuar As you said, "Opening an issue for high-level discussion".
Whether or not V3 achieves its goals is definitely a high level discussion that needs to be resolved here. I showed that V3 does not, as attackers can still cause defenders to pay significant amounts of money in response to pinning attacks, and that the way we intend to use V3 with ephemeral anchors is quite possibly *worse* than the status quo as anyone can be the attacker. I also provided a simple fix, and some less simple
...
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1911215494)
@sdaftuar As you said, "Opening an issue for high-level discussion".
Whether or not V3 achieves its goals is definitely a high level discussion that needs to be resolved here. I showed that V3 does not, as attackers can still cause defenders to pay significant amounts of money in response to pinning attacks, and that the way we intend to use V3 with ephemeral anchors is quite possibly *worse* than the status quo as anyone can be the attacker. I also provided a simple fix, and some less simple
...
💬 petertodd commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1911224605)
Relevant to this discussion: I've started a Bitcoin Libre Relay fork, that among other things does the following:
1. Peers with other Bitcoin Libre Relay nodes via a service bit (similar to my full-RBF peering fork).
2. Remove's Bitcoin Core's OP_Return limits.
The peering code is already sufficiently successful that multiple people have been able to get non-standard OP_Return outputs mined without interacting with miners directly. For example https://mempool.space/tx/c3dd9ae8b5e9811514ef
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1911224605)
Relevant to this discussion: I've started a Bitcoin Libre Relay fork, that among other things does the following:
1. Peers with other Bitcoin Libre Relay nodes via a service bit (similar to my full-RBF peering fork).
2. Remove's Bitcoin Core's OP_Return limits.
The peering code is already sufficiently successful that multiple people have been able to get non-standard OP_Return outputs mined without interacting with miners directly. For example https://mempool.space/tx/c3dd9ae8b5e9811514ef
...
💬 sdaftuar commented on issue "Cluster mempool, CPFP carveout, and V3 transaction policy":
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1911228975)
This topic is about the CPFP carveout rule, which cannot be supported in the cluster mempool design. My goal is to establish whether a particular set of policy rules would be a suitable replacement for carveout. You and ariard are discussing pinning more generally, and I infer from your prior comments that you think CPFP is a mistaken idea to begin with. So I assume that means you don't believe that dropping the CPFP carveout rule should be a big deal either, is that a fair statement?
That
...
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1911228975)
This topic is about the CPFP carveout rule, which cannot be supported in the cluster mempool design. My goal is to establish whether a particular set of policy rules would be a suitable replacement for carveout. You and ariard are discussing pinning more generally, and I infer from your prior comments that you think CPFP is a mistaken idea to begin with. So I assume that means you don't believe that dropping the CPFP carveout rule should be a big deal either, is that a fair statement?
That
...
💬 petertodd commented on pull request "RBF: Require unconfirmed inputs to come from a single conflicting transaction":
(https://github.com/bitcoin/bitcoin/pull/29297#issuecomment-1911233104)
@morehouse
> Actually, I think replacement cycling is still possible because the second parent doesn't need to be unconfirmed.
Heh, you nearly noticed the dumb mistake I made here. As Murch mentioned on the mailing list, this does not fix the infinite cycle problem with replace-by-fee-rate, as the second parent isn't unconfirmed.
However, if I change this pull req to only allowing fresh confirmed inputs, not including in an existing conflict, if transaction is being replaced that spend
...
(https://github.com/bitcoin/bitcoin/pull/29297#issuecomment-1911233104)
@morehouse
> Actually, I think replacement cycling is still possible because the second parent doesn't need to be unconfirmed.
Heh, you nearly noticed the dumb mistake I made here. As Murch mentioned on the mailing list, this does not fix the infinite cycle problem with replace-by-fee-rate, as the second parent isn't unconfirmed.
However, if I change this pull req to only allowing fresh confirmed inputs, not including in an existing conflict, if transaction is being replaced that spend
...
💬 RicYashiroLee commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1911234497)
> Relevant to this discussion: I've started a Bitcoin Libre Relay fork, that among other things does the following:
>
> 1. Peers with other Bitcoin Libre Relay nodes via a service bit (similar to my full-RBF peering fork).
> 2. Remove's Bitcoin Core's OP_Return limits.
>
> The peering code is already sufficiently successful that multiple people have been able to get non-standard OP_Return outputs mined without interacting with miners directly. For example https://mempool.space/tx/c3dd9ae8
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1911234497)
> Relevant to this discussion: I've started a Bitcoin Libre Relay fork, that among other things does the following:
>
> 1. Peers with other Bitcoin Libre Relay nodes via a service bit (similar to my full-RBF peering fork).
> 2. Remove's Bitcoin Core's OP_Return limits.
>
> The peering code is already sufficiently successful that multiple people have been able to get non-standard OP_Return outputs mined without interacting with miners directly. For example https://mempool.space/tx/c3dd9ae8
...
🤔 brunoerg reviewed a pull request: "fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses"
(https://github.com/bitcoin/bitcoin/pull/26859#pullrequestreview-1844943742)
utACK b851c5385d0a0acec4493be1561cea285065d5dc
(https://github.com/bitcoin/bitcoin/pull/26859#pullrequestreview-1844943742)
utACK b851c5385d0a0acec4493be1561cea285065d5dc
💬 TheBlueMatt commented on issue "Cluster mempool, CPFP carveout, and V3 transaction policy":
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1911313822)
> However, doing so would eliminate the ability to batch-CPFP several unconfirmed commitment transactions at once (although this isn't reliable anyway today, since the carveout protections don't apply to this case, but perhaps this sometimes works fine and is more efficient).
I believe this is something that needs to be addressed at some point in v3 transaction, but because, as you note, it is not reliable today, waiting to address it until later send acceptable. The only requirement here, then
...
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1911313822)
> However, doing so would eliminate the ability to batch-CPFP several unconfirmed commitment transactions at once (although this isn't reliable anyway today, since the carveout protections don't apply to this case, but perhaps this sometimes works fine and is more efficient).
I believe this is something that needs to be addressed at some point in v3 transaction, but because, as you note, it is not reliable today, waiting to address it until later send acceptable. The only requirement here, then
...
💬 theStack commented on pull request "test: p2p: check disconnect due to lack of desirable service flags":
(https://github.com/bitcoin/bitcoin/pull/29279#issuecomment-1911327678)
Solved the [flaky test issue](https://github.com/bitcoin/bitcoin/pull/29279#pullrequestreview-1844051383) by introducing a `wait_for_disconnect` parameter to `add_p2p_outbound_connection`, which hits the same code path as for feeler connections if set (i.e. waiting for immediate disconnect).
(https://github.com/bitcoin/bitcoin/pull/29279#issuecomment-1911327678)
Solved the [flaky test issue](https://github.com/bitcoin/bitcoin/pull/29279#pullrequestreview-1844051383) by introducing a `wait_for_disconnect` parameter to `add_p2p_outbound_connection`, which hits the same code path as for feeler connections if set (i.e. waiting for immediate disconnect).
👋 theStack's pull request is ready for review: "test: p2p: check disconnect due to lack of desirable service flags"
(https://github.com/bitcoin/bitcoin/pull/29279)
(https://github.com/bitcoin/bitcoin/pull/29279)
💬 Bitcoin-Lebowski commented on issue "Witness scripts being abused to bypass datacarriersize limit (CVE-2023-50428)":
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1911458289)
Everything @wizkid057 has said in the thread above in in line with my thoughts. I find it amazing and deeply worrying that those with ulterior motives could so easily drive Bitcoin discussion around this obvious exploit.
Bitcoin is a monetary network, other use cases which make it more difficult or expensive for users to use it as such are damaging to its adoption. It's common sense.
Some of the comments and behaviour I've seen from people defending the exploit are shocking to me. If we tr
...
(https://github.com/bitcoin/bitcoin/issues/29187#issuecomment-1911458289)
Everything @wizkid057 has said in the thread above in in line with my thoughts. I find it amazing and deeply worrying that those with ulterior motives could so easily drive Bitcoin discussion around this obvious exploit.
Bitcoin is a monetary network, other use cases which make it more difficult or expensive for users to use it as such are damaging to its adoption. It's common sense.
Some of the comments and behaviour I've seen from people defending the exploit are shocking to me. If we tr
...
💬 eragmus commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1911557649)
> Relevant to this discussion: I've started a Bitcoin Libre Relay fork, that among other things does the following:
@petertodd Thank you so much, Peter.
Some of us have made a series of (aligned with first principles reasoning, logic, bitcoin incentives and game theory) arguments, re: why this PR and other PRs of this nature: 1) do not make sense to achieve the goal (neither the original goal, nor the often-moved goalposts) + 2) can be counterproductive by having various negative effects. (i.e
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1911557649)
> Relevant to this discussion: I've started a Bitcoin Libre Relay fork, that among other things does the following:
@petertodd Thank you so much, Peter.
Some of us have made a series of (aligned with first principles reasoning, logic, bitcoin incentives and game theory) arguments, re: why this PR and other PRs of this nature: 1) do not make sense to achieve the goal (neither the original goal, nor the often-moved goalposts) + 2) can be counterproductive by having various negative effects. (i.e
...
👍 maflcko approved a pull request: "consensus: Store transaction nVersion as uint32_t"
(https://github.com/bitcoin/bitcoin/pull/29325#pullrequestreview-1845209715)
Agree that a rename of the field should be considered, but my preference would be just `version`, as opposed to `unsigned_version`. Otherwise, the code might be annoyingly verbose, even when it is clear to everyone that the transaction version is unsigned. (Maybe in a separate commit, so that if the changes are touching too many lines, or reviewers don't like it, it can be dropped again)
(https://github.com/bitcoin/bitcoin/pull/29325#pullrequestreview-1845209715)
Agree that a rename of the field should be considered, but my preference would be just `version`, as opposed to `unsigned_version`. Otherwise, the code might be annoyingly verbose, even when it is clear to everyone that the transaction version is unsigned. (Maybe in a separate commit, so that if the changes are touching too many lines, or reviewers don't like it, it can be dropped again)
💬 maflcko commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1467314983)
```suggestion
entry.pushKV("version", tx.nVersion);
```
No need for a cast here. Also, the comment above is wrong.
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1467314983)
```suggestion
entry.pushKV("version", tx.nVersion);
```
No need for a cast here. Also, the comment above is wrong.
💬 maflcko commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1467316635)
```suggestion
tx.nVersion = random.choice([1, 2, random.randbits(32)])
```
nit: No need to call the randbits wrapper
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1467316635)
```suggestion
tx.nVersion = random.choice([1, 2, random.randbits(32)])
```
nit: No need to call the randbits wrapper
💬 maflcko commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-1911633537)
> Slightly related to this PR: since this currently doesn't produce a warning, was it ever considered to enable `-Wsign-conversion`?
I think it is a common convention to write `-1` as an alias for `std::numeric_limits<unsigned ...>::max()`. Also, I expect there will be many places where positive signed integers are sign-preserving converted to unsigned (and vice-versa). So enabling that compiler warning may not be possible without changing a lot more code.
Luckily, if a sign isn't preserve
...
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-1911633537)
> Slightly related to this PR: since this currently doesn't produce a warning, was it ever considered to enable `-Wsign-conversion`?
I think it is a common convention to write `-1` as an alias for `std::numeric_limits<unsigned ...>::max()`. Also, I expect there will be many places where positive signed integers are sign-preserving converted to unsigned (and vice-versa). So enabling that compiler warning may not be possible without changing a lot more code.
Luckily, if a sign isn't preserve
...
💬 vostrnad commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-1911639280)
Concept ACK
As someone quite familiar with Bitcoin's consensus rules but largely unfamiliar with Bitcoin Core's internals, I had no idea until recently that the version was stored as a signed integer. This should bring it in line with expectations of most future contributors.
Also negative versions are just weird.
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-1911639280)
Concept ACK
As someone quite familiar with Bitcoin's consensus rules but largely unfamiliar with Bitcoin Core's internals, I had no idea until recently that the version was stored as a signed integer. This should bring it in line with expectations of most future contributors.
Also negative versions are just weird.
💬 vostrnad commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1467332516)
Comment above needs updating.
(https://github.com/bitcoin/bitcoin/pull/29325#discussion_r1467332516)
Comment above needs updating.
💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1467380073)
> Why not switch over entirely to Podman? It's quite confusing that we're using both.
I don't see the benefit of requiring that docker is uninstalled. If someone wants to use their container engine of their choice locally, just let them do it?
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1467380073)
> Why not switch over entirely to Podman? It's quite confusing that we're using both.
I don't see the benefit of requiring that docker is uninstalled. If someone wants to use their container engine of their choice locally, just let them do it?
💬 glozow commented on issue "Cluster mempool, CPFP carveout, and V3 transaction policy":
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1911702787)
> > However, doing so would eliminate the ability to batch-CPFP several unconfirmed commitment transactions at once (although this isn't reliable anyway today, since the carveout protections don't apply to this case, but perhaps this sometimes works fine and is more efficient).
>
> I believe this is something that needs to be addressed at some point in v3 transaction, but because, as you note, it is not reliable today, waiting to address it until later send acceptable. The only requirement he
...
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1911702787)
> > However, doing so would eliminate the ability to batch-CPFP several unconfirmed commitment transactions at once (although this isn't reliable anyway today, since the carveout protections don't apply to this case, but perhaps this sometimes works fine and is more efficient).
>
> I believe this is something that needs to be addressed at some point in v3 transaction, but because, as you note, it is not reliable today, waiting to address it until later send acceptable. The only requirement he
...