💬 furszy commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#issuecomment-1911119751)
> I just don't understand what motivated this PR, because "failure during the WalletBatch destruction" seems like something I would never expect to happen unless someone modified the code or manually corrupted the data on disk. So I'm wondering did we accidentally cause a breakage like that during development of these PRs? Or did you just notice that the error handling in the destructor wasn't very good, and decide to fix it? Or can failures to roll back actually happen in more cases than it see
...
(https://github.com/bitcoin/bitcoin/pull/29253#issuecomment-1911119751)
> I just don't understand what motivated this PR, because "failure during the WalletBatch destruction" seems like something I would never expect to happen unless someone modified the code or manually corrupted the data on disk. So I'm wondering did we accidentally cause a breakage like that during development of these PRs? Or did you just notice that the error handling in the destructor wasn't very good, and decide to fix it? Or can failures to roll back actually happen in more cases than it see
...
💬 theStack commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-1911138365)
Concept ACK
Another instance in the tests which could be adapted to 0xffffffff:
https://github.com/bitcoin/bitcoin/blob/717103bccec8d271f61f9cd6481b334bd9889146/src/test/transaction_tests.cpp#L783
Slightly related to this PR: since this currently doesn't produce a warning, was it ever considered to enable `-Wsign-conversion`?
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-1911138365)
Concept ACK
Another instance in the tests which could be adapted to 0xffffffff:
https://github.com/bitcoin/bitcoin/blob/717103bccec8d271f61f9cd6481b334bd9889146/src/test/transaction_tests.cpp#L783
Slightly related to this PR: since this currently doesn't produce a warning, was it ever considered to enable `-Wsign-conversion`?
💬 sipa commented on pull request "consensus: Store transaction nVersion as uint32_t":
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-1911141799)
Concept ACK on making transaction's `nVersion` unsigned, as that is in practice how it already behaves.
Would it make sense to also rename the field to e.g. `unsigned_version` or so, so that:
* Reviewing the diff makes it obvious whether any references to the field exist that aren't modified.
* Any future contributor who missed this change won't accidentally assume the field still has signed semantics?
For normal code I'd consider this measure overkill, but this is pretty fundamental con
...
(https://github.com/bitcoin/bitcoin/pull/29325#issuecomment-1911141799)
Concept ACK on making transaction's `nVersion` unsigned, as that is in practice how it already behaves.
Would it make sense to also rename the field to e.g. `unsigned_version` or so, so that:
* Reviewing the diff makes it obvious whether any references to the field exist that aren't modified.
* Any future contributor who missed this change won't accidentally assume the field still has signed semantics?
For normal code I'd consider this measure overkill, but this is pretty fundamental con
...
🤔 mzumsande reviewed a pull request: "fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses"
(https://github.com/bitcoin/bitcoin/pull/26859#pullrequestreview-1844867102)
ACK b851c5385d0a0acec4493be1561cea285065d5dc
I let the `banman` fuzzer run for a while, and it didn't crash anymore.
(https://github.com/bitcoin/bitcoin/pull/26859#pullrequestreview-1844867102)
ACK b851c5385d0a0acec4493be1561cea285065d5dc
I let the `banman` fuzzer run for a while, and it didn't crash anymore.
💬 petertodd commented on issue "Cluster mempool, CPFP carveout, and V3 transaction policy":
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1911201292)
@sdaftuar
> bounded in size to at most 1000 vbytes, if the child of an unconfirmed (and therefore v3) parent
This is insufficient to fix pinning in comparison to existing solutions: https://petertodd.org/2023/v3-txs-pinning-vulnerability
For example, at the moment the transaction fee required to get into the next block is about 23sat/vB, while the minimum relay fee of a typical mempool is 20sat/vB. So an attacker who simply did a straightforward pinning attack on an ephemeral anchor sp
...
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1911201292)
@sdaftuar
> bounded in size to at most 1000 vbytes, if the child of an unconfirmed (and therefore v3) parent
This is insufficient to fix pinning in comparison to existing solutions: https://petertodd.org/2023/v3-txs-pinning-vulnerability
For example, at the moment the transaction fee required to get into the next block is about 23sat/vB, while the minimum relay fee of a typical mempool is 20sat/vB. So an attacker who simply did a straightforward pinning attack on an ephemeral anchor sp
...
💬 petertodd commented on issue "Cluster mempool, CPFP carveout, and V3 transaction policy":
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1911204636)
@ariard
> however the dynamic N replace-by-feerate window might be a mess for miners mempools.
Can you give a bit more detail on what challenges you think that'll pose?
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1911204636)
@ariard
> however the dynamic N replace-by-feerate window might be a mess for miners mempools.
Can you give a bit more detail on what challenges you think that'll pose?
💬 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.