In commit "p2p: For assumeutxo, download snapshot chain before background chain" (ac547ea)

It seems like the pindexLastCommonBlock->nHeight < snapshot_height check is not strict enough because the snapshot block might not necessarily be an ancestor the peer's best known block.

In this case, the pindexLastCommonBlock assignments on line 1406 and 1411 will reset it back to the fork point between the two nodes each time FindNextBlocksToDownload is called. And if the peer's best block is more than BLOCK_DOWNLOAD_WINDOW blocks away from the current chain, the node will never be able to download enough blocks to reach it and validate it. This means if the snapshot is not on the best chain, the node may be unable to sync to a better chain.

Suggestion to fix would be:

--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -1487,12 +1487,14 @@ void PeerManagerImpl::FindNextBlocksToDownload(const Peer& peer, unsigned int co
         return;
     }
 
-    std::optional<int> snapshot_height{m_chainman.GetSnapshotBaseHeight()};
+    // If pindexLastCommonBlock has not been set yet, set it now. Also reset it
+    // if an assumetxo snapshot has been loaded and it is an ancestor of the
+    // snapshot, so only blocks after the snapshot will be downloaded.
+    const CBlockIndex* snap_base{m_chainman.GetSnapshotBaseBlock()};
     if (state->pindexLastCommonBlock == nullptr ||
-        (snapshot_height.has_value() && state->pindexLastCommonBlock->nHeight < snapshot_height.value())) {
+        (snap_base && snap_base->GetAncestor(state->pindexLastCommonBlock->nHeight) == state->pindexLastCommonBlock)) {
         // Bootstrap quickly by guessing a parent of our best tip is the forking point.
         // Guessing wrong in either direction is not a problem.
-        // Also applies after loading a snapshot, when we want to prioritise loading the snapshot chain and therefore need to skip ahead.
         state->pindexLastCommonBlock = m_chainman.ActiveChain()[std::min(state->pindexBestKnownBlock->nHeight, m_chainman.ActiveChain().Height())];
     }
 

This means if the snapshot is not on the best chain, the node may be unable to sync to a better chain.

I've thought about this for a while, and I'm not sure if that problem is really being fixed by your suggestion for the following (slightly philosophical) reasons:

