consensus: Remove checkpoints (take 2)

marcofleon commented at 3:38 pm on January 13, 2025: contributor

The headers presync logic (only downloading headers that lead to a chain with sufficient work) should be enough to prevent memory DoS using low-work headers. Therefore, we no longer have any use for checkpoints.

All checkpoints and checkpoint logic are removed in a single commit, to make it easy to revert if necessary.

Some previous discussion can be found in #25725. The conclusion at the time was that more testing of the presync logic was needed. Now that we have unit, functional, and fuzz tests for this logic, it seems safe to move forward with checkpoint removal.

DrahtBot commented at 3:38 pm on January 13, 2025: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31649.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	darosior, l0rinc, Sjors, BrandonOdiwuor, ariard
Stale ACK	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:

#31250 (wallet: Disable creating and loading legacy wallets by achow101)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
#28710 (Remove the legacy wallet and BDB dependency by achow101)

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.

DrahtBot added the label Consensus on Jan 13, 2025

sipa commented at 4:44 pm on January 13, 2025: member

If we go through with this, perhaps it’s worth keeping the -checkpoints option around, just to trigger a startup warning if provided (as opposed to an error if someone still has it in the config).

dergoegge commented at 4:58 pm on January 13, 2025: member

Concept ACK

Sjors commented at 5:29 pm on January 13, 2025: member

You can delete blockheader_testnet3.hex as it’s no longer used and testnet3 may get deprecated anyway. #31583 introduces something similar based on mainnet.

marcofleon force-pushed on Jan 13, 2025

marcofleon commented at 6:06 pm on January 13, 2025: contributor

Updated the -checkpoints option and added a warning (similar to -upnp). Also removed blockheader_testnet3.hex. Thanks!

darosior commented at 7:43 pm on January 13, 2025: member

Concept ACK.

in src/consensus/validation.h:66 in a694e1d5f8 outdated

