💬 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
💬 laanwj commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090946812)
i mean, there's probably some crappy C implementation of PCP out there that we could include, but this one is written in C++, it integrates with bitcoin's networking and logging, well commented, and it's straightforward enough. Besides, any code included from a third party would have to be reviewed just as well.
i've already agreed to add IPv4 support so it can replace libnatpmp.
@fanquake's comment was based on a misunderstanding.
(https://github.com/bitcoin/bitcoin/pull/30005#issuecomment-2090946812)
i mean, there's probably some crappy C implementation of PCP out there that we could include, but this one is written in C++, it integrates with bitcoin's networking and logging, well commented, and it's straightforward enough. Besides, any code included from a third party would have to be reviewed just as well.
i've already agreed to add IPv4 support so it can replace libnatpmp.
@fanquake's comment was based on a misunderstanding.
💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1587919121)
pushed with the `Assume()` check inline with the "making sure package is cluster size two" check
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1587919121)
pushed with the `Assume()` check inline with the "making sure package is cluster size two" check
🤔 hebasto reviewed a pull request: "Fix misleading signmessage error with segwit"
(https://github.com/bitcoin-core/gui/pull/819#pullrequestreview-2036243242)
Approach ACK 0fea73fc7444884dc6831b1fe9e608898a398a92.
Linter failure?
(https://github.com/bitcoin-core/gui/pull/819#pullrequestreview-2036243242)
Approach ACK 0fea73fc7444884dc6831b1fe9e608898a398a92.
Linter failure?
💬 laanwj commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587940735)
i don't think that's the case. The mandatory flag is not a user choice, it's part of the RFC definition of the option:
- Figure 14 says "Option Code=2" only.
- Section 19.4 says "The values 0-127 are mandatory to process, and 128-255 are optional to process." This implies these are disjunct ranges.
In any case, we could be more lenient about this if we're willing to accept different addresses (or maybe even ports) and advertise them.
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587940735)
i don't think that's the case. The mandatory flag is not a user choice, it's part of the RFC definition of the option:
- Figure 14 says "Option Code=2" only.
- Section 19.4 says "The values 0-127 are mandatory to process, and 128-255 are optional to process." This implies these are disjunct ranges.
In any case, we could be more lenient about this if we're willing to accept different addresses (or maybe even ports) and advertise them.
💬 laanwj commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587955428)
Right. Let's not do this. Such a stand-alone tool, if it is useful, doesn't need to be part of bitcoin.
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587955428)
Right. Let's not do this. Such a stand-alone tool, if it is useful, doesn't need to be part of bitcoin.