net processing: Remove nStartingHeight check from block relay #20624

1. 
   jnewbery commented at 10:49 am on December 11, 2020: member

   nStartingHeight was introduced in commit 7a47324c7 (Bitcoin version 0.2.9, P2P version 209) with the comment “better prevention of inventory relaying during initial download”. At that time, there was no function to determine whether the node was still in Initial Block Download, so to prevent syncing nodes from relaying old blocks to their peers, a check was added to never relay a block to a peer where the height was lower than 2000 less than the peer’s best block. That check was updated several times in later commits to ensure that we weren’t relaying blocks before the latest checkpoint if the peer didn’t provide a startingheight. The checkpoint comparison was changed to compare with an estimate of the highest block in commit eae82d8e.

   In commit 202e0194, all block relay was gated on being out of Initial Block Download. In commit 0278fb5f, the comparison to nBlockEstimate was removed since “we already checked IsIBD()”.

   We can remove the check against nStartingHeight entirely. If the node is out of Initial Block Download, then its tip height must have been within 24 hours of current time, so should not be more than ~144 blocks behind the most work tip.

   This simplifies moving block inventory state into the Peer object (#19829).

2. [net processing] Remove nStartingHeight check from block relay

   nStartingHeight was introduced in commit 7a47324c7 (Bitcoin version
   0.2.9, P2P version 209) with the comment "better prevention of inventory
   relaying during initial download". At that time, there was no function
   to determine whether the node was still in Initial Block Download, so to
   prevent syncing nodes from relaying old blocks to their peers, a check
   was added to never relay a block to a peer where the height was lower
   than 2000 less than the peer's best block. That check was updated
   several times in later commits to ensure that we weren't relaying blocks
   before the latest checkpoint if the peer didn't provide a
   startingheight. The checkpoint comparison was changed to compare with an
   estimate of the highest block in commit eae82d8e.
   
   In commit 202e0194, all block relay was gated on being out of Initial
   Block Download. In commit 0278fb5f, the comparison to nBlockEstimate was
   removed since "we already checked IsIBD()".
   
   We can remove the check against nStartingHeight entirely. If the node is
   out of Initial Block Download, then its tip height must have been within
   24 hours of current time, so should not be more than ~144 blocks behind
   the most work tip.

   8b57013473
3. [net processing] Remove unnecesary nNewHeight variable in UpdatedBlockTip() 94d2cc35be
4. [net processing] Clarify UpdatedBlockTip() f6360088de
5. fanquake added the label P2P on Dec 11, 2020
6. 
   jonatack commented at 11:44 am on December 11, 2020: member

   ACK f6360088de8ca02f9d198da2f8cca4ea8d64c992

   Thanks for the interesting history and background exposition.

   Recommend reviewers view the first and last commit diffs with -w

7. 
   jonatack commented at 12:33 pm on December 11, 2020: member

   IRC discussion about this PR: http://www.erisian.com.au/bitcoin-core-dev/log-2020-12-11.html#l-196
8. MarcoFalke approved
9. 
   MarcoFalke commented at 12:37 pm on December 11, 2020: member

   ACK f6360088de8ca02f9d198da2f8cca4ea8d64c992 💽

   Signature:

    0-----BEGIN PGP SIGNED MESSAGE-----
    1Hash: SHA512
    2
    3ACK f6360088de8ca02f9d198da2f8cca4ea8d64c992 💽
    4-----BEGIN PGP SIGNATURE-----
    5
    6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
    7pUjGOQwAl/Dkeosfmj7zyR4q/leLDs5EhKeyxL2Pb8ANP3g0ZILSRwzK640vdUQw
    8yrwd/7MByN4Jb9k5ED0Y0PLFZXlj8H1NADecA4Bm/p/4ZJCc9TdvEc+8vxBzHYty
    9ssXSj+qAByz+JoUvg+gnKLFV42d8/IKO+CFKOqxO1wkkzB/WxepPaelODeqvpBn9
   10MY7jqAPXIFtrUDojRXq3dmotglWjEtxAO18EPPVlE+eQOvcUS7YxZti1irrMXvGz
   11ycC3AsAaH1iZTz0e+S7d6k/lVfx5x20+RYUOwwoPzuGEey3P7+88S7Enumq2rOff
   12GtEtxzeZsJ55cEaThH/l4GB9Q9gZwBAohnYtw5IbWuYneSreyXuURhcx72Sf77Ty
   13qQaYa/rnlV6hI65x9HznwkYYBj8kjbtQiE0xT/5+STc1d4//MUErnyin8BFPG/Ov
   14JK3Gm9xpfrhy0N548Jgh2bZiwqakV9kE5PVxznRuWLP/8KDi41QJX9BNH1y/EKce
   156pjc9ZkZ
   16=TGqQ
   17-----END PGP SIGNATURE-----
   

   Timestamp of file with hash 4fb6bbf8d5f3292f6589ad436dea99ec4778621103ec520fd4512518e020eb5e -

10. 
    Sjors commented at 3:40 pm on December 11, 2020: member

    utACK f636008

    IIUC: we were already checking fInitialDownload == false and this PR drops an additional check pindexNew->nHeight > pnode->nStartingHeight - 2000. One of the criteria for initial block download is m_chain.Tip()->GetBlockTime() < (GetTime() - nMaxTipAge, i.e. the new block is less than 24 hours old. The most likely reason for a new node to be more than 2000 blocks ahead of us, is if we are in IBD, so in that case the check was indeed redundant.

    Another reason for a node to be 2000 bocks ahead of us is if they know a longer chain. In that case they won’t care about our header announcements (which we now start sending it), but won’t punish us either afaik.

11. in src/net_processing.cpp:1300 in f6360088de

    1308-                // Limit announcements in case of a huge reorganization.
    1309-                // Rely on the peer's synchronization mechanism in that case.
    1310-                break;
    1311-            }
    1312+    // Don't relay inventory during initial block download.
    1313+    if (fInitialDownload) return;
    

    ---

    ---

    

    ariard commented at 4:39 pm on December 11, 2020:

    Note, if we do have a failing GetTime() and the node is falsely out-of-IBD, it will start to advertise its new best chain in case of reorg. Before this PR, even assuming a IBD bug, chain advertisement won’t happen if peers starting heights are far ahead by 2000 blocks from us IIUC ?

    I think that’s okay just to be aware of.

    ---

    

    jnewbery commented at 5:00 pm on December 11, 2020:

    yes, that sounds right. And even if we are announcing a less-good chain to a peer, I don’t think that’s a big problem.
12. 
    ariard commented at 4:41 pm on December 11, 2020: member

    Code Review ACK f636008
13. 
    decryp2kanon commented at 8:43 pm on December 11, 2020: contributor

    Concept ACK
14. MarcoFalke merged this on Dec 14, 2020
15. MarcoFalke closed this on Dec 14, 2020

    ---

16. jnewbery deleted the branch on Dec 14, 2020
17. sidhujag referenced this in commit 242e112abe on Dec 14, 2020
18. 
    rebroad commented at 12:12 pm on May 11, 2021: contributor

    Block announcement seems quirky currently. For example, if a node hasn’t requested headers announcements, then we’ll not announce blocks invs to nodes that have indicated they have the block, but if they have requested headers announcements, then we’ll announce headers to them whether they have the block or not! And also, if we have a lot of headers to announce, then we’ll revert to announcing via block invs!

    The behaviour seems to change over the years without any obvious discussion on github as to why. I think ideally we ought not to be sending headers of a block to nodes that have already sent us the same header - seems kinda pointless to me - if the intention is to let them know what our latest block is, then sending them a block inv would use less bandwidth.

19. Fabcien referenced this in commit 929af2da84 on Jan 24, 2022
20. DrahtBot locked this on Aug 18, 2022

Contributors
jnewbery jonatack MarcoFalke Sjors ariard decryp2kanon rebroad

Labels
P2P