Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

p2p: For assumeutxo, download snapshot chain before background chain #29519

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+4 −1
Open

p2p: For assumeutxo, download snapshot chain before background chain #29519

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1397,9 +1397,12 @@ void PeerManagerImpl::FindNextBlocksToDownload(const Peer& peer, unsigned int co
return;
}

if (state->pindexLastCommonBlock == nullptr) {
std::optional<int> snapshot_height{m_chainman.GetSnapshotBaseHeight()};
if (state->pindexLastCommonBlock == nullptr ||
(snapshot_height.has_value() && state->pindexLastCommonBlock->nHeight < snapshot_height.value())) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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())];
     }

// 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())];
}

Expand Down