p2p: Advance pindexLastCommonBlock early after connecting blocks #32180

1. 
   mzumsande commented at 10:40 pm on March 31, 2025: contributor

   As described in #32179, pindexLastCommonBlock is updated later than necessary in master. In case of a linear chain with no forks, it can be moved forward at the beginning of FindNextBlocksToDownload, so that the updated value can be used to better estimate nWindowEnd. This helps the node to request all blocks from peers within the correct 1024-block-window and avoids peers being incorrectly marked as stallers.

   The second commit removes a special case introduced in 49d569cb1fdd62a9da8dff51dccaf4680fe3d0eb which is not necessary, because the more general logic also covers this case.

   I also changed p2p_ibd_stalling.py to cover the situation after a resolved situation, making sure that no additional peers are marked for stalling.

   Fixes #32179

2. 
   DrahtBot commented at 10:40 pm on March 31, 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/32180.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	stringintech, i-am-yuvi

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

3. mzumsande marked this as a draft on Mar 31, 2025
4. DrahtBot added the label P2P on Mar 31, 2025
5. mzumsande marked this as ready for review on Apr 2, 2025
6. mzumsande force-pushed on Apr 7, 2025
7. in src/net_processing.cpp:1376 in 205e651713 outdated

   1380+        state->pindexLastCommonBlock = m_chainman.ActiveTip();
   1381+    }
   1382+
   1383     // If the peer reorganized, our previous pindexLastCommonBlock may not be an ancestor
   1384     // of its current tip anymore. Go back enough to fix that.
   1385     state->pindexLastCommonBlock = LastCommonAncestor(state->pindexLastCommonBlock, state->pindexBestKnownBlock);
   

   ---

   ---

   

   stringintech commented at 9:59 am on April 10, 2025:

   Could there be an issue if both nodes reorganize to the same chain, but the last common block is still on another fork?

   If our new tip height is less than the outdated pindexLastCommonBlock height (like when reorging to a shorter chain with more work), we’d skip the optimization when we shouldn’t.

   Since LastCommonAncestor() brings back the pindexLastCommonBlock on the peer’s best chain, maybe the new code should come after this call?

   Not sure if this edge case matters in practice, but thought I’d mention it.​​​​​​​​​​​​​​​​

   ---

   

   mzumsande commented at 7:45 pm on April 11, 2025:

   Good observation! I also think that this edge case shouldn’t matter much. It’s absolutely essential that pindexLastCommonBlock is an ancestor of pindexBestKnownBlock when calling FindNextBlocks, which is why I originally left that check last. But the added check can’t change that since it only sets pindexLastCommonBlock to the tip if that tip is an ancestor of pindexBestKnownBlock. So I’ve changed the order.
8. 
   stringintech commented at 10:02 am on April 10, 2025: none

   Concept ACK.
9. 
   i-am-yuvi commented at 7:47 am on April 11, 2025: contributor

   Concept ACK

   Thanks for this PR.

   I have made some notes regarding this PR and will add some guide for testing as well very soon.

10. p2p: During block download, adjust pindexLastCommonBlock right away

    This allows us to calculate nWndowEnd better.
    Before, it would be calculated based on an old pindexLastCommonBlock,
    which could result in blocks not requested for download that could have been
    requested, and peers being wrongly marked as staller.

    1d8504c5e6
11. p2p: remove extra logic for assumeutxo

    This reverts commit 49d569cb1fdd62a9da8dff51dccaf4680fe3d0eb
    which introduced extra logic for when a snapshot was loaded.
    With the previous commit, this is not longer necessary
    because the more general logic of advancing pindexLastCommonBlock
    also covers this case.

    f5e6a8475b
12. test: remove magic number when checking for blocks that have arrived

    getpeerinfo provides a list of blocks that are inflight, which can be used
    instead.

    f419017aae
13. test: improve coverage for a resolved stalling situation

    This test (which would fail without the previous commit) checks
    that after the stalling block was received, we don't incorrectly
    mark another peer as a staller immediately.

    c69ee2d5b9
14. mzumsande force-pushed on Apr 11, 2025

Contributors
mzumsande DrahtBot stringintech i-am-yuvi

Labels
P2P