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

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.

DrahtBot added the label P2P on Oct 12, 2025

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	fjahr, sedited, achow101
Concept ACK	Raimo33, mzumsande
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

No conflicts as of last run.

LLM Linter (✨ experimental)

Possible places where comparison-specific test macros should replace generic comparisons:

test/functional/feature_assumeutxo.py: “assert int(forking_node.getblockchaininfo()[‘chainwork’], 16) > forking_node_old_chainwork” -> recommendation: use assert_greater_than(int(forking_node.getblockchaininfo()[‘chainwork’], 16), forking_node_old_chainwork)

2026-01-24 19:23:44

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

Nice, Concept ACK.

fanquake commented at 8:45 pm on October 12, 2025: member

cc @mzumsande

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.

Raimo33 commented at 8:33 am on October 14, 2025: contributor

Concept ACK

approach seems fine

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

DrahtBot requested review from sedited on Oct 14, 2025

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.

mzumsande commented at 9:49 pm on October 15, 2025: contributor

Concept ACK

DrahtBot added the label Needs rebase on Dec 16, 2025

fjahr commented at 6:49 pm on December 17, 2025: contributor

Concept ACK

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.

stringintech force-pushed on Dec 18, 2025

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.

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

stringintech force-pushed on Dec 18, 2025

DrahtBot added the label CI failed on Dec 18, 2025

DrahtBot removed the label CI failed on Dec 18, 2025

DrahtBot removed the label Needs rebase on Dec 18, 2025

fjahr commented at 4:11 pm on December 19, 2025: contributor

tACK 5cd096803d223166b9aa36e783406a36cdb150bc

Reviewed code and confirmed the new test failed previous to the behavior change.

DrahtBot requested review from mzumsande on Dec 19, 2025

DrahtBot requested review from stratospher on Dec 19, 2025

in test/functional/feature_assumeutxo.py:357 in 5cd096803d

352+        After background validation completes, node should be able
353+        to download and process blocks from peers without the snapshot block in their chain.
354+        """
355+        self.log.info("Testing sync to the most-work chain without the snapshot block after background validation")
356+
357+        miner = self.nodes[0]

sedited commented at 9:59 pm on January 23, 2026:

Nitty nit: I think it would be a bit clearer if you’d call this forking_node, or something along the lines.

sedited approved

sedited commented at 10:03 pm on January 23, 2026: contributor

ACK 5cd096803d223166b9aa36e783406a36cdb150bc

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.

7d9e1a8102

stringintech force-pushed on Jan 24, 2026

stringintech commented at 8:29 pm on January 24, 2026: contributor

5cd0968 to 7d9e1a8 - Addressed #33604 (review).

fjahr commented at 10:17 pm on January 24, 2026: contributor

Re-ACK 7d9e1a810239a65a153c35f0f94490560441db49

Only change since last review was a variable renaming.

DrahtBot requested review from sedited on Jan 24, 2026

sedited approved

sedited commented at 2:09 pm on January 28, 2026: contributor

Re-ACK 7d9e1a810239a65a153c35f0f94490560441db49

achow101 commented at 11:00 pm on January 29, 2026: member

ACK 7d9e1a810239a65a153c35f0f94490560441db49

achow101 merged this on Jan 29, 2026

achow101 closed this on Jan 29, 2026

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

Code Coverage & Benchmarks

Reviews

Conflicts

LLM Linter (✨ experimental)