62@@ -63,7 +63,6 @@ enum class BlockValidationResult {
63     BLOCK_MISSING_PREV,      //!< We don't have the previous block the checked one is built on
64     BLOCK_INVALID_PREV,      //!< A block this one builds on is invalid
65     BLOCK_TIME_FUTURE,       //!< block timestamp was > 2 hours in the future (or our clock is bad)
66-    BLOCK_CHECKPOINT,        //!< the block failed to meet one of our checkpoints

l0rinc commented at 8:01 pm on January 13, 2025:

To guard against BLOCK_HEADER_LOW_WORK changing its numeric value now (to 8 instead of previous 9), we could hard-code the previous values for the remaining ones

 0enum class BlockValidationResult {
 1    BLOCK_RESULT_UNSET    = 0, //!< initial value. Block has not yet been rejected
 2    BLOCK_CONSENSUS       = 1, //!< invalid by consensus rules (excluding any below reasons)
 3    BLOCK_CACHED_INVALID  = 2, //!< this block was cached as being invalid and we didn't store the reason why
 4    BLOCK_INVALID_HEADER  = 3, //!< invalid proof of work or time too old
 5    BLOCK_MUTATED         = 4, //!< the block's data didn't match the data committed to by the PoW
 6    BLOCK_MISSING_PREV    = 5, //!< We don't have the previous block the checked one is built on
 7    BLOCK_INVALID_PREV    = 6, //!< A block this one builds on is invalid
 8    BLOCK_TIME_FUTURE     = 7, //!< block timestamp was > 2 hours in the future (or our clock is bad)
 9 // BLOCK_CHECKPOINT      = 8, //!< Deprecated and removed
10    BLOCK_HEADER_LOW_WORK = 9  //!< the block header may be on a too-little-work chain
11};

Sjors commented at 11:31 am on January 14, 2025:

I was wondering if these are stored to disk as part of the block index, or exposed via RPC. Otherwise changing their numeric values shouldn’t matter.

Sjors commented at 11:37 am on January 14, 2025:

Judging from the change to ipc_test.cpp below it seems that eventually (#10102) these values will be sent over IPC. At that point we have to fix their integer values. Might as well do it now? cc @ryanofsky

Should be a separate commit, since we would not want to revert that.

maflcko commented at 1:12 pm on January 14, 2025:

these values will be sent over IPC. At that point we have to fix their integer values. Might as well do it now?

Why would they need to be fixed? IPC is only an internal interface. It is currently experimental and in draft and there are no plans for the interface to support mismatching server/client binaries. I’d say the code should be left as-is until there are plans (with support) to even support that. Until then, changing the code for that reason seems like pointless churn to me.

Sjors commented at 1:18 pm on January 14, 2025:

no plans for the interface to support mismatching server/client binaries

That tends to happen without a plan, but indeed we can wait.

part of the block index, or exposed via RPC

^ this still need to be clarified

sipa commented at 7:09 pm on January 14, 2025:

part of the block index, or exposed via RPC

I don’t believe either of these to be the case.

in src/node/blockstorage.cpp:582 in a694e1d5f8 outdated

574@@ -575,20 +575,6 @@ void BlockManager::ScanAndUnlinkAlreadyPrunedFiles()
575     UnlinkPrunedFiles(block_files_to_prune);
576 }
577 
578-const CBlockIndex* BlockManager::GetLastCheckpoint(const CCheckpointData& data)
579-{
580-    const MapCheckpoints& checkpoints = data.mapCheckpoints;
581-
582-    for (const MapCheckpoints::value_type& i : checkpoints | std::views::reverse) {

l0rinc commented at 8:06 pm on January 13, 2025:

you can likely remove #include <ranges> now

in src/net_processing.cpp:4154 in a694e1d5f8 outdated

4148-            LogDebug(BCLog::NET, "Ignoring getheaders from peer=%d because active chain has too little work; sending empty response\n", pfrom.GetId());
4149-            // Just respond with an empty headers message, to tell the peer to
4150-            // go away but not treat us as unresponsive.
4151-            MakeAndPushMessage(pfrom, NetMsgType::HEADERS, std::vector<CBlockHeader>());
4152-            return;
4153-        }

darosior commented at 8:11 pm on January 13, 2025:

Seems worth keeping, at least for a few versions?

Sjors commented at 11:34 am on January 14, 2025:

Indeed seems better to keep this. Perhaps mention the last mainnet checkpoint height, since MinimumChainWork() is far above that.

marcofleon commented at 2:08 pm on January 14, 2025:

I agree that if there were code to keep for now it would be this. I ultimately decided to remove it because the purpose it serves no longer seems to apply if we remove checkpoints. With the headers sync logic, iiuc, it would be fine for our node in IBD to respond to GETHEADERS because the only headers we could have would belong to a chain with enough work. There isn’t a risk anymore of sending a bogus chain to a peer and getting disconnected.

This behavior was first introduced in #6172 and then changed to MinimumChainWork() in #24178. Some more discussion can be found in #21106 (comment) and #21106 (comment).

Let me know if there’s something I’m missing here, wrt to the logic of why it can be safely removed. If we do keep it, then I should at least update the comment (to explain why the check is still there?).

sipa commented at 2:48 pm on January 14, 2025:

The only way I can see this still being a problem is:

Someone started a node with 23.x or earlier (before #25717)
Started being fed a low-work headers chain.
Upgraded to 30.x(?) or later (with this PR)
Never made any headers sync progress since.

That seems pretty unlikely, so I think it’s fine to remove this if/when we get rid of checkpoints.

darosior commented at 4:03 pm on January 14, 2025:

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 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 darosior: so you’d detect it quickly and disconnect but be left with headers that are possibly checkpoint-violating (if your node isn’t enforcing checkpoints)

Sjors commented at 4:18 pm on January 14, 2025:

It seems that we respond 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)

Even without a malicious peer involved, we might be only part way in obtaining headers from an honest peer. If another peer then wants headers from us, IIUC we’d be wasting their time because we can’t send them enough.

sipa commented at 5:36 pm on January 14, 2025:

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 avoid offering peers to sync from us before we’re at a point where we can offer enough that we’d accept it ourselves, like @Sjors points out.

I’d vote to keep it, but update the comments.

mzumsande commented at 6:42 pm on January 14, 2025:

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 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 and are unlikely to get inbound peers anyway.

marcofleon commented at 2:48 pm on January 15, 2025:

Sounds good, I kept the check and adjusted the comment in a separate commit. Let me know if it makes sense or can be improved in any way.

in src/init.cpp:611 in a694e1d5f8 outdated

606@@ -607,7 +607,8 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc)
607     argsman.AddArg("-checkblockindex", strprintf("Do a consistency check for the block tree, chainstate, and other validation data structures every <n> operations. Use 0 to disable. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
608     argsman.AddArg("-checkaddrman=<n>", strprintf("Run addrman consistency checks every <n> operations. Use 0 to disable. (default: %u)", DEFAULT_ADDRMAN_CONSISTENCY_CHECKS), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
609     argsman.AddArg("-checkmempool=<n>", strprintf("Run mempool consistency checks every <n> transactions. Use 0 to disable. (default: %u, regtest: %u)", defaultChainParams->DefaultConsistencyChecks(), regtestChainParams->DefaultConsistencyChecks()), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
610-    argsman.AddArg("-checkpoints", strprintf("Enable rejection of any forks from the known historical chain until block %s (default: %u)", defaultChainParams->Checkpoints().GetHeight(), DEFAULT_CHECKPOINTS_ENABLED), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
611+    // Checkpoints were removed. We keep `-checkpoints` as a hidden arg to display a more user friendly error when set.
612+    argsman.AddArg("-checkpoints", "", ArgsManager::ALLOW_ANY, OptionsCategory::HIDDEN);

l0rinc commented at 8:12 pm on January 13, 2025:

usually there’s a single line per arg here

0    argsman.AddArg("-checkpoints", "", ArgsManager::ALLOW_ANY, OptionsCategory::HIDDEN); // Checkpoints were removed. We keep `-checkpoints` as a hidden arg to display a more user friendly error when set.

marcofleon commented at 2:12 pm on January 14, 2025:

I used the same format as the -upnp option above. Should be removed soon after (if this gets merged) so probably doesn’t matter too much.

in src/init.cpp:881 in a694e1d5f8 outdated

877@@ -877,6 +878,11 @@ bool AppInitParameterInteraction(const ArgsManager& args)
878         InitWarning(_("Option '-upnp' is set but UPnP support was dropped in version 29.0. Consider using '-natpmp' instead."));
879     }
880 
881+    // We removed checkpoints but keep the option to warn users who still have it in their config.

l0rinc commented at 8:13 pm on January 13, 2025:

this is already obvious from the error, the comment is redundant

marcofleon commented at 2:12 pm on January 14, 2025:

Same thing here, just imitating the -upnp format above.

in src/kernel/chainparams.h:27 in a694e1d5f8 outdated

23@@ -24,17 +24,6 @@
24 #include <utility>
25 #include <vector>
26 
27-typedef std::map<int, uint256> MapCheckpoints;

l0rinc commented at 8:13 pm on January 13, 2025:

#include <map> and a few others can likely be removed now

l0rinc changes_requested

l0rinc commented at 8:20 pm on January 13, 2025: contributor

Concept ACK The remaining “checkpoint” references in the code seem unrelated.

in src/init.cpp:883 in a694e1d5f8 outdated

877@@ -877,6 +878,11 @@ bool AppInitParameterInteraction(const ArgsManager& args)
878         InitWarning(_("Option '-upnp' is set but UPnP support was dropped in version 29.0. Consider using '-natpmp' instead."));
879     }
880 
881+    // We removed checkpoints but keep the option to warn users who still have it in their config.
882+    if (args.IsArgSet("-checkpoints")) {
883+        InitWarning(_("Option '-checkpoints' is set but checkpoints were removed. This option has no effect."));

Sjors commented at 11:46 am on January 14, 2025:

This is a nice improvement. I’ve been running my node with -nocheckpoints=1 for years, so now it will just throw a warning rather than abort launch. (which I tested)

Sjors commented at 11:51 am on January 14, 2025: member

Concept ACK

If we go ahead with this, right after the v29 branch-off would seem like a good time.

I plan to expand the alternate mainnet chain in #31583 to 11111+ blocks and have ~p2p_dos_header_tree.py` use that instead. That would make it easier to revert the checkpoints even after testnet3 is completely dropped.

(update: actually that test is hard to replace without also replaying the real mainnet)

marcofleon force-pushed on Jan 14, 2025

mzumsande commented at 3:50 pm on January 14, 2025: contributor

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

dergoegge commented at 3:50 pm on January 14, 2025: member

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 only raise the cost of attack slightly to an ever shrinking fraction of the cost of mining a block at the tip (see https://bitcoincore.org/en/2024/09/18/disclose-headers-oom/).

If we are not comfortable with removing the checkpoints, then more of them should be added, as the protections they provide will converge towards zero otherwise.

Philosophically it is nice to get rid of the checkpoints, as they are one of the only things in this code base that dictate what the canonical chain is, which isn’t a position we want to be in (this concern was also part of the design considerations for features such as assumevalid and assumeutxo).

I think we could reasonably ship this removal in v29.0, as I don’t see what additional assurances waiting even longer would give us.

DrahtBot requested review from l0rinc on Jan 14, 2025

DrahtBot requested review from Sjors on Jan 14, 2025

DrahtBot requested review from darosior on Jan 14, 2025

Sjors commented at 4:14 pm on January 14, 2025: member

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.

marcofleon commented at 5:32 pm on January 14, 2025: contributor

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.

luke-jr commented at 10:38 pm on January 14, 2025: member

All checkpoints and checkpoint logic are removed in a single commit, to make it easy to revert if necessary.

It’s easy to revert an entire merge, so feel free to split it up if it makes sense to…

marcofleon force-pushed on Jan 15, 2025

update comment on MinimumChainWork check c59cbba518

Remove checkpoints

The headers presync logic should be enough to prevent memory DoS using
low-work headers. Therefore, we no longer have any use for checkpoints.

301017c621

marcofleon force-pushed on Jan 17, 2025

BrandonOdiwuor commented at 6:21 pm on January 20, 2025: contributor

Concept ACK

ariard commented at 9:10 pm on January 20, 2025: contributor

Concept ACK.

consensus: Remove checkpoints (take 2) #31649

Code Coverage & Benchmarks

Reviews

Conflicts