💬 stickies-v commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1587694422)
> The argument I'm making in this PR is that in all the places we are checking by txid, we shouldn't be doing that, because we're missing same-txid-different-witness cases.
I agree, the rationale makes sense. My concern is about segwit txs (`txid != wtxid`) where we check by txid (for whatever reason: intentional because we don't have witness data, legacy and we never updated it, buggy, ...): just creating a wtxid from the txid hash as is done here seems like it has potential to start missing
...
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1587694422)
> The argument I'm making in this PR is that in all the places we are checking by txid, we shouldn't be doing that, because we're missing same-txid-different-witness cases.
I agree, the rationale makes sense. My concern is about segwit txs (`txid != wtxid`) where we check by txid (for whatever reason: intentional because we don't have witness data, legacy and we never updated it, buggy, ...): just creating a wtxid from the txid hash as is done here seems like it has potential to start missing
...
💬 stickies-v commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1587705035)
To be clear: I'm not saying this PR needs to overhaul `AlreadyHaveTx` like I did, and I think having `TxOrphanage` index on `Wtxid` instead of `Txid` is better. I just think that if we're going to be calling `TxOrphanage` methods with (what are in fact) txids, we should probably have `TxOrphanage` _also_ keep a `Txid` index and not to blind conversion such as is happening here? I think that would address my concerns and make things easier to review?
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1587705035)
To be clear: I'm not saying this PR needs to overhaul `AlreadyHaveTx` like I did, and I think having `TxOrphanage` index on `Wtxid` instead of `Txid` is better. I just think that if we're going to be calling `TxOrphanage` methods with (what are in fact) txids, we should probably have `TxOrphanage` _also_ keep a `Txid` index and not to blind conversion such as is happening here? I think that would address my concerns and make things easier to review?
💬 instagibbs commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1587705703)
fwiw, I think the in-orphanage txid==wtixd case seems pretty rare all things considered. I had 4 over a 24 hour period, vs about 400 times where we fetched a segwit parent via txid though that particular txid was in the orphanage already.
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1587705703)
fwiw, I think the in-orphanage txid==wtixd case seems pretty rare all things considered. I had 4 over a 24 hour period, vs about 400 times where we fetched a segwit parent via txid though that particular txid was in the orphanage already.
📝 fanquake locked a pull request: "Implement BIP 118 validation (SIGHASH_ANYPREVOUT)"
(https://github.com/bitcoin/bitcoin/pull/30018)
Forward port of #34
(https://github.com/bitcoin/bitcoin/pull/30018)
Forward port of #34
💬 laanwj commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090624771)
> I see. I guess future UI confusion can ben prevented by calling this "PCP" with a note saying that it works with IPv6 only for now.
OK, i see your point now. Let's replace NAT-PMP for IPv4 at the same time, so we keep the same number of options, and lose one dependency.
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090624771)
> I see. I guess future UI confusion can ben prevented by calling this "PCP" with a note saying that it works with IPv6 only for now.
OK, i see your point now. Let's replace NAT-PMP for IPv4 at the same time, so we keep the same number of options, and lose one dependency.
💬 stickies-v commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1587730485)
> iow I'm not sure extra complexity is worth it,
Oh, fair enough. I don't have a good enough view of the attack vectors here. In my review, I was operating on the assumption that we wouldn't want to miss anything we're currently doing. If it's okay to relax that, that could make sense - but it should probably be made explicit then?
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1587730485)
> iow I'm not sure extra complexity is worth it,
Oh, fair enough. I don't have a good enough view of the attack vectors here. In my review, I was operating on the assumption that we wouldn't want to miss anything we're currently doing. If it's okay to relax that, that could make sense - but it should probably be made explicit then?
👋 fanquake's pull request is ready for review: "build: no-longer disable WARN_CXXFLAGS when CXXFLAGS is set"
(https://github.com/bitcoin/bitcoin/pull/25972)
(https://github.com/bitcoin/bitcoin/pull/25972)
💬 instagibbs commented on pull request "p2p: index TxOrphanage by wtxid, allow entries with same txid":
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1587742487)
IIUC If the check is removed, you'll waste bandwidth when you get orphan chains that are non-segwit, but you won't keep or relay them, so it shouldn't churn the orphanage.
Would it help if the comment was touched up to be explicit about what it's saving us from doing?
(https://github.com/bitcoin/bitcoin/pull/30000#discussion_r1587742487)
IIUC If the check is removed, you'll waste bandwidth when you get orphan chains that are non-segwit, but you won't keep or relay them, so it shouldn't churn the orphanage.
Would it help if the comment was touched up to be explicit about what it's saving us from doing?
📝 jasonandjay opened a pull request: "doc: add dustThreshold explain of P2SH"
(https://github.com/bitcoin/bitcoin/pull/30023)
The calculation of dust in Bitcoin has different values for different payment types.
For non-Witness: (148 + vout size)*rate
For Witness: (67+vout size)*rate
According to our calculation logic, our common P2PKH is 546, P2SH is 540, the minimum value of Witness is P2WPKH is 294, and P2TR is 330
However, the comments only mentioned P2PKH’s 546 and P2WPKH’s 294, which is inconsistent with the code logic.
These four payment types are very common in BTC wallets, so I think it is necessary
...
(https://github.com/bitcoin/bitcoin/pull/30023)
The calculation of dust in Bitcoin has different values for different payment types.
For non-Witness: (148 + vout size)*rate
For Witness: (67+vout size)*rate
According to our calculation logic, our common P2PKH is 546, P2SH is 540, the minimum value of Witness is P2WPKH is 294, and P2TR is 330
However, the comments only mentioned P2PKH’s 546 and P2WPKH’s 294, which is inconsistent with the code logic.
These four payment types are very common in BTC wallets, so I think it is necessary
...
💬 jlopp commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1587753988)
What's the reasoning for raising the minimum difficulty to 1 million? This will effectively prevent a developer from being able to move the chain forward without an ASIC.
I'd think that closing the timewarp loophole + the difficulty adjustment loophole should be sufficient to prevent block storms without actually making it more difficult for a dev to mint a low cost block if the chain has stalled.
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1587753988)
What's the reasoning for raising the minimum difficulty to 1 million? This will effectively prevent a developer from being able to move the chain forward without an ASIC.
I'd think that closing the timewarp loophole + the difficulty adjustment loophole should be sufficient to prevent block storms without actually making it more difficult for a dev to mint a low cost block if the chain has stalled.
💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090792385)
> I'm really suprised there isn't software that already exists, for doing the same thing, that we could just point users to?
If there's a library that does the same thing, and doesn't have 1 million lines of code, then it seems fine to include it. We can also make it a library under the core repo, which perhaps other projects will find useful for its simplicity. At least now I have a sense of what that library should be doing.
I don't think we should ask users to install a separate piece o
...
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090792385)
> I'm really suprised there isn't software that already exists, for doing the same thing, that we could just point users to?
If there's a library that does the same thing, and doesn't have 1 million lines of code, then it seems fine to include it. We can also make it a library under the core repo, which perhaps other projects will find useful for its simplicity. At least now I have a sense of what that library should be doing.
I don't think we should ask users to install a separate piece o
...
💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587827580)
No need to add networking indeed. As long as the eventual implementation (library or otherwise) has proper debug logging, we probably won't need a standalone tool for that.
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587827580)
No need to add networking indeed. As long as the eventual implementation (library or otherwise) has proper debug logging, we probably won't need a standalone tool for that.
🤔 glozow reviewed a pull request: "doc: add dustThreshold explain of P2SH & P2TR"
(https://github.com/bitcoin/bitcoin/pull/30023#pullrequestreview-2036066205)
Thanks for your interest in contributing, but this comment doesn't reflect what the code does (also see #22779 which is linked in a comment in the code). I think this could just be confusing?
(https://github.com/bitcoin/bitcoin/pull/30023#pullrequestreview-2036066205)
Thanks for your interest in contributing, but this comment doesn't reflect what the code does (also see #22779 which is linked in a comment in the code). I think this could just be confusing?
💬 saadbitcoin commented on pull request "doc: add dustThreshold explain of P2SH & P2TR":
(https://github.com/bitcoin/bitcoin/pull/30023#issuecomment-2090811444)
Thank you
في الخميس، ٢ مايو ٢٠٢٤ ٥:٤٠ م DrahtBot ***@***.***> كتب:
> The following sections might be updated with supplementary metadata
> relevant to reviewers and maintainers.
> Code Coverage
>
> For detailed information about the code coverage, see the test coverage
> report <https://corecheck.dev/bitcoin/bitcoin/pulls/30023>.
> Reviews
>
> See the guideline
> <https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review>
> for information on the review process
...
(https://github.com/bitcoin/bitcoin/pull/30023#issuecomment-2090811444)
Thank you
في الخميس، ٢ مايو ٢٠٢٤ ٥:٤٠ م DrahtBot ***@***.***> كتب:
> The following sections might be updated with supplementary metadata
> relevant to reviewers and maintainers.
> Code Coverage
>
> For detailed information about the code coverage, see the test coverage
> report <https://corecheck.dev/bitcoin/bitcoin/pulls/30023>.
> Reviews
>
> See the guideline
> <https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#code-review>
> for information on the review process
...
💬 Sjors commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1587857002)
> I'd think that closing the timewarp loophole + the difficulty adjustment loophole should be sufficient to prevent block storms without actually making it more difficult for a dev to mint a low cost block if the chain has stalled.
If that's true then that's fine. It seems multiple people like this ability, see @maflcko's comments above. I'm still a bit confused about the intersection of bugs / features here.
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1587857002)
> I'd think that closing the timewarp loophole + the difficulty adjustment loophole should be sufficient to prevent block storms without actually making it more difficult for a dev to mint a low cost block if the chain has stalled.
If that's true then that's fine. It seems multiple people like this ability, see @maflcko's comments above. I'm still a bit confused about the intersection of bugs / features here.
💬 jasonandjay commented on pull request "doc: add dustThreshold explain of P2SH & P2TR":
(https://github.com/bitcoin/bitcoin/pull/30023#issuecomment-2090841903)
> Thanks for your interest in contributing, but this comment doesn't reflect what the code does (also see #22779 which is linked in a comment in the code). I think this could just be confusing?
I checked #22779
Which part do you think is more confusing?
I think comments should be consistent with the code
(https://github.com/bitcoin/bitcoin/pull/30023#issuecomment-2090841903)
> Thanks for your interest in contributing, but this comment doesn't reflect what the code does (also see #22779 which is linked in a comment in the code). I think this could just be confusing?
I checked #22779
Which part do you think is more confusing?
I think comments should be consistent with the code
💬 fjahr commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1587866521)
It was suggested on the mailing list and it seemed to get some interest, so I added it here. But it has become clear in the discussion here that it seems to be pretty controversial so I will remove it here shortly.
Maybe there is a lower number that is not 1 that most people are happy with like @murchandamus [suggested](https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2047673316) and I will be happy to add it back in that case but while that is being bikeshedded, I think it's better
...
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1587866521)
It was suggested on the mailing list and it seemed to get some interest, so I added it here. But it has become clear in the discussion here that it seems to be pretty controversial so I will remove it here shortly.
Maybe there is a lower number that is not 1 that most people are happy with like @murchandamus [suggested](https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2047673316) and I will be happy to add it back in that case but while that is being bikeshedded, I think it's better
...
💬 glozow commented on pull request "Policy: Enforce witness script size limit for tapscript":
(https://github.com/bitcoin/bitcoin/pull/29769#issuecomment-2090895336)
The decision to close this PR was primarily based on the well-reasoned NACKs. Based on the technical arguments given by you and the reviewers, it is clear that the PR isn't a good idea or is at least very controversial. Changes to transaction relay policy that can impact people beyond the node operator should be discussed in the broader community beyond just Bitcoin Core. For example, I'd recommend starting a thread on the mailing list or Delving Bitcoin.
If you disagree and believe it was cl
...
(https://github.com/bitcoin/bitcoin/pull/29769#issuecomment-2090895336)
The decision to close this PR was primarily based on the well-reasoned NACKs. Based on the technical arguments given by you and the reviewers, it is clear that the PR isn't a good idea or is at least very controversial. Changes to transaction relay policy that can impact people beyond the node operator should be discussed in the broader community beyond just Bitcoin Core. For example, I'd recommend starting a thread on the mailing list or Delving Bitcoin.
If you disagree and believe it was cl
...
💬 glozow commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1587844135)
in d08e7a9ab6b353f415ca30f7714b12f39f84fe48, instead of `FindInPackageParents`, maybe just call `IsChildWithParents()`? Or even better, `Assume(IsChildWithParents())`?
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1587844135)
in d08e7a9ab6b353f415ca30f7714b12f39f84fe48, instead of `FindInPackageParents`, maybe just call `IsChildWithParents()`? Or even better, `Assume(IsChildWithParents())`?
💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1587905175)
Oh even easier than what I replaced it with, duh
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1587905175)
Oh even easier than what I replaced it with, duh