p2p: avoid traversing blocks (twice) during IBD

furszy commented at 4:54 pm on June 11, 2025: member

Context: Every time a block is connected, a BlockConnected() event is appended to the validation interface queue. This queue is consumed sequentially by a single worker thread. To avoid excessive memory usage, the queue is limited to 10 events at a time, and we stop processing new blocks until the queued events are handled.

Issue: Within the PeerManager::BlockConnected() listener, we traverse the block twice inside the transaction download manager — despite not needing to handle orphans or transaction requests during IBD.

Extra info about the changes: The new arg added to BlockConnected() in the first commit is primarily meant to avoid locking cs_main on the listener side (to avoid calling IsInitialBlockDownload() there). Another way of implementing this could be to add a bool field enabled to the TxDownloadManager class that is only set to true when we are out of IBD, so that we don’t have to include a new arg inside the block connection event. This is open for discussion. Could try implementing this second approach too.

DrahtBot commented at 4:54 pm on June 11, 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/32730.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32941 (p2p: TxOrphanage revamp cleanups by glozow)
#32827 (mempool: Avoid needless vtx iteration during IBD by l0rinc)
#32631 (refactor: Convert GenTxid to std::variant by marcofleon)
#32189 (refactor: Txid type safety (parent PR) by marcofleon)
#31829 (p2p: improve TxOrphanage denial of service bounds by glozow)
#30214 (refactor: Improve assumeutxo state representation by ryanofsky)
#30116 (p2p: Fill reconciliation sets (Erlay) attempt 2 by sr-gi)
#29415 (Broadcast own transactions only via short-lived Tor or I2P connections by vasild)

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.

DrahtBot added the label P2P on Jun 11, 2025

in src/txorphanage.cpp:250 in e48bfeb58f outdated

