consensus: Remove checkpoints (take 2) #31649

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, implemented in #25717) 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
ACK	Sjors, dergoegge, instagibbs
Concept ACK	darosior, BrandonOdiwuor, ariard, glozow
Stale ACK	sipa, maflcko, l0rinc, mzumsande

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:

#31991 (RFC: Add operator""_uint256 compile-time user-defined literal by l0rinc)
#31981 (Add checkBlock() to Mining interface by Sjors)
#31859 (test: Rename send_message to send_without_ping by maflcko)
#31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
#31250 (wallet: Disable creating and loading legacy wallets by achow101)
#30595 (kernel: Introduce initial C header API by TheCharlatan)
#30342 (kernel, logging: Pass Logger instances to kernel objects by ryanofsky)
#29641 (scripted-diff: Use LogInfo over LogPrintf [WIP, NOMERGE, DRAFT] by maflcko)
#28710 (Remove the legacy wallet and BDB dependency by achow101)
#17783 (common: Disallow calling IsArgSet() on ALLOW_LIST options by ryanofsky)
#17581 (refactor: Remove settings merge reverse precedence code by ryanofsky)
#17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
#17493 (util: Forbid ambiguous multiple assignments in config file by ryanofsky)

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

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

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:583 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:4159 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 <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 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.

instagibbs commented at 4:25 PM on January 24, 2025:

Leaving the check in makes sense to me and can be removed later with little cost if desired.

in src/init.cpp:612 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

    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:904 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:906 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

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.

instagibbs commented at 5:13 PM on January 24, 2025: member

ACK 301017c6217378ca868e17c7cb8ffe3447abb11d

I think it makes sense to merge this shortly after 29.0 branchoff?

Confirmed that the removed tests would fail in the expected way under this branch, with the pre-checkpoint headers being accepted as a valid fork from testnet3 genesis.

All remaining references to "checkp" appear related to compact block filters or wallet db stuff.

DrahtBot requested review from dergoegge on Jan 24, 2025

DrahtBot requested review from BrandonOdiwuor on Jan 24, 2025

Armss9936 approved

dergoegge approved

dergoegge commented at 9:49 AM on February 4, 2025: member

ACK 301017c6217378ca868e17c7cb8ffe3447abb11d

