P2P: request blocks from new peer, if "block" not seen in 1 hour #1835

pull jgarzik wants to merge 2 commits into bitcoin:master from jgarzik:kickblocks2 changing 1 files +10 −1

jgarzik commented at 2:37 AM on September 19, 2012: contributor

If we appear stuck and a new peer has blocks for us, ask for them.

NOTE: This triggers during the "version" message, and so, does not help cases where there is no connection turn-over.

However, this seems a very simple way to address stuck network sync under reverse-header-sync or something more fancy appears.

Builds on top of bug fix submitted in #1834.

P2P: Do not request blocks from peers with fewer blocks than us

If the remote node has a shorter chain, do not waste our
special getblocks request on them.

d5d19c74a5

gmaxwell commented at 5:03 AM on September 19, 2012: contributor

I don't think I get the point of this. Currently, we'll start fetching blocks as soon as one happens on the network. >1 hr blocks are fairly rare.. once per six days on average. So it would be quite infrequently useful.

How about instead— if we connect to someone, and it claims to have more blocks than we have, and we haven't gotten a block in X time (e.g. we're not currently getting them from another peer) then trigger it... and then we go add node rotation. This would work around our silly stuckness reasons, and would also help with stuckness due to idiot peers.
P2P: send 'getblocks' to new peers, if the chain has not advanced in a while b23e79938b
jgarzik commented at 5:20 PM on September 19, 2012: contributor

It is a fair criticism that this change did not accurately capture the 'stuck' condition. The "stuck" condition is defined as: new blocks arrive, but our hashBestChain does not change.

Updated commit to record when the chain last advanced, and use that timestamp to trigger a "getblocks" for newly connected peers.
BitcoinPullTester commented at 6:28 AM on September 20, 2012: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/b23e79938b6aa5bd22389f393abf2d6905df770a for binaries and test log.

in src/main.cpp:None in b23e79938b

2893 | @@ -2890,8 +2894,13 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv)
2894 |          CInv inv(MSG_BLOCK, block.GetHash());
2895 |          pfrom->AddInventoryKnown(inv);
2896 |  
2897 | +        uint256 hashTmpBest = hashBestChain;
2898 |          if (ProcessBlock(pfrom, &block))
2899 |              mapAlreadyAskedFor.erase(inv);
2900 | +
2901 | +        if (hashTmpBest != hashBestChain)
2902 | +            nTimeChainChanged = GetTime();

luke-jr commented at 9:40 AM on September 20, 2012:

Any reason not to use nTimeBestReceived here?

sipa commented at 12:35 PM on September 21, 2012: member

Have you actually observed this helps? If anything, I'd do the re-requests for blocks outside of the "version" message handler, as frequently the connections are quite stable when this happens.
jgarzik commented at 5:18 PM on September 24, 2012: contributor

Closing, hopefully can look at implementing @sipa's suggestion
jgarzik closed this on Sep 24, 2012
jgarzik deleted the branch on Aug 24, 2014
KolbyML referenced this in commit cbd9271afb on Dec 5, 2020
DrahtBot locked this on Sep 8, 2021

Contributors

Linked (view graph)

#1834 P2P: Do not request blocks from peers with fewer blocks than us