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

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

NAK. This potentially breaks the bitcoin distributed algorithm.

(Ignore checkpoints for a moment.) I mine a long fork with 300,000 diff1 blocks. I feed it to you. You'll now never fetch the much longer real chain from honest nodes. Same problem exists later in the chain too, it's just not trivial to pull off as an attack in that case. May also have some weird effects with forks around the retarget (you can have short competing forks with different difficulties, and because the maximum retarget change is >=2x a lower diff chain could have more blocks, even for a fork only a few blocks long)

That case is nigh impossible given checkpoints. You ask to ignore this, to present a theoretical case that does not therefore apply to real world client releases.

One case obviously can lead to stuck downloads today. The client sends getblocks to a remote peer that we know in advance does not have the data we need. nAskedForBlocks is incremented, preventing further queries, directly and immediately leading to a stuck download.

It does seem a bit dangerous to rely on pfrom->nStartingHeight so completely. It's very hard to see what effect this will have in edge cases.

Couldn't we address the "stuck downloads" problem in another place, ie by detecting stuck downloads actively and requesting from different nodes. After all, even if a node claims to have more blocks, it can still be stuck.

@jgarzik the download happily continues when we get another block from the network in an average of 10 minutes.

I presented the toy example ignoring checkpoints because its easier to understand. The same exposure exists at retargets where a forks can be created that differ in difficulty by a factor of two, or if a non-majority but high hashpower (e.g. asics before they are widely adopted) attacker used timewarp to mine a fork down to low difficulty starting anywhere after the highest checkpoint.

I don't think the exposure, even if it is fringe and unexciting is worth avoiding waiting for a block in the rare case that you get a peer which is stuck and behind the chain.

Gah, sorry, I read the title without actually reading the patch. My above concerns aren't an issue here: even in those cases when the real network finds a block again it'll still pull from them, which is adequate and fine.

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

Something like this is useful, but it will fail to fetch the new chain in some edge cases (I know, those will be fetched when a successor block is broadcast, but still). Maybe some flexibility, like (nStartingHeight > nBestHeight - 144) is more safe, and will still prevent against the majority of silly getblocks?

Updated commit to offset height count by 144.

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

ACK with the 144 offset

ACK