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

pull furszy wants to merge 1 commits into bitcoin:master from furszy:2025_net_avoid_traversing_block_twice changing 5 files +14 −9
  1. 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.

  2. 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:

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

  3. DrahtBot added the label P2P on Jun 11, 2025
  4. 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
  5. 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
  6. 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

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

  8. 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?

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

    image

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

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

  11. 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?
  12. 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
  13. furszy force-pushed on Jun 17, 2025
  14. 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.

  15. furszy renamed this:
    p2p: avoid traversing blocks (twice) during IBD for no reason
    p2p: avoid traversing blocks (twice) during IBD
    on Jun 17, 2025
  16. 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?

  17. 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 …
  18. 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.
  19. 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
  20. 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…
  21. 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)
  22. l0rinc approved
  23. 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;
    
  24. 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.


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-07-06 03:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me