💬 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.
💬 jlopp commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2091032430)
In keeping with genesis block tradition I'd propose the following coinbase string referencing an article published today regarding testnet: "CCN 02/May/2024 Bitcoin Testnet Could Need Reset"
```
./generate-genesis -timestamp 1714658825 -pubkey 03752c999db32c08a6d62550a1ad83cf1731f35ac27096c27dfca5127c0c6dbd9b -psz "CCN 02/May/2024 Bitcoin Testnet Could Need Reset" -nonce 1
Ctrl Hash: 0x000000004693a387d558bbf7e321f6915e82d1afac4db25ee40f44bb783e802b
Target: 0x00000000ffff00000000000000000
...
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2091032430)
In keeping with genesis block tradition I'd propose the following coinbase string referencing an article published today regarding testnet: "CCN 02/May/2024 Bitcoin Testnet Could Need Reset"
```
./generate-genesis -timestamp 1714658825 -pubkey 03752c999db32c08a6d62550a1ad83cf1731f35ac27096c27dfca5127c0c6dbd9b -psz "CCN 02/May/2024 Bitcoin Testnet Could Need Reset" -nonce 1
Ctrl Hash: 0x000000004693a387d558bbf7e321f6915e82d1afac4db25ee40f44bb783e802b
Target: 0x00000000ffff00000000000000000
...
💬 jlopp commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2091039789)
> Only allowing lower difficulty blocks after 6 hours could easily make testnet useless for extended periods, if someone put several ASICs on testnet for a while, it might prevent other users from getting confirmations for up to 6 hours. I could see an increase from the twenty minute rule to maybe an hour, but more seems counter to why the rule was introduced in the first place.
6 hours seems overkill to me. I'd think 3 hours would suffice, since it would mean that at worst someone would be a
...
(https://github.com/bitcoin/bitcoin/pull/29775#issuecomment-2091039789)
> Only allowing lower difficulty blocks after 6 hours could easily make testnet useless for extended periods, if someone put several ASICs on testnet for a while, it might prevent other users from getting confirmations for up to 6 hours. I could see an increase from the twenty minute rule to maybe an hour, but more seems counter to why the rule was introduced in the first place.
6 hours seems overkill to me. I'd think 3 hours would suffice, since it would mean that at worst someone would be a
...
💬 glozow commented on pull request "test: add MiniWallet tagging support to avoid UTXO mixing, use in `fill_mempool`":
(https://github.com/bitcoin/bitcoin/pull/29939#discussion_r1587963767)
Could also continue to get all the UTXOs up front?
(https://github.com/bitcoin/bitcoin/pull/29939#discussion_r1587963767)
Could also continue to get all the UTXOs up front?
💬 Sjors commented on pull request "[PoC, nomerge] IPv6 PCP pinhole test":
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587973156)
I was looking at 7.3:
> Option Code: 8 bits. Its most significant bit indicates if this
> option is mandatory (0) or optional (1) to process.
But that's implied by using these integer ranges.
Tend to agree we should drop this option, though we should pay attention to its return value. Otherwise we'd potentially gossip the wrong port (in practice this likely only happens if you have two nodes in the same home).
(https://github.com/bitcoin/bitcoin/pull/30005#discussion_r1587973156)
I was looking at 7.3:
> Option Code: 8 bits. Its most significant bit indicates if this
> option is mandatory (0) or optional (1) to process.
But that's implied by using these integer ranges.
Tend to agree we should drop this option, though we should pay attention to its return value. Otherwise we'd potentially gossip the wrong port (in practice this likely only happens if you have two nodes in the same home).
📝 jonatack opened a pull request: "doc: replace remaining "520" magic nums with MAX_SCRIPT_ELEMENT_SIZE"
(https://github.com/bitcoin/bitcoin/pull/30024)
Noticed these while reviewing BIPs yesterday.
It would be clearer and more future-proof to refer to their constant name.
(https://github.com/bitcoin/bitcoin/pull/30024)
Noticed these while reviewing BIPs yesterday.
It would be clearer and more future-proof to refer to their constant name.
💬 instagibbs commented on pull request "doc: replace remaining "520" magic nums with MAX_SCRIPT_ELEMENT_SIZE":
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2091169327)
maybe just replace 520 with MAX_SCRIPT_ELEMENT_SIZE and nothing else? concept ACK otherwise. I can confirm they're all referring to the same limit at least.
(https://github.com/bitcoin/bitcoin/pull/30024#issuecomment-2091169327)
maybe just replace 520 with MAX_SCRIPT_ELEMENT_SIZE and nothing else? concept ACK otherwise. I can confirm they're all referring to the same limit at least.