consensus: Remove mainnet checkpoints

sdaftuar commented at 9:56 pm on July 27, 2022: member

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.

DrahtBot added the label Consensus on Jul 27, 2022

DrahtBot commented at 3:59 pm on July 28, 2022: contributor

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.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	michaelfolkson, Sjors, ariard, brunoerg, dergoegge

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26177 (refactor / kernel: Move non-gArgs chainparams functionality to kernel by TheCharlatan)

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.

michaelfolkson commented at 4:50 pm on July 28, 2022: contributor

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 on Aug 12, 2022

Remove mainnet checkpoints

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.

1b54186fcc

sdaftuar force-pushed on Aug 30, 2022

Sjors commented at 9:26 am on August 31, 2022: member

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 172f006020965ae8763a0610845c051ed1e3b522 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.

fanquake added this to the milestone Future on Aug 31, 2022

sdaftuar commented at 3:31 pm on September 7, 2022: member

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.

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.

Sjors commented at 4:09 pm on September 7, 2022: member

Meanwhile developers and other volunteers could run their node with -nocheckpoints to see if any attacks show up.

in src/chainparams.cpp:151 in 1b54186fcc

159-                {225430, uint256S("0x00000000000001c108384350f74090433e7fcf79a606b8e797f065b130575932")},
160-                {250000, uint256S("0x000000000000003887df1f29024b06fc2200b55f8af8f35453d7be294df2d214")},
161-                {279000, uint256S("0x0000000000000001ae8c72a0b0c301f67e3afca10e819efa9041e458e9bd7e40")},
162-                {295000, uint256S("0x00000000000000004d9b4ef50f0f9d686fd69db2e03af35a100370c64632a983")},
163+                {0, uint256S("0x000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f")},
164             }

ariard commented at 8:58 pm on February 7, 2023:

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.

ajtowns commented at 0:52 am on February 10, 2023:

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?

ariard commented at 11:06 pm on February 10, 2023:

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).

Sjors commented at 9:47 am on February 11, 2023:

I think it’s fine to drop the checkpoints in one release and then drop the checkpoint logic in a later release.

ariard commented at 9:07 pm on February 7, 2023: member

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.

brunoerg commented at 8:43 pm on February 16, 2023: contributor

Concept ACK …since #25719 has been merged.

DrahtBot added the label Needs rebase on Mar 16, 2023

DrahtBot commented at 2:33 pm on March 16, 2023: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot commented at 0:31 am on June 14, 2023: contributor

There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

DrahtBot commented at 2:03 am on September 12, 2023: contributor

There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

dergoegge commented at 12:50 pm on September 12, 2023: member

Concept ACK

For anyone interested in moving this forward, I opened #28043 to give us some more confidence in making this change.

achow101 commented at 4:58 pm on September 23, 2023: member

Are you still working on this? Given that #25717 has been long since merged, should this still be a draft?

sdaftuar commented at 5:06 pm on September 23, 2023: member

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.

Sjors commented at 7:37 pm on September 23, 2023: member

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 checkpoints=0, and also perhaps move that option out of --help-debug to --help (#28524).

ariard commented at 1:06 am on September 27, 2023: member

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.

DrahtBot commented at 0:01 am on December 26, 2023: contributor

There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

fanquake commented at 2:20 pm on March 6, 2024: member

Waiting for #28043.

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.

sdaftuar closed this on Mar 6, 2024

consensus: Remove mainnet checkpoints #25725

Code Coverage

Reviews

Conflicts