💬 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
...
💬 willcl-ark commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1467385670)
Not specifically related to the issue at hand: the new podman desktop app is pretty smart, it can show you containers running via docker and podman (and kubernetes clusters) all in the same UI: https://podman-desktop.io/
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1467385670)
Not specifically related to the issue at hand: the new podman desktop app is pretty smart, it can show you containers running via docker and podman (and kubernetes clusters) all in the same UI: https://podman-desktop.io/
💬 glozow commented on pull request "doc: clarify `BroadcastTransaction` comment":
(https://github.com/bitcoin/bitcoin/pull/29308#discussion_r1467389150)
I agree with @stickies-v that listing all the callers of a function within code documentation is not maintainable. This should not be something that a developer relies on and is what a grepper / search bar is for. If it's confusing that not all RPCs are listed, I'd be in favor of just deleting the names of the RPCs that use this function, as it doesn't matter to the comment.
(https://github.com/bitcoin/bitcoin/pull/29308#discussion_r1467389150)
I agree with @stickies-v that listing all the callers of a function within code documentation is not maintainable. This should not be something that a developer relies on and is what a grepper / search bar is for. If it's confusing that not all RPCs are listed, I'd be in favor of just deleting the names of the RPCs that use this function, as it doesn't matter to the comment.
🤔 maflcko reviewed a pull request: "test: Check object hashes in wait_for_getheaders"
(https://github.com/bitcoin/bitcoin/pull/29318#pullrequestreview-1845337916)
In Bitcoin Core, the stop hash will always be zero, so not sure if it is worth it to require passing it every time?
It may be better to instead pass the top hash and check that, if provided?
(https://github.com/bitcoin/bitcoin/pull/29318#pullrequestreview-1845337916)
In Bitcoin Core, the stop hash will always be zero, so not sure if it is worth it to require passing it every time?
It may be better to instead pass the top hash and check that, if provided?
💬 maflcko commented on pull request "test: Check object hashes in wait_for_getheaders":
(https://github.com/bitcoin/bitcoin/pull/29318#discussion_r1467397387)
I think the block hash can be returned from `announce_random_block` and then checked here against the first item in the block locator hashes?
(https://github.com/bitcoin/bitcoin/pull/29318#discussion_r1467397387)
I think the block hash can be returned from `announce_random_block` and then checked here against the first item in the block locator hashes?
💬 maflcko commented on pull request "test: Check object hashes in wait_for_getheaders":
(https://github.com/bitcoin/bitcoin/pull/29318#discussion_r1467397574)
same?
(https://github.com/bitcoin/bitcoin/pull/29318#discussion_r1467397574)
same?