p2p: Allow block downloads from peers without snapshot block after assumeutxo validation #33604

1. 
   stringintech commented at 8:36 pm on October 12, 2025: contributor

   Currently, after assumeutxo background validation finishes, the node continues to skip peers that don’t have the snapshot block in their best chain until restart. This unnecessarily excludes peers from block downloads even though the background sync has completed and undo data is available.

   The restriction persists because m_chainman.CurrentChainstate().SnapshotBase() continues to return the snapshot base block until restart, even after validation completes. Added m_chainman.CurrentChainstate().m_assumeutxo == Assumeutxo::UNVALIDATED check to only apply the peer restriction while background validation is ongoing.

   Also added test coverage in feature_assumeutxo.py that verifies peers without the snapshot block can be used for block downloads after background validation completes. The test fails without this fix.

2. DrahtBot added the label P2P on Oct 12, 2025
3. 
   DrahtBot commented at 8:36 pm on October 12, 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/33604.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	sedited, Raimo33, mzumsande, fjahr
   Stale ACK	stratospher

   If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #30214 (refactor: Improve assumeutxo state representation 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.

4. 
   sedited commented at 8:41 pm on October 12, 2025: contributor

   Nice, Concept ACK.
5. 
   fanquake commented at 8:45 pm on October 12, 2025: member

   cc @mzumsande
6. 
   stringintech commented at 9:47 pm on October 12, 2025: contributor

   This change allows the snapshot chainstate to reorg to chains that don’t include the snapshot block after validation completes (not sure yet if this is the only mechanism that would allow such reorgs), so we must not assume anywhere in the codebase that SnapshotBase() is always an ancestor of the active tip. I’ll take a closer look to see if any such assumptions exist, but I thought this was worth noting here.
7. 
   Raimo33 commented at 8:33 am on October 14, 2025: contributor

   Concept ACK

   approach seems fine

8. 
   stratospher commented at 6:22 pm on October 14, 2025: contributor

   ACK 9f946a7.

   nice! I think this aligns with the original intention to restrict the peer only when background sync isn’t over in #29519 (review) where this code was introduced.

   looks like we might incorrectly set pindexLastCommonBlock in L1401 to the previous best known block tip again in this scenario. but it’s not a problem since we immediately reset it back to the correct value + comments suggest it’s ok to set it incorrectly and that’s way better than introducing more conditions.

   also slightly related: https://github.com/bitcoin/bitcoin/issues/30288

9. DrahtBot requested review from sedited on Oct 14, 2025
10. 
    stringintech commented at 10:41 am on October 15, 2025: contributor

    Thanks for the links @stratospher! FYI, calculating pindexLastCommonBlock is undergoing some improvements in #32180, and I will probably have to rebase onto that change. But either way, as you mentioned, it shouldn’t cause any issues.
11. 
    mzumsande commented at 9:49 pm on October 15, 2025: contributor

    Concept ACK
12. DrahtBot added the label Needs rebase on Dec 16, 2025
13. 
    fjahr commented at 6:49 pm on December 17, 2025: contributor

    Concept ACK
14. in src/net_processing.cpp:1388 in 3e14e69a10 outdated

    1387@@ -1388,7 +1388,7 @@ void PeerManagerImpl::FindNextBlocksToDownload(const Peer& peer, unsigned int co
    1388     // We can't reorg to this chain due to missing undo data until the background sync has finished,
    

    ---

    ---

    

    fjahr commented at 6:53 pm on December 17, 2025:

    nit: Could clarify the behavior in the comment here a bit more.
15. stringintech force-pushed on Dec 18, 2025
16. 
    stringintech commented at 9:02 am on December 18, 2025: contributor

    9f946a7 to 5cd0968 (compare) - rebased, resolved conflict with #30214, and addressed #33604 (review). Also PR and first commit descriptions are updated.

    edit: second force push (6b92a69 to 5cd0968) is to drop an accidentally included iostream header.

17. p2p: Allow block downloads from peers without snapshot block after assumeutxo validation

    After assumeutxo background validation completes, allow block downloads from peers that don't have the snapshot block in their best chain.
    
    Previously, these peers were skipped until restart because `m_chainman.CurrentChainstate().SnapshotBase()` continued returning non-null even after validation finished. Add `m_chainman.CurrentChainstate().m_assumeutxo == Assumeutxo::UNVALIDATED` check to only apply the restriction while background validation is ongoing.

    0067abe153
18. test: Verify peer usage after assumeutxo validation completes

    Add test coverage to ensure peers without the snapshot block in their chain can be used for block downloads after background validation completes. The test fails without the fix in the previous commit.

    5cd096803d
19. stringintech force-pushed on Dec 18, 2025
20. DrahtBot added the label CI failed on Dec 18, 2025
21. DrahtBot removed the label CI failed on Dec 18, 2025
22. DrahtBot removed the label Needs rebase on Dec 18, 2025

Contributors
stringintech DrahtBot sedited fanquake Raimo33 stratospher mzumsande fjahr

Review Requested
sedited

Labels
P2P