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 GetSnapshotBaseBlock() continues to return the snapshot base block even after validation completes. While m_ibd_chainstate->m_disabled is set to true when validation finishes, the active chainstate remains the snapshot chainstate until the next restart, when LoadChainstate() tears down both chainstates and reinitializes a single normal (non-snapshot) chainstate.

   Added !m_chainman.IsSnapshotValidated() 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. 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 `GetSnapshotBaseBlock()` continued returning non-null even after validation finished. Add `!IsSnapshotValidated()` check to only apply the restriction while background validation is ongoing.

   3e14e69a10
3. 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.

   9f946a79ca
4. DrahtBot added the label P2P on Oct 12, 2025
5. 
   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
   ACK	stratospher
   Concept ACK	TheCharlatan, Raimo33, 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:

   • #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.

6. 
   TheCharlatan commented at 8:41 pm on October 12, 2025: contributor

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

   cc @mzumsande
8. 
   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.
9. 
   Raimo33 commented at 8:33 am on October 14, 2025: contributor

   Concept ACK

   approach seems fine

10. 
    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

11. DrahtBot requested review from TheCharlatan on Oct 14, 2025
12. 
    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.
13. 
    mzumsande commented at 9:49 pm on October 15, 2025: contributor

    Concept ACK

Contributors
stringintech DrahtBot TheCharlatan fanquake Raimo33 stratospher mzumsande

Review Requested
TheCharlatan

Labels
P2P