245@@ -246,6 +246,9 @@ bool TxOrphanage::HaveTxToReconsider(NodeId peer)
246 
247 void TxOrphanage::EraseForBlock(const CBlock& block)
248 {
249+    // No orphans, nothing to do
250+    if (m_orphans.empty()) return;

l0rinc commented at 5:08 pm on June 11, 2025:

This makes sense on its own I think - it’s similar to another draft PR I had (https://github.com/bitcoin-dev-tools/benchcoin/pull/167). If you think it’s related and adds anything, feel free to cherry-pick it here.

instagibbs commented at 6:00 pm on June 11, 2025:

this is a common-sense change by itself :+1:

furszy commented at 7:20 pm on June 11, 2025:

tiny note: this also skip post-IBD blocks when we don’t have any orphans. –Credits to @mzumsande for pointing it out too– We could do the same for the tx requests tracker loop but.. that wouldn’t change much as we still need to traverse the block for the “recently confirmed” filter anyway.

furszy commented at 7:39 pm on June 11, 2025:

This makes sense on its own I think - it’s similar to another draft PR I had (bitcoin-dev-tools#167). If you think it’s related and adds anything, feel free to cherry-pick it here.

That’s cool, will check it in the coming days. First quick glance it seems that if the mempool is empty; we could avoid calling removeForBlock entirely. Which will save us from adding another callback into the validation queue too, which will leave space for processing another block during ibd. Yet, that was a fast check. Should check who is using the MempoolTransactionsRemovedForBlock event.

l0rinc commented at 6:25 am on June 18, 2025:

To simplify review, could you split it into several commits - one changing the m_initial_sync(_finished) implementation, one adding it next to role == ChainstateRole::BACKGROUND and one with this change - explaining the context (which are a bit muffled for me currently) in each commit message?

I’m also curious if it’s possible to get back to being in IBD after we’re finished (e.g. after being offline for weeks, i.e. if (chain.Tip()->Time() < Now<NodeSeconds>() - m_options.max_tip_age) {).

l0rinc commented at 6:24 pm on June 29, 2025:

Opened it separately in #32827, please resolve this comment

in src/txorphanage.cpp:249 in e48bfeb58f outdated

245@@ -246,6 +246,9 @@ bool TxOrphanage::HaveTxToReconsider(NodeId peer)
246 
247 void TxOrphanage::EraseForBlock(const CBlock& block)
248 {
249+    // No orphans, nothing to do

l0rinc commented at 5:29 pm on June 11, 2025:

I find the comment redundant, it states exactly what the code already states clearly, doesn’t add any new info

in src/net_processing.cpp:2000 in e48bfeb58f outdated

1993@@ -1994,7 +1994,7 @@ void PeerManagerImpl::BlockConnected(
1994 
1995     // The following task can be skipped since we don't maintain a mempool for
1996     // the ibd/background chainstate.
1997-    if (role == ChainstateRole::BACKGROUND) {
1998+    if (is_ibd || role == ChainstateRole::BACKGROUND) {
1999         return;
2000     }
2001     LOCK(m_tx_download_mutex);

l0rinc commented at 5:35 pm on June 11, 2025:

We can safely skip this because we’re never requesting single transactions during IBD (i.e. AlreadyHaveTx isn’t needed during IBD)?

0for (const auto& ptx : pblock->vtx) {
1    RecentConfirmedTransactionsFilter().insert(ptx->GetHash().ToUint256());
2    if (ptx->HasWitness()) {
3        RecentConfirmedTransactionsFilter().insert(ptx->GetWitnessHash().ToUint256());
4    }
5    m_txrequest.ForgetTxHash(ptx->GetHash());
6    m_txrequest.ForgetTxHash(ptx->GetWitnessHash());
7}

Can we guard this by some other criteria instead of the IBD boolean, like we’re doing with EraseForBlock, e.g.

0if (!m_txrequest.Size()) return;

furszy commented at 7:30 pm on June 11, 2025:

We can safely skip this because we’re never requesting single transactions during IBD (i.e. AlreadyHaveTx isn’t needed during IBD)?
0for (const auto& ptx : pblock->vtx) {
1    RecentConfirmedTransactionsFilter().insert(ptx->GetHash().ToUint256());
2    if (ptx->HasWitness()) {
3        RecentConfirmedTransactionsFilter().insert(ptx->GetWitnessHash().ToUint256());
4    }
5    m_txrequest.ForgetTxHash(ptx->GetHash());
6    m_txrequest.ForgetTxHash(ptx->GetWitnessHash());
7}
Can we guard this by some other criteria instead of the IBD boolean, like we’re doing with EraseForBlock, e.g.
0if (!m_txrequest.Size()) return;

I thought about it but didn’t do it on purpose. The reasoning is that we still need to add the txs into the recently confirmed filters post-IBD even when we don’t have any tx requests. In other words, we still need to traverse the block. Could at most skip the last two lines with the emptiness check.

l0rinc commented at 7:13 am on June 18, 2025:

Could at most skip the last two lines with the emptiness check

Not sure that helps

in src/net_processing.cpp:1997 in e48bfeb58f outdated

1993@@ -1994,7 +1994,7 @@ void PeerManagerImpl::BlockConnected(
1994 
1995     // The following task can be skipped since we don't maintain a mempool for
1996     // the ibd/background chainstate.
1997-    if (role == ChainstateRole::BACKGROUND) {
1998+    if (is_ibd || role == ChainstateRole::BACKGROUND) {

l0rinc commented at 5:41 pm on June 11, 2025:

is the comment (already mentioning IBD) still accurate?

furszy commented at 7:25 pm on June 11, 2025:

is the comment (already mentioning IBD) still accurate?

The comment was accurate, but the code didn’t behave as described. Now it does.

in src/net_processing.cpp:1979 in e48bfeb58f outdated

1974@@ -1975,7 +1975,8 @@ void PeerManagerImpl::ActiveTipChange(const CBlockIndex& new_tip, bool is_ibd)
1975 void PeerManagerImpl::BlockConnected(
1976     ChainstateRole role,
1977     const std::shared_ptr<const CBlock>& pblock,
1978-    const CBlockIndex* pindex)
1979+    const CBlockIndex* pindex,
1980+    bool is_ibd)

l0rinc commented at 5:41 pm on June 11, 2025:

there should be a better way to do this than passing around a boolean over so many layers. I’ll review it properly this week.

furszy commented at 7:24 pm on June 11, 2025:

there should be a better way to do this than passing around a boolean over so many layers. I’ll review it properly this week.

Did you check the second approach proposed in the PR description?

l0rinc commented at 5:57 pm on June 11, 2025: contributor

Only had time for adding a few questions and quickly checking the expected effect this will have on IBD.

A flame graph I did this week for master, showing a -reindex until 900k blocks shows this part of the code to be 0.07% of IBD (~20 seconds). It can still make sense to do it, we should avoid doing unnecessary work, just setting the expectations straight.

Left a few notes, the guard makes sense to me, but the part I passionately disagree with is the boolean argument train - is there a simpler way to avoid that either via a different guard or the locally available m_initial_sync_finished (we’ve been locking before anyway).

in src/net_processing.cpp:1997 in e48bfeb58f outdated

1993@@ -1993,7 +1994,7 @@ void PeerManagerImpl::BlockConnected(
1994 
1995     // The following task can be skipped since we don't maintain a mempool for
1996     // the ibd/background chainstate.
1997-    if (role == ChainstateRole::BACKGROUND) {
1998+    if (is_ibd || role == ChainstateRole::BACKGROUND) {

TheCharlatan commented at 12:52 pm on June 12, 2025:

Shouldn’t checking role == ChainstateRole::BACKGROUND || m_chainman.IsInitialBlockDownload() be good enough? I realize it is racy, the condition may already be false even when the last few ibd blocks are still being processed, but I don’t think that really matters here. We also use it similarly in the BlockChecked method.

l0rinc commented at 1:44 pm on June 12, 2025:

That’s why I asked for !m_initial_sync_finished as well. If the race is biased towards reenabling it too early (rather than later), I agree that it should be fine.

TheCharlatan commented at 2:06 pm on June 12, 2025:

furszy reminded me that BlockChecked is run synchronously, so I don’t think that comparison makes sense after all. I guess this comment can be resolved.

furszy commented at 2:07 pm on June 12, 2025:

Shouldn’t checking role == ChainstateRole::BACKGROUND || m_chainman.IsInitialBlockDownload() be good enough? I realize it is racy, the condition may already be false even when the last few ibd blocks are still being processed, but I don’t think that really matters here. We also use it similarly in the BlockChecked method.

The diff between BlockConnected() and BlockChecked() is that the former one runs async while the latter runs synchronously. So the idea was to avoid locking cs_main in a different thread which delays other parts of the node.

I also thought about using the ’m_initial_sync_finished’ bool for this, but that’s not good because the field is calculated at a different time interval in the stale tip check thread (we could compute it more often but that’s a different story).

The last idea that had was the enabled flag inside the TxDownloadManager (check the PR description, you might like this one more).

l0rinc commented at 3:01 pm on June 12, 2025:

The last idea that had was the enabled flag inside the TxDownloadManager (check the PR description, you might like this one more).

I saw that but not yet sure until I see it in action. I definitely dislike the current arg passing, so I’d vote for that if we really need this param

furszy commented at 6:05 pm on June 12, 2025:

I saw that but not yet sure until I see it in action. I definitely dislike the current arg passing, so I’d vote for that if we really need this param

Ok, I’m a ~0 on this. I think this field is special enough (reason below) to be signaled as a separate arg or it could be wrapped into a struct — similar to how it’s done in the interfaces::Chain::Notifications class. With this, I’m not saying to not try the TxDownloadManager enabled flag approach, I just think that the current approach is slightly more useful for future listeners usages.

The only thing I’m somewhat convinced of is that, just like listeners are informed about blocks being downloaded through AssumeUTXO background chain validation, it would be nice if they are aware of IBD blocks — otherwise, sooner or later they will end up re-implementing the same racy/locking isIBD() logic across multiple layers (which could be fine for some cases but it doesn’t seem to be generally good).

l0rinc commented at 11:57 am on June 13, 2025:

it would be nice if they are aware of IBD blocks — otherwise […] they will end up re-implementing

Is there a more reusable way we could implement this?

TheCharlatan commented at 10:15 am on June 13, 2025: contributor

One thing that still irks me a bit is that we are communicating more ChainstateManager state through a Chainstate callback. I think ideally the chainstate should not have to know if it is in ibd or not, but that is a difficult thing to achieve with out current code. What does seem non-ideal is that re-indexing is treated the same is_ibd. Maybe the SynchronizationState from the headerTip notification would give us more flexibility in that regard if needed in the future?

p2p: avoid traversing blocks (twice) during IBD

Context:
Every time a block is connected, a 'BlockConnected' event is appended to the validation
interface queue. This queue is consumed sequentially by a single worker thread.
To avoid excessive memory usage, the queue is limited to 10 events at a time, and
we stop processing new blocks until the queued events are handled.

Issue:
Within the 'PeerManager::BlockConnected()' listener, we traverse the block twice
inside the transaction download manager — despite not needing to handle orphans or
transaction requests during IBD.
This redundant work adds unnecessary delay to the initial block download process.

8b9d39612d

furszy force-pushed on Jun 17, 2025

furszy commented at 7:41 pm on June 17, 2025: member

One thing that still irks me a bit is that we are communicating more ChainstateManager state through a Chainstate callback. I think ideally the chainstate should not have to know if it is in ibd or not, but that is a difficult thing to achieve with out current code.

Hmm yeah, we would need to decouple the block download logic from the chain state class first. This reminds me to #27837, which is a small step in that direction.

What does seem non-ideal is that re-indexing is treated the same is_ibd. Maybe the SynchronizationState from the headerTip notification would give us more flexibility in that regard if needed in the future?

Check it now. Ended up reimplementing it differently.

furszy renamed this:
~~p2p: avoid traversing blocks (twice) during IBD for no reason~~
p2p: avoid traversing blocks (twice) during IBD
on Jun 17, 2025

in src/net_processing.cpp:794 in 8b9d39612d

790@@ -790,7 +791,7 @@ class PeerManagerImpl final : public PeerManager
791 
792     /** Whether we've completed initial sync yet, for determining when to turn
793       * on extra block-relay-only peers. */
794-    bool m_initial_sync_finished GUARDED_BY(cs_main){false};
795+    std::atomic<bool> m_initial_sync_finished{false};

l0rinc commented at 6:16 am on June 18, 2025:

👍 for using an atomic here!

Note: the usages are always inverted now, can we rather have std::atomic<bool> m_initial_sync{true} instead?

in src/net_processing.cpp:5270 in 8b9d39612d

5267@@ -5267,7 +5268,6 @@ void PeerManagerImpl::CheckForStaleTipAndEvictPeers()
5268 
5269     if (!m_initial_sync_finished && CanDirectFetch()) {
5270         m_connman.StartExtraBlockRelayPeers();
5271-        m_initial_sync_finished = true;

l0rinc commented at 6:48 am on June 18, 2025:

does this introduce any behavioral change? (i.e. m_initial_sync_finished being set to true at a different lifecycle)

maflcko commented at 7:17 am on June 18, 2025:

m_initial_sync_finished will normally be set to true, so this is dead code now. Surprising that no tests failed …

in src/init.cpp:1931 in 8b9d39612d

1924@@ -1924,9 +1925,10 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
1925             tip_info->header_height = chainman.m_best_header->nHeight;
1926             tip_info->header_time = chainman.m_best_header->GetBlockTime();
1927         }
1928+        initial_sync_finished = !chainman.IsInitialBlockDownload();
1929     }
1930     LogPrintf("nBestHeight = %d\n", chain_active_height);
1931-    if (node.peerman) node.peerman->SetBestBlock(chain_active_height, std::chrono::seconds{best_block_time});
1932+    if (node.peerman) node.peerman->SetBestBlock(chain_active_height, std::chrono::seconds{best_block_time}, initial_sync_finished);

l0rinc commented at 7:08 am on June 18, 2025:

Q: is the guard only meant for tests or can this happen in prod as well? Given that we only need initial_sync_finished when this is set, was wondering if it would make sense to guard the initial_sync_finished = !chainman.IsInitialBlockDownload() as well - but not sure if that makes it clearer or more cumbersome. Just documenting my questions, no action needed.

in src/net_processing.h:127 in 8b9d39612d

123@@ -124,8 +124,8 @@ class PeerManager : public CValidationInterface, public NetEventsInterface
124     /** Send ping message to all peers */
125     virtual void SendPings() = 0;
126 
127-    /** Set the height of the best block and its time (seconds since epoch). */
128-    virtual void SetBestBlock(int height, std::chrono::seconds time) = 0;
129+    /** Set the height of the best block and its time (seconds since epoch), and indicate whether historical block sync has completed. */

l0rinc commented at 7:12 am on June 18, 2025:

indicate doesn’t hint that we’re actually setting it as well
historical might not be accurate given that we can re-enter IBD mode I think

in src/net_processing.cpp:540 in 8b9d39612d

536@@ -537,10 +537,11 @@ class PeerManagerImpl final : public PeerManager
537     PeerManagerInfo GetInfo() const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
538     void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
539     void RelayTransaction(const uint256& txid, const uint256& wtxid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
540-    void SetBestBlock(int height, std::chrono::seconds time) override
541+    void SetBestBlock(int height, std::chrono::seconds time, bool initial_sync_finished) override

l0rinc commented at 7:14 am on June 18, 2025:

Given that we’re always calling this with tip->GetBlockTime() and converting it to seconds on the call sites, we might simplify it to accept uint64_t block_time and convert to seconds inside instead. Not sure it’s better, just noticed a pattern…

maflcko commented at 7:21 am on June 18, 2025: member

Would be good to first add proper tests for the touched code in another pull, before refactoring the code for a 0.07% speedup. (The refactor broke the code, as I see it)

l0rinc approved

l0rinc commented at 7:39 am on June 18, 2025: contributor

I like this new patch a lot more, setting IBD is a lot cleaner this way. Somebody with more experience should validate whether that’s correct or not. My main suggestion is splitting to commits where the messages put the 3 distinct changes in context - and maybe to invert the naming of a the new field which is always negated at use site anyway.

  0diff --git a/src/init.cpp b/src/init.cpp
  1index 88fe531119..80b2fc7d6a 100644
  2--- a/src/init.cpp
  3+++ b/src/init.cpp
  4@@ -1909,7 +1909,7 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
  5     // ********************************************************* Step 12: start node
  6 
  7     int64_t best_block_time{};
  8-    bool initial_sync_finished{false};
  9+    bool initial_sync{true};
 10     {
 11         LOCK(chainman.GetMutex());
 12         const auto& tip{*Assert(chainman.ActiveTip())};
 13@@ -1925,10 +1925,10 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
 14             tip_info->header_height = chainman.m_best_header->nHeight;
 15             tip_info->header_time = chainman.m_best_header->GetBlockTime();
 16         }
 17-        initial_sync_finished = !chainman.IsInitialBlockDownload();
 18+        initial_sync = chainman.IsInitialBlockDownload();
 19     }
 20     LogPrintf("nBestHeight = %d\n", chain_active_height);
 21-    if (node.peerman) node.peerman->SetBestBlock(chain_active_height, std::chrono::seconds{best_block_time}, initial_sync_finished);
 22+    if (node.peerman) node.peerman->SetBestBlock(chain_active_height, best_block_time, initial_sync);
 23 
 24     // Map ports with NAT-PMP
 25     StartMapPort(args.GetBoolArg("-natpmp", DEFAULT_NATPMP));
 26diff --git a/src/net_processing.cpp b/src/net_processing.cpp
 27index 97e3e05d4f..89229d6cae 100644
 28--- a/src/net_processing.cpp
 29+++ b/src/net_processing.cpp
 30@@ -511,7 +511,7 @@ public:
 31         EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex);
 32     void BlockDisconnected(const std::shared_ptr<const CBlock> &block, const CBlockIndex* pindex) override
 33         EXCLUSIVE_LOCKS_REQUIRED(!m_tx_download_mutex);
 34-    void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload) override
 35+    void UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool initial_sync) override
 36         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
 37     void BlockChecked(const CBlock& block, const BlockValidationState& state) override
 38         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
 39@@ -537,11 +537,11 @@ public:
 40     PeerManagerInfo GetInfo() const override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
 41     void SendPings() override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
 42     void RelayTransaction(const uint256& txid, const uint256& wtxid) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
 43-    void SetBestBlock(int height, std::chrono::seconds time, bool initial_sync_finished) override
 44+    void SetBestBlock(int height, uint64_t block_time, bool initial_sync) override
 45     {
 46         m_best_height = height;
 47-        m_best_block_time = time;
 48-        m_initial_sync_finished = initial_sync_finished;
 49+        m_best_block_time = std::chrono::seconds{block_time};
 50+        m_initial_sync = initial_sync;
 51     };
 52     void UnitTestMisbehaving(NodeId peer_id) override EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex) { Misbehaving(*Assert(GetPeerRef(peer_id)), ""); };
 53     void ProcessMessage(CNode& pfrom, const std::string& msg_type, DataStream& vRecv,
 54@@ -789,9 +789,9 @@ private:
 55 
 56     bool RejectIncomingTxs(const CNode& peer) const;
 57 
 58-    /** Whether we've completed initial sync yet, for determining when to turn
 59+    /** Whether we're still in initial sync, for determining when to turn
 60       * on extra block-relay-only peers. */
 61-    std::atomic<bool> m_initial_sync_finished{false};
 62+    std::atomic<bool> m_initial_sync{true};
 63 
 64     /** Protects m_peer_map. This mutex must not be locked while holding a lock
 65      *  on any of the mutexes inside a Peer object. */
 66@@ -1994,7 +1994,7 @@ void PeerManagerImpl::BlockConnected(
 67 
 68     // The following task can be skipped since we don't maintain a mempool for
 69     // the ibd/background chainstate.
 70-    if (!m_initial_sync_finished || role == ChainstateRole::BACKGROUND) {
 71+    if (m_initial_sync || role == ChainstateRole::BACKGROUND) {
 72         return;
 73     }
 74     LOCK(m_tx_download_mutex);
 75@@ -2066,12 +2066,12 @@ void PeerManagerImpl::NewPoWValidBlock(const CBlockIndex *pindex, const std::sha
 76  * Update our best height and announce any block hashes which weren't previously
 77  * in m_chainman.ActiveChain() to our peers.
 78  */
 79-void PeerManagerImpl::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool fInitialDownload)
 80+void PeerManagerImpl::UpdatedBlockTip(const CBlockIndex *pindexNew, const CBlockIndex *pindexFork, bool initial_sync)
 81 {
 82-    SetBestBlock(pindexNew->nHeight, std::chrono::seconds{pindexNew->GetBlockTime()}, !fInitialDownload);
 83+    SetBestBlock(pindexNew->nHeight, pindexNew->GetBlockTime(), initial_sync);
 84 
 85     // Don't relay inventory during initial block download.
 86-    if (fInitialDownload) return;
 87+    if (initial_sync) return;
 88 
 89     // Find the hashes of all blocks that weren't previously in the best chain.
 90     std::vector<uint256> vHashes;
 91@@ -5266,8 +5266,9 @@ void PeerManagerImpl::CheckForStaleTipAndEvictPeers()
 92         m_stale_tip_check_time = now + STALE_CHECK_INTERVAL;
 93     }
 94 
 95-    if (!m_initial_sync_finished && CanDirectFetch()) {
 96+    if (m_initial_sync && CanDirectFetch()) {
 97         m_connman.StartExtraBlockRelayPeers();
 98+        // TODO? m_initial_sync = false;
 99     }
100 }
101 
102diff --git a/src/net_processing.h b/src/net_processing.h
103index 5a5980a9da..d7ea6a12a3 100644
104--- a/src/net_processing.h
105+++ b/src/net_processing.h
106@@ -124,8 +124,8 @@ public:
107     /** Send ping message to all peers */
108     virtual void SendPings() = 0;
109 
110-    /** Set the height of the best block and its time (seconds since epoch), and indicate whether historical block sync has completed. */
111-    virtual void SetBestBlock(int height, std::chrono::seconds time, bool initial_sync_finished) = 0;
112+    /** Set the height of the best block, its block time, and whether block sync has completed. */
113+    virtual void SetBestBlock(int height, uint64_t block_time, bool initial_sync) = 0;
114 
115     /* Public for unit testing. */
116     virtual void UnitTestMisbehaving(NodeId peer_id) = 0;
117diff --git a/src/test/peerman_tests.cpp b/src/test/peerman_tests.cpp
118index 30d680ffde..0bedae7859 100644
119--- a/src/test/peerman_tests.cpp
120+++ b/src/test/peerman_tests.cpp
121@@ -41,8 +41,7 @@ BOOST_AUTO_TEST_CASE(connections_desirable_service_flags)
122     // Make peerman aware of the initial best block and verify we accept limited peers when we start close to the tip time.
123     auto tip = WITH_LOCK(::cs_main, return m_node.chainman->ActiveChain().Tip());
124     uint64_t tip_block_time = tip->GetBlockTime();
125-    int tip_block_height = tip->nHeight;
126-    peerman->SetBestBlock(tip_block_height, std::chrono::seconds{tip_block_time}, /*initial_sync_finished=*/true);
127+    peerman->SetBestBlock(tip->nHeight, tip_block_time, /*initial_sync=*/false);
128 
129     SetMockTime(tip_block_time + 1); // Set node time to tip time
130     BOOST_CHECK(peerman->GetDesirableServiceFlags(peer_flags) == ServiceFlags(NODE_NETWORK_LIMITED | NODE_WITNESS));
131diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp
132index ca47bf949f..80d25df047 100644
133--- a/src/txorphanage.cpp
134+++ b/src/txorphanage.cpp
135@@ -246,7 +246,6 @@ bool TxOrphanage::HaveTxToReconsider(NodeId peer)
136 
137 void TxOrphanage::EraseForBlock(const CBlock& block)
138 {
139-    // No orphans, nothing to do
140     if (m_orphans.empty()) return;
141 
142     std::vector<Wtxid> vOrphanErase;

furszy commented at 12:53 pm on June 18, 2025: member

m_initial_sync_finished will normally be set to true, so this is dead code now. Surprising that no tests failed …

yeah.. This reminded me why I didn’t use the m_initial_sync_finished flag in the previous version.. it is being set differently because it can toggle back and forth, while the IBD one doesn’t. We had a whole discussion about this in #28170. Before getting into any detail, will think more about the approach (not this week) and whether it’s worth embarking on a larger journey just for this change.

DrahtBot commented at 7:11 pm on July 11, 2025: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot added the label Needs rebase on Jul 11, 2025

p2p: avoid traversing blocks (twice) during IBD #32730

Code Coverage & Benchmarks

Reviews

Conflicts