💬 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
💬 mzumsande commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1915425426)
Another reason to keep this check might be the following:
If we are in IBD and our chain is below minchainwork, we don't send headers to inbound peers, so they don't get to know our chain and won't attempt to download blocks from us (e.g. if they are even further behind in IBD).
I think that while in IBD it makes sense to use of the bandwidth for downloading the chain instead of serving others. Though admittedly this isn't a very realistic scenario because we don't self-advertise during IBD
...
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1915425426)
Another reason to keep this check might be the following:
If we are in IBD and our chain is below minchainwork, we don't send headers to inbound peers, so they don't get to know our chain and won't attempt to download blocks from us (e.g. if they are even further behind in IBD).
I think that while in IBD it makes sense to use of the bandwidth for downloading the chain instead of serving others. Though admittedly this isn't a very realistic scenario because we don't self-advertise during IBD
...
📝 maflcko opened a pull request: " refactor: Avoid UB in SHA3_256::Write "
(https://github.com/bitcoin/bitcoin/pull/31655)
It is UB to apply a distance to a pointer or iterator further than the
end itself, even if the distance is (partially) revoked later on.
Fix the issue by advancing the data pointer at most to the end.
This fix is required to adopt C++ safe buffers https://github.com/bitcoin/bitcoin/issues/31272.
Also included is a somewhat unrelated commit.
(https://github.com/bitcoin/bitcoin/pull/31655)
It is UB to apply a distance to a pointer or iterator further than the
end itself, even if the distance is (partially) revoked later on.
Fix the issue by advancing the data pointer at most to the end.
This fix is required to adopt C++ safe buffers https://github.com/bitcoin/bitcoin/issues/31272.
Also included is a somewhat unrelated commit.
💬 maflcko commented on pull request "refactor: Avoid UB in SHA3_256::Write":
(https://github.com/bitcoin/bitcoin/pull/31655#issuecomment-2590857397)
I tested with g++-14 and the resulting `bitcoind` was unchanged, so I tagged this as "refactor". However, this may not hold for every compiler and build setting.
(https://github.com/bitcoin/bitcoin/pull/31655#issuecomment-2590857397)
I tested with g++-14 and the resulting `bitcoind` was unchanged, so I tagged this as "refactor". However, this may not hold for every compiler and build setting.
💬 sipa commented on pull request "refactor: Avoid UB in SHA3_256::Write":
(https://github.com/bitcoin/bitcoin/pull/31655#issuecomment-2590878790)
utACK fabeca3458b38a3d8930cb0cbc866388c3f120f1
(https://github.com/bitcoin/bitcoin/pull/31655#issuecomment-2590878790)
utACK fabeca3458b38a3d8930cb0cbc866388c3f120f1
💬 hebasto commented on pull request "depends, NetBSD: Fix `bdb` package compilation with GCC-14":
(https://github.com/bitcoin/bitcoin/pull/31613#issuecomment-2590898164)
> It looks like `string.h` includes `strings.h` which defines these functions, but only if `_NETBSD_SOURCE` is defined....
>
> So agreed with @maflcko, let's just add a patch to include `strings.h` for netbsd.
The `strsep` function declaration is [guarded by `_NETBSD_SOURCE`](https://github.com/NetBSD/src/blob/c147cae3a562b7215a69630b1e3f3aaed09cd1dd/include/string.h#L97-L112) as well::
```c
#if defined(_NETBSD_SOURCE)
#include <strings.h> /* for backwards-compatibility */
__BEGIN_DEC
...
(https://github.com/bitcoin/bitcoin/pull/31613#issuecomment-2590898164)
> It looks like `string.h` includes `strings.h` which defines these functions, but only if `_NETBSD_SOURCE` is defined....
>
> So agreed with @maflcko, let's just add a patch to include `strings.h` for netbsd.
The `strsep` function declaration is [guarded by `_NETBSD_SOURCE`](https://github.com/NetBSD/src/blob/c147cae3a562b7215a69630b1e3f3aaed09cd1dd/include/string.h#L97-L112) as well::
```c
#if defined(_NETBSD_SOURCE)
#include <strings.h> /* for backwards-compatibility */
__BEGIN_DEC
...
💬 sipa commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1915464618)
> part of the block index, or exposed via RPC
I don't believe either of these to be the case.
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1915464618)
> part of the block index, or exposed via RPC
I don't believe either of these to be the case.
📝 yancyribbens opened a pull request: "test: Add expected result assertions"
(https://github.com/bitcoin/bitcoin/pull/31656)
This is a trivial addition to the test suit, however it shouldn't be required to add debug statements and manually run the tests if someone needs to know the results of this test.
(https://github.com/bitcoin/bitcoin/pull/31656)
This is a trivial addition to the test suit, however it shouldn't be required to add debug statements and manually run the tests if someone needs to know the results of this test.
👍 hodlinator approved a pull request: "refactor: Check translatable format strings at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31061#pullrequestreview-2550849478)
re-ACK fa51381790fe19e37e04a01a39cc94a00dcc441c
Just resolved conflict with #31616 since my last review.
(https://github.com/bitcoin/bitcoin/pull/31061#pullrequestreview-2550849478)
re-ACK fa51381790fe19e37e04a01a39cc94a00dcc441c
Just resolved conflict with #31616 since my last review.