consensus: Remove mainnet checkpoints #25725
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Concept ACK (once the logic at the p2p layer is added). It could be argued by theoretical sticklers that this change is technically a hard fork. But in practice removing checkpoints poses zero risk of a network split and checkpoints have caused much confusion in the past. |
sdaftuar
force-pushed
the
2022-07-remove-checkpoints
branch
from
eeb83be
to
d3928db
Compare
Replace them with a single checkpoint of the genesis block (which is already effectively checkpointed). Now that we have logic at the p2p layer to avoid permanently storing headers unless they lead to a sufficiently high work chain, we no longer need to use checkpoints to protect against spam from low-difficulty headers.
sdaftuar
force-pushed
the
2022-07-remove-checkpoints
branch
from
d3928db
to
1b54186
Compare
|
Concept ACK. I always tested the previous PR with this patch on top of it. No longer needs to be draft. I suggest holding off merge until after the v24 branch-off, perhaps a bit longer. We should give people a chance to come up with attacks that are still possible after #25717, yet are somehow thwarted by checkpoints. Also, if we find a structural problem with that PR and have to revert it, I'd rather not give CNN the headline "Bitcoin Core brings back checkpoints". @michaelfolkson I think dropping checkpoints is in the same category as retroactively applying soft forks from genesis. With checkpoints an attacker can "only" wipe out eight years of transaction history (the last checkpoint is from 2014). Without checkpoints they can wipe out thirteen years. Those who run an older client will be "saved" in the case of the latter attack. Interestingly, if the attacker violates P2SH rules before block 173,805 (see #11739) they would previously be rejected by all nodes, just based on headers, because that's before the most recent checkpoint. With this PR the blocks would be rejected based on violating P2SH rules. That's still not the deepest burried soft-fork, that honor either goes to the 1MB limit (which Satoshi buried in 172f006 almost immediately after introducing it) or something older. Note that the changes in #25717 do not prevent such a massive reorg: as long as the headers have valid proof-of-work, they'll get processed. |
I agree with this -- it would be good for reviewers to consider whether there are any avenues to accepting headers that may have been overlooked in #25717, and generally how to ensure that our code is robust to bugs being introduced in the future that could open up an attack that would otherwise be mitigated by checkpoints. |
|
Meanwhile developers and other volunteers could run their node with |
There was a problem hiding this comment.
Concept ACK
After #25717, there shouldn't be a concern of denial-of-service where a new node disk is being feeded with low-pow headers, and as the checkpoints have raised concerns in the determination of the consensus algorithm outcome, this is a valuable philosophical improvement. Of course, one could argue removing checkpoints is materially a hard-fork as new clients could plausibly accept a chain diverging from the synced-with-checkpoints clients, however I think this would assume an adversary with more hashrate capabilities than the set of miners building on top of the current chain. As raised by other reviewers, I think there is still the open question if they're other (actual or future) attacks or risks vectors thwarted by checkpoints.
| {250000, uint256S("0x000000000000003887df1f29024b06fc2200b55f8af8f35453d7be294df2d214")}, | ||
| {279000, uint256S("0x0000000000000001ae8c72a0b0c301f67e3afca10e819efa9041e458e9bd7e40")}, | ||
| {295000, uint256S("0x00000000000000004d9b4ef50f0f9d686fd69db2e03af35a100370c64632a983")}, | ||
| {0, uint256S("0x000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f")}, | ||
| } |
There was a problem hiding this comment.
I think the whole checkpoint logic (checkpointData, GetLastCheckpoint, fCheckpointsEnabled, DEFAULT_CHECKPOINTS_ENABLED) could be removed, as we've already the genesis block hash available in CChainParams, unless we would like to keep the mechanism for testnet/regtest/signet but given the lack of checkpoints on testing networks, I would say they're actually useful for no one on those networks.
There was a problem hiding this comment.
Keeping the logic but not using it seems like it would make it easier to quickly reintroduce if we do later find an attack that this mitigates?
There was a problem hiding this comment.
Yeah, no strong opinion here. Doesn't seem the most complex commit to reverse if needed. At the same time good to cleanup the code (or add a historical note to explain what where checkpoints and why unused code is still there i don't know).
There was a problem hiding this comment.
I think it's fine to drop the checkpoints in one release and then drop the checkpoint logic in a later release.
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
Concept ACK For anyone interested in moving this forward, I opened #28043 to give us some more confidence in making this change. |
|
Are you still working on this? Given that #25717 has been long since merged, should this still be a draft? |
|
I think the question here is whether we're confident enough in the checks we have in place to prevent low-work-header DoS that we're ok with dropping the existing checkpoints. Philosophically, the change from #25717 should make us theoretically protected from these kinds of concerns. In practice, however, bugs are easy to inadvertently introduce, and if we're not confident that we have adequate test coverage or protections against future bugs that would allow for low-difficulty headers to be accepted in some edge cases, then the existing checkpoints at least raise the cost of exploiting such bugs by a material amount. So I think this is worth further discussion. My view is that we should at least wait for #28043, and then have a discussion about the risk/reward for dropping the checkpoints. |
|
In any case I prefer to punt this until at least after the upcoming release branch-off :-) Meanwhile we should make hats and t-shirts with |
|
Open to manually test and review more the removal of mainnet checkpoints when there is momentum on moving forward to remove this logic from consensus. Reviewed last year the low-work-header DoS mitigation, always valuable to check again to prevent future bugs or issues circling in. |
|
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
This PR is now closed. If the want here is to move that PR forward, then people interested in this change, will want to review that PR. |
Once we have logic at the p2p layer to avoid permanently storing headers unless they lead to a sufficiently high work chain, we no longer need to use checkpoints to protect against spam from low-difficulty headers.
Waiting for #28043.