Only a comment changed since my last review (https://github.com/bitcoin/bitcoin/pull/31649#pullrequestreview-2550177263).

sipa commented at 12:21 PM on February 4, 2025: member

utACK 301017c6217378ca868e17c7cb8ffe3447abb11d

maflcko commented at 1:54 PM on February 14, 2025: member

review ACK 301017c6217378ca868e17c7cb8ffe3447abb11d 🎸

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 301017c6217378ca868e17c7cb8ffe3447abb11d 🎸
W1mvb0D7M3mH4hEyQsXVg4p3mPt2I2zFZe8roehIKMIJKyGuwSxC1cmUBBznu01rJH4gvtktapJCFwF/DjiIAw==

</details>

fanquake added this to the milestone 30.0 on Feb 14, 2025

dergoegge commented at 11:11 AM on February 17, 2025: member

rfm?

achow101 commented at 7:50 PM on February 17, 2025: member

rfm?

I think this should be merged after v29.x branch off so that it can be tested in master for several months before being released in v30.0

dergoegge commented at 9:36 AM on February 18, 2025: member

I think this should be merged after v29.x branch off so that it can be tested in master for several months before being released in v30.0

It's obviously fine to wait until v30.0 but I don't see why we should? If things on master get additional testing, then we've been testing the headers pre-sync for ~2.5 years now. I don't think this needs anymore testing or what exactly are the expectations here, i.e. what tests are we missing?

l0rinc commented at 10:32 AM on February 21, 2025: contributor

To make sure this doesn't affect normal usage (as suggested by @glozow in https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2025-02-20#1091000), I ran an IBD with and without this change.

I have rebased the PR to latest master yesterday and ran it until 884000 with real peers, comparing it against master. It passed and didn't introduce any speed regression.

ACK 301017c6217378ca868e17c7cb8ffe3447abb11d

<details> <summary>Details</summary>

7db860c610451cf1c39b84746b61d4da3388c452 - Remove checkpoints
58f15d4b215705e07e23f1fc5d97a26261ef0da1 - Merge bitcoin/bitcoin#31379: cmake: Fix passing APPEND_*FLAGS to secp256k1 subtree.

COMMITS="7db860c610451cf1c39b84746b61d4da3388c452 58f15d4b215705e07e23f1fc5d97a26261ef0da1"; \
STOP_HEIGHT=884000; DBCACHE=10000; \
C_COMPILER=gcc; CXX_COMPILER=g++; \
hyperfine \
--export-json "/mnt/my_storage/ibd-${COMMITS// /-}-${STOP_HEIGHT}-${DBCACHE}-${C_COMPILER}.json" \
--runs 1 \
--parameter-list COMMIT ${COMMITS// /,} \
--prepare "killall bitcoind || true; rm -rf /mnt/my_storage/BitcoinData/*; git checkout {COMMIT}; git clean -fxd; git reset --hard; cmake -B build -DCMAKE_BUILD_TYPE=Release -DENABLE_WALLET=OFF -DCMAKE_C_COMPILER=$C_COMPILER -DCMAKE_CXX_COMPILER=$CXX_COMPILER && cmake --build build -j$(nproc) --target bitcoind && ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=1 -printtoconsole=0 || true" \
--cleanup "cp /mnt/my_storage/BitcoinData/debug.log /mnt/my_storage/logs/debug-{COMMIT}-$(date +%s).log || true" \
"COMPILER=$C_COMPILER COMMIT={COMMIT} ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=$STOP_HEIGHT -dbcache=$DBCACHE -printtoconsole=0"

Benchmark 1: COMPILER=gcc COMMIT=7db860c610451cf1c39b84746b61d4da3388c452 ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=884000 -dbcache=10000 -printtoconsole=0
  Time (abs ≡):        30600.425 s               [User: 55389.021 s, System: 1924.754 s]
 
Benchmark 2: COMPILER=gcc COMMIT=58f15d4b215705e07e23f1fc5d97a26261ef0da1 ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=884000 -dbcache=10000 -printtoconsole=0
  Time (abs ≡):        30689.328 s               [User: 55695.356 s, System: 2099.981 s]
 
Summary
  COMPILER=gcc COMMIT=7db860c610451cf1c39b84746b61d4da3388c452 ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=884000 -dbcache=10000 -printtoconsole=0 ran
    1.00 times faster than COMPILER=gcc COMMIT=58f15d4b215705e07e23f1fc5d97a26261ef0da1 ./build/src/bitcoind -datadir=/mnt/my_storage/BitcoinData -stopatheight=884000 -dbcache=10000 -printtoconsole=0

</details>

l0rinc approved

mzumsande commented at 8:41 PM on February 21, 2025: contributor

ACK 301017c6217378ca868e17c7cb8ffe3447abb11d (for v30)

I did a sanity check of the code paths leading to AddToBlockIndex, to verify that it should be impossible for us to accept a low-work header from an untrusted p2p source.

DrahtBot added the label Needs rebase on Mar 4, 2025

achow101 added the label Has ACKs on Mar 4, 2025

achow101 removed the label Has ACKs on Mar 4, 2025

glozow commented at 6:37 PM on March 12, 2025: member

concept ACK, seems rfm (after a rebase?)

update comment on MinimumChainWork check 632ae47372

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.

3c5d1a4681

marcofleon force-pushed on Mar 13, 2025

DrahtBot removed the label Needs rebase on Mar 13, 2025

Sjors commented at 2:57 PM on March 13, 2025: member

Concept ACK (again)

It would be good to mention #25717 in the PR description.

Digging through older issues, I found a few references to the function of checkpoints.

#4512 (comment):

With headers-first, checkpoint-based sigcheck skipping can be replaced by a "do not verify signatures when buried beneath enough PoW". We'll need a weaker form of checkpoints still to prevent the "fill your memory with a ton of low-difficulty headers" attack, but those don't need to lock in a specific chain.

This was implemented in #25717.

As is #5927#issue-62973803

We still need to prevent being fed a multitude of low-difficulty headers filling up our memory. The mechanism for that is unchanged for now: once a checkpoint is reached with headers, no headers chain branching off before that point are allowed anymore.

#6971 (comment)

I wanted to avoid a situation where a newly syncing node could be partitioned from the network by a malicious peer that served it a bogus headers chain.)

We still do this by not responding to getheaders before our tip has MinimumChainWork (which implies our headers are too).

#5927 (comment)

when a node starts up, once it gets through enough IBD, it's impossible to DOS it by just feeding it low difficulty full blocks, because those blocks would either have to be a fork before a checkpoint, or they wouldn't connect back to a valid chain. After this change, I think it's possible to push through a long low-difficulty fork before the node gets through the checkpoints. If the length is greater than the height of the last checkpoint, then from then on nothing prevents you from stuffing low difficulty blocks at this node forever until you fill up its disk or tie up its CPU. Once you've snuck in that low difficulty long fork at the beginning it's usable any time after.

This was dealt with in #5875. This still works without checkpoints, as we check unsolicited NetMsgType::BLOCK against GetAntiDoSWorkThreshold(), which would set min_pow_checked to false and is eventually stopped in AcceptBlockHeader.

(why doesn't net_processing just early return for !min_pow_checked && !forceProcessing? cc @mzumsande)

#7591 (comment)

They were initially added to the software as the result of another not-understood-at-the-time bug that harmed verification performance which was long since fixed.

And again:

Removing them entirely requires replacing the role they serve in preventing low difficulty header flooding attacks, which would need some design and development.

There was a paper written about this in 2019: https://gnusha.org/pi/bitcoindev/42cd5ffd-63e8-b738-c4ea-13d0699b1268@purse.io/ (though we ended up with a different solution)

There could be other documented functions I didn't find. Or undocumented functions. Or entirely unknown reasons to keep them.

Several discussions point out that the fixing the timewarp bug makes these attacks more difficult. Though possibly only when limiting the "width" of header alternatives to consider, which we don't want to do. We could wait for such a soft fork to activate as an extra precaution.

However at this point the last checkpoint offers rather little security if such an attack appears in the wild, as mentioned in #31649#pullrequestreview-2550177263.

We'd either have to introduce a fresh checkpoint or mitigate the attack in a nicer way. Adding a fresh checkpoint can be combined with a partial revert of this PR. Solving the attack in some other way may even be slightly easier without the need to consider our checkpoint code.

marcofleon commented at 3:20 PM on March 13, 2025: contributor

Rebased after 44041ae0eca9d2034b7c2bdef24724809cc35e90 and added #25717 in the description.

Sjors commented at 3:21 PM on March 13, 2025: member

Code review ACK 3c5d1a468199722da620f1f3d8ae3319980a46d5

If #31974 lands then this PR will be more difficult to revert. So it might be marginally better to drop the checkpoints in a separate commit. Here's a commit sequence that would work.

DrahtBot requested review from l0rinc on Mar 13, 2025

DrahtBot requested review from mzumsande on Mar 13, 2025

DrahtBot requested review from instagibbs on Mar 13, 2025

DrahtBot requested review from sipa on Mar 13, 2025

DrahtBot requested review from dergoegge on Mar 13, 2025

DrahtBot requested review from glozow on Mar 13, 2025

DrahtBot requested review from maflcko on Mar 13, 2025

dergoegge approved

dergoegge commented at 4:53 PM on March 13, 2025: member

ACK 3c5d1a468199722da620f1f3d8ae3319980a46d5

instagibbs commented at 5:32 PM on March 13, 2025: member

reACK 3c5d1a468199722da620f1f3d8ae3319980a46d5

fanquake merged this on Mar 14, 2025

fanquake closed this on Mar 14, 2025