net processing: Remove nStartingHeight check from block relay #20624
Conversation
nStartingHeight was introduced in commit 7a47324 (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 eae82d8. In commit 202e0194, all block relay was gated on being out of Initial Block Download. In commit 0278fb5, 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.
|
IRC discussion about this PR: http://www.erisian.com.au/bitcoin-core-dev/log-2020-12-11.html#l-196 |
There was a problem hiding this comment.
ACK f636008 💽
Show signature and timestamp
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
ACK f6360088de8ca02f9d198da2f8cca4ea8d64c992 💽
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjGOQwAl/Dkeosfmj7zyR4q/leLDs5EhKeyxL2Pb8ANP3g0ZILSRwzK640vdUQw
yrwd/7MByN4Jb9k5ED0Y0PLFZXlj8H1NADecA4Bm/p/4ZJCc9TdvEc+8vxBzHYty
ssXSj+qAByz+JoUvg+gnKLFV42d8/IKO+CFKOqxO1wkkzB/WxepPaelODeqvpBn9
MY7jqAPXIFtrUDojRXq3dmotglWjEtxAO18EPPVlE+eQOvcUS7YxZti1irrMXvGz
ycC3AsAaH1iZTz0e+S7d6k/lVfx5x20+RYUOwwoPzuGEey3P7+88S7Enumq2rOff
GtEtxzeZsJ55cEaThH/l4GB9Q9gZwBAohnYtw5IbWuYneSreyXuURhcx72Sf77Ty
qQaYa/rnlV6hI65x9HznwkYYBj8kjbtQiE0xT/5+STc1d4//MUErnyin8BFPG/Ov
JK3Gm9xpfrhy0N548Jgh2bZiwqakV9kE5PVxznRuWLP/8KDi41QJX9BNH1y/EKce
6pjc9ZkZ
=TGqQ
-----END PGP SIGNATURE-----
Timestamp of file with hash 4fb6bbf8d5f3292f6589ad436dea99ec4778621103ec520fd4512518e020eb5e -
|
utACK f636008 IIUC: we were already checking 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. |
| break; | ||
| } | ||
| // Don't relay inventory during initial block download. | ||
| if (fInitialDownload) return; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Concept ACK |
|
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. |
Summary: ``` nStartingHeight was introduced in commit 7a47324 (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 eae82d8. In commit 202e019, all block relay was gated on being out of Initial Block Download. In commit 0278fb5, 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. ``` Backport of [[bitcoin/bitcoin#20624 | core#20624]]. Depends on D10862. Ref T1696. Test Plan: ninja all check-all Run IBD Reviewers: #bitcoin_abc, PiRK Reviewed By: #bitcoin_abc, PiRK Maniphest Tasks: T1696 Differential Revision: https://reviews.bitcoinabc.org/D10863
nStartingHeight was introduced in commit 7a47324 (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 eae82d8.
In commit 202e0194, all block relay was gated on being out of Initial
Block Download. In commit 0278fb5, 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
Peerobject (#19829).