💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1915015927)
4c23aacc2b82083f96255963f543dc35fff28334: since you need to print log statements here in a few places anyway, might as well drop the `errmsg` argument and always print logs here.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1915015927)
4c23aacc2b82083f96255963f543dc35fff28334: since you need to print log statements here in a few places anyway, might as well drop the `errmsg` argument and always print logs here.
💬 stickies-v commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915078874)
> but that is what the current implementation in [e0a5417](https://github.com/bitcoin/bitcoin/commit/e0a541702def3a4c6cce8e44b6ebe8d608edad4e)) would do.
Are you sure it would? I don't see how the `Output` return type could be deduced with `auto`. That's kind of the point of the separate `Input` and `Output` types: it force the user to specify the return type, because especially with left shift the chances of the return type being much bigger than the input are high. Using (and potentially wr
...
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915078874)
> but that is what the current implementation in [e0a5417](https://github.com/bitcoin/bitcoin/commit/e0a541702def3a4c6cce8e44b6ebe8d608edad4e)) would do.
Are you sure it would? I don't see how the `Output` return type could be deduced with `auto`. That's kind of the point of the separate `Input` and `Output` types: it force the user to specify the return type, because especially with left shift the chances of the return type being much bigger than the input are high. Using (and potentially wr
...
💬 hebasto commented on pull request "doc: Improve dependencies documentation":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1915084448)
CMake 3.24.2 is used for release binaries in the Guix environment.
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1915084448)
CMake 3.24.2 is used for release binaries in the Guix environment.
🤔 jonatack reviewed a pull request: "test: avoid internet traffic"
(https://github.com/bitcoin/bitcoin/pull/31646#pullrequestreview-2550171291)
ACK 2ed161c5ce648cb66ec3d2941b02d68b6ca4c390
(did not observe an increase in speed on arm64 m1 max macos 15.2)
(https://github.com/bitcoin/bitcoin/pull/31646#pullrequestreview-2550171291)
ACK 2ed161c5ce648cb66ec3d2941b02d68b6ca4c390
(did not observe an increase in speed on arm64 m1 max macos 15.2)
🤔 mzumsande reviewed a pull request: "consensus: Remove checkpoints (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31649#pullrequestreview-2550175831)
In #25725 the approach was to remove just the actual checkpoints, but leave the supporting code to be removed in the next / a later release (https://github.com/bitcoin/bitcoin/pull/25725#discussion_r1103582363). Did you consider this, instead of doing it all in one commit?
(https://github.com/bitcoin/bitcoin/pull/31649#pullrequestreview-2550175831)
In #25725 the approach was to remove just the actual checkpoints, but leave the supporting code to be removed in the next / a later release (https://github.com/bitcoin/bitcoin/pull/25725#discussion_r1103582363). Did you consider this, instead of doing it all in one commit?
👍 dergoegge approved a pull request: "consensus: Remove checkpoints (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31649#pullrequestreview-2550177263)
ACK 9e91e536a45e3fe802eb8b38ccc893ef894e72e7
The "new" headers pre-sync (shipped in v24.0) protects us against the same denial-of-service issues that the checkpoints were solving in the past. Therefore it is reasonable to remove them, especially considering the testing the pre-sync has undergone and the amount of time that has passed.
Even under the assumption that there is a bug in the headers pre-sync logic (i.e. it's somehow still possible to submit low work headers), the checkpoints on
...
(https://github.com/bitcoin/bitcoin/pull/31649#pullrequestreview-2550177263)
ACK 9e91e536a45e3fe802eb8b38ccc893ef894e72e7
The "new" headers pre-sync (shipped in v24.0) protects us against the same denial-of-service issues that the checkpoints were solving in the past. Therefore it is reasonable to remove them, especially considering the testing the pre-sync has undergone and the amount of time that has passed.
Even under the assumption that there is a bug in the headers pre-sync logic (i.e. it's somehow still possible to submit low work headers), the checkpoints on
...
💬 Sjors commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2590320921)
If we're going to set a minimum value for `-blockreservedweight` we should probably just pick the absolute minimum:
- a block with only a coinbase transaction
- header weight + 1 (for the number of transactions)
- the coinbase has 1 input (required by `IsCoinBase()`)
- just the BIP34 scriptSig
- no witness commitment
- the coinbase has 1 output (required by `CheckTransaction`)
- `OP_TRUE` output script (or empty?)
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2590320921)
If we're going to set a minimum value for `-blockreservedweight` we should probably just pick the absolute minimum:
- a block with only a coinbase transaction
- header weight + 1 (for the number of transactions)
- the coinbase has 1 input (required by `IsCoinBase()`)
- just the BIP34 scriptSig
- no witness commitment
- the coinbase has 1 output (required by `CheckTransaction`)
- `OP_TRUE` output script (or empty?)
💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915119234)
> Are you sure it would? I don't see how the `Output` return type could be deduced with `auto`.
You're right. I didn't realize e0a541702def3a4c6cce8e44b6ebe8d608edad4e required specifying what type to cast the output to. In that case, I take back the comment that it has a footgun.
But I would say the implementation does not seem consistent with other functions in this file, or consistent with what I would expect a function that is doing a saturating multiplication operation to do. I think
...
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915119234)
> Are you sure it would? I don't see how the `Output` return type could be deduced with `auto`.
You're right. I didn't realize e0a541702def3a4c6cce8e44b6ebe8d608edad4e required specifying what type to cast the output to. In that case, I take back the comment that it has a footgun.
But I would say the implementation does not seem consistent with other functions in this file, or consistent with what I would expect a function that is doing a saturating multiplication operation to do. I think
...
💬 darosior commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1915120237)
Made sense to me without being familiar with how the anti-DoS sync works in details, asked on IRC, Suhas points out it's still possible for a node to have checkpoint-violating headers to serve:
> 11:00 <sdaftuar> darosior: that's not quite true, because an adversary can feed you the honest chain during pre-sync, and then a bogus one during redownload
> 11:00 <sdaftuar> darosior: so you'd detect it quickly and disconnect but be left with headers that are possibly checkpoint-violating (if your n
...
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1915120237)
Made sense to me without being familiar with how the anti-DoS sync works in details, asked on IRC, Suhas points out it's still possible for a node to have checkpoint-violating headers to serve:
> 11:00 <sdaftuar> darosior: that's not quite true, because an adversary can feed you the honest chain during pre-sync, and then a bogus one during redownload
> 11:00 <sdaftuar> darosior: so you'd detect it quickly and disconnect but be left with headers that are possibly checkpoint-violating (if your n
...
💬 Sjors commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2590399846)
> I don't see what additional assurances waiting even longer would give us.
Merged pull requests tend to get additional attention, e.g. in the Optech Newsletter. Perhaps this will jolt someones memory, and there wouldn't be a rush to revert close to a release.
Perhaps someone is sitting on an exploit and wants to collect a bug bounty for it. For it to be a bug, it needs to be merged. That said, there's no such bounty and we probably don't want to incentivise that type of behaviour.
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2590399846)
> I don't see what additional assurances waiting even longer would give us.
Merged pull requests tend to get additional attention, e.g. in the Optech Newsletter. Perhaps this will jolt someones memory, and there wouldn't be a rush to revert close to a release.
Perhaps someone is sitting on an exploit and wants to collect a bug bounty for it. For it to be a bug, it needs to be merged. That said, there's no such bounty and we probably don't want to incentivise that type of behaviour.
💬 Sjors commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1915163525)
It seems that we respons to `GETHEADERS` as soon as we're listening. It seems like a good idea in general to not answer until we have `MinimumChainWork` worth of headers. (using an empty headers message to ensure they don't hang up on us)
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1915163525)
It seems that we respons to `GETHEADERS` as soon as we're listening. It seems like a good idea in general to not answer until we have `MinimumChainWork` worth of headers. (using an empty headers message to ensure they don't hang up on us)
💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2590452047)
Updated 0e2887262e1a3a4a5ab0382f9e8713b135408a77 -> 922aa7a25650bdfd8428198f171f88915af1ffa0 ([kernel_cache_sizes_14](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_14) -> [kernel_cache_sizes_15](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_15), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_14..kernel_cache_sizes_15))
* Addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1912972952), ren
...
(https://github.com/bitcoin/bitcoin/pull/31483#issuecomment-2590452047)
Updated 0e2887262e1a3a4a5ab0382f9e8713b135408a77 -> 922aa7a25650bdfd8428198f171f88915af1ffa0 ([kernel_cache_sizes_14](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_14) -> [kernel_cache_sizes_15](https://github.com/TheCharlatan/bitcoin/tree/kernel_cache_sizes_15), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernel_cache_sizes_14..kernel_cache_sizes_15))
* Addressed @stickies-v's [comment](https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1912972952), ren
...
💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915196192)
> But I would say the implementation does not seem consistent with other functions in this file, or consistent with what I would expect a function that is doing a saturating multiplication operation to do.
I tend to agree with this, and rolled with your suggestion for now, but still open for other suggestions.
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915196192)
> But I would say the implementation does not seem consistent with other functions in this file, or consistent with what I would expect a function that is doing a saturating multiplication operation to do.
I tend to agree with this, and rolled with your suggestion for now, but still open for other suggestions.
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1915209026)
`BindAndStartListening()` needs to return an error string to the caller, because the caller not only logs it but also passes it to `ThreadSafeMessageBox()`.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1915209026)
`BindAndStartListening()` needs to return an error string to the caller, because the caller not only logs it but also passes it to `ThreadSafeMessageBox()`.
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1915209813)
Done, `s/at/on/`
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1915209813)
Done, `s/at/on/`
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2590500946)
`f71f1a346c...d83ff66fd7`:
> ... dropping `AddSocketPermissionFlags`. Can you move that to a separate (earlier?) commit?
Done.
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2590500946)
`f71f1a346c...d83ff66fd7`:
> ... dropping `AddSocketPermissionFlags`. Can you move that to a separate (earlier?) commit?
Done.
💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1915228277)
Ah ok, I missed that bit.
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1915228277)
Ah ok, I missed that bit.
💬 marcofleon commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2590653524)
> Did you consider this, instead of doing it all in one commit?
I did consider it. I figured if we're confident enough to remove checkpoints, then might as well do it all at once. I feel like reverting this commit, if necessary, wouldn't be too difficult. Of course, if we end up preferring the split commit approach then I'm happy to do that too.
(https://github.com/bitcoin/bitcoin/pull/31649#issuecomment-2590653524)
> Did you consider this, instead of doing it all in one commit?
I did consider it. I figured if we're confident enough to remove checkpoints, then might as well do it all at once. I feel like reverting this commit, if necessary, wouldn't be too difficult. Of course, if we end up preferring the split commit approach then I'm happy to do that too.
💬 sipa commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1915319815)
After some more discussion on IRC and IRL.
For this to have an effect, it means an attacker needs to get us to accept blocks on an attack chain that has more work than minchainwork (at the time it is accepted), which should be very expensive. So I don't think it's a necessity to keep the check for the purpose of preventing being disconnected by checkpoint-enabled nodes.
On the other hand, there seems to be little reason not to do the check. It's cheap, and it doesn't seem unreasonable to a
...
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1915319815)
After some more discussion on IRC and IRL.
For this to have an effect, it means an attacker needs to get us to accept blocks on an attack chain that has more work than minchainwork (at the time it is accepted), which should be very expensive. So I don't think it's a necessity to keep the check for the purpose of preventing being disconnected by checkpoint-enabled nodes.
On the other hand, there seems to be little reason not to do the check. It's cheap, and it doesn't seem unreasonable to a
...
💬 vasild commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2590687348)
`d83ff66fd7...bcf1254e91`: adjust test after change of the log message in the previous push
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2590687348)
`d83ff66fd7...bcf1254e91`: adjust test after change of the log message in the previous push