• Accepting a snapshot means that we've created a new chainstate with its Active Tip being set to that block.
• That is a commitment, we can't just reorg away from it to another chain that doesn't contain the snapshot block, even if such a chain existed - at least not until the background sync has finished (we'd need the full blocks below the snapshot), and once that has happened, the snapshot chainstate gets deleted.
• If all of our peers are on a better chain not containing the Snapshot, it seems to me that we are in a hopeless situation anyway: With the current patch we might download no blocks from those peers, with your suggestion we'd download useless blocks that can't be connected - I'm not sure why one is better than the other.

So I think that the premise that we could get into this situation should be rejected:
If a valid snapshot was provided and put into a release, it should only be possible if there was a massive reorg of multiple thousands of blocks (and we'd also need to have accepted header chains ending in both chaintips from different peers). I'd say that in this hypothetical situation, AssumeUtxo is not at the top of the list of our concerns - bitcoin would have far bigger problems.

Without a massive reorg, there is the alternative that the snapshot provided was bad somehow. But then (apart from the fact that it shouldn't have been hardcoded into chainparams), it also shouldn't have been loaded in the first place because its header should not have been accepted to our block index with our header DoS protections.

So all in all I think that once we've accepted a snapshot, we've made a commitment and have to treat it as part of the best chain, at least until background sync has finished.

re: #29519 (comment)

Good points. My suggested change was premised on the false assumption that the node would be able to seamlessly reorg to the most-work chain. But that's not true because of missing undo data.

Your philosophical point is interesting. I believe that:

• Loading snapshots should not affect how nodes come to consensus, it should only affect what order they download blocks, and help them compute the latest UTXO sets sooner.

And you seem to be suggesting:

• Loading a snapshot should force a node to only consider chains that include the snapshot block and ignore other chains, even if they have more work.

On this PR more specifically:

When I made my suggested change, I was forgetting that we don't have any undo data for the snapshot block and blocks before it, so unfortunately it is not possible for a node to follow the most-work chain when a snapshot is loaded until all the preceding blocks are downloaded.

Given this fact, I think the best thing FindNextBlocksToDownload can do when an unvalidated snapshot is loaded is to ignore peers on other chains not containing the snapshot block, until it actually has the blocks and undo data that would be needed to validate other chains. I think the current PR is not exactly doing this though. It looks like it will still download up to BLOCK_DOWNLOAD_WINDOW useless blocks on the peer's chain, starting at the fork point between the two chains, which is not great. It will also not download later blocks after snapshot is validated, when undo data needed to enable a re-org becomes available.

On the broader approach to downloading blocks when a snapshot is loaded:

I'd agree now that the only sensible block downloading behavior for an unvalidated snapshot chainstate is to only download blocks after the snapshot (due to lack of undo data before then) and ignore any peers whose best blocks don't descend from the snapshot block.

But it is less clear which blocks should be downloaded and attached to the older chainstate, and whether it should target the most-work block or the snapshot block. Since d43a1f1 from #27746 it targets the snapshot block, but was not doing that originally. Comparing the two alternatives:

• Targeting the most-work block seems like a purer approach that would probably simplify the code and remove special cases, and would also have the benefit of the node syncing to the most-work chain faster, with less wasted work if it the chain turns out to be valid.

• Targeting the snapshot block is also very reasonable, and is what the code is doing currently, and guarantees the snapshot will be validated even if it is not used, and could be a practical defense against eclipse attacks, or fork chains that appear to have more work but turn out to be invalid.

On whether we should care about any of this:

I don't think we need to care about these design choices too much, since everything above is referring to a case we don't expect to encounter, where there is a possibly-valid chain of blocks, not including the snapshot block, that has more work than any known chain including the snapshot block. The only time when we should see this is when there is a major fork with a lot of work behind it.

However, I don't think "the premise that we could get into this situation should be rejected." That seems like it is going too far. We should put some thought into how code will behavior in presence for forks for practical reasons, and also to inform design of the code and to write it in a clean way that isn't full of assumptions and special cases.

I do think, as long as we are hardcoding snapshot hashes in the source code, we can reject premise that snapshot chainstate turns out to be invalid, and just refuse to run in that case. But the case where the snapshot is valid, but the snapshot block is not on the most-work chain seems different and worth thinking about.

And you seem to be suggesting:
Loading a snapshot should force a node to only consider chains that include the snapshot block and ignore other chains, even if they have more work.

This was meant more as a description of the status quo than a description of the ideal world. I think that with today's logic we don't really have another choice but to do so - temporarily until the background sync has finished.

but the snapshot block is not on the most-work chain seems different and worth thinking about

I agree with that.Some possible ways to deal with that:

1.) Tighten the criteria for snapshot acceptance: We could reject loading the snapshot if the snapshot block is not a predecessor of m_best_header. Currently, we only check that it has more work than the ActiveTip. This is easy to implement, though it doesn't cover the case where we only learn about the better chain after the snapshot has been loaded.

2.) After a snapshot is accepted and we detect that the snapshot is no longer on the most-work chain we know of, we could abort syncing the snapshot, delete the chainstate and go back to normal sync mode. This sounds more complicated to implement, and I'm not convinced it's worth it.

It looks like it will still download up to BLOCK_DOWNLOAD_WINDOW useless blocks on the peer's chain, starting at the fork point between the two chains, which is not great.

That's true. I will try to adjust the PR to deal with that. Since I find the logic in FindNextBlocksToDownload / TryDownloadingHistoricalBlocks / FindNextBlocks rather hard to understand, I would like to reproduce the situation in a test (at least locally, sounds like timing issues could make this hard to include in a PR) to be more confident that this is really how it works, which could take a few days.

I have now added a first commit that avoids downloading blocks not on the snapshot chain completely.
Local testing revealed that not only are we unable to reorg to a more-work chain, we also won't fail gracefully: On master, we would still download the blocks and attempt to reorg, but then the node would just crash.

7a88551: although this scenario is worthy of LogWarning, we have to be careful to avoid a log spam attack. So for now LogDebug is fine.