Parallel block download

rebroad commented at 2:02 am on May 17, 2012: contributor

Builds upon pull #1271, which fixes Issue #1234.

This allows multiple peers to provide blocks, not just one. Up to 8 peers can provide blocks in parallel, thereby increasing block download speed by up to 8.

Submitted for comments. I think ideally I’ll add some more code to cater for stuck downloads, so it can give up and move onto another peer. Currently, it’ll only move onto another peer if the selected peer disconnects. It a peer took a long time to disconnect, then all blocks downloaded would be orphans until the missing block was eventually downloaded.

Also, the “8” concurrent downloads is hard-coded, would people prefer this to be a configurable command-line option?

Fixes Issue #1234 - re-issues getblocks when current node providing them disappears.

Conflicts:

	.gitignore
	src/main.cpp
	src/net.cpp
	src/net.h

Conflicts:

	src/main.cpp
	src/net.h

2a0a591d75

Allow blocks to be downloaded from multiple nodes. First commit.

TODO: Deal with stuck downloads so the same block can be downloaded by another node. Perhaps keep count of the re-asks. Log these.

Conflicts:

	src/main.cpp
	src/net.h

95ca3aa54a

Not needed when caught up... ea3271b7dd

in .gitignore: in cf79c8a9e8 outdated

19@@ -20,3 +20,4 @@ qrc_*.cpp
20 *.pro.user
21 #mac specific
22 .DS_Store
23+build/

Diapolo commented at 3:25 pm on May 17, 2012:

That should not be here I guess.

rebroad commented at 3:28 pm on May 17, 2012:

If i don’t add this line, git complains. How do you manage without git wanting this directory added or ignored?

Diapolo commented at 3:38 pm on May 17, 2012:

Sorry, I’m no GIT master :-/. But I never had a pull or commit, where I needed to include .gitignore.

rebroad commented at 3:40 pm on May 17, 2012:

Strangely, i can’t add .gitignore to .gitignore. It ignores that line!

in src/main.cpp: in cf79c8a9e8 outdated

926@@ -925,6 +927,12 @@ int GetNumBlocksOfPeers()
927     return std::max(cPeerBlockCounts.median(), Checkpoints::GetTotalBlocksEstimate());
928 }
929 
930+bool CaughtUp()

Diapolo commented at 3:29 pm on May 17, 2012:

You could use just: return (nBestHeight >= GetNumBlocksOfPeers()); or even just use (nBestHeight >= GetNumBlocksOfPeers()) directly, as you currently use CaughtUp() only once.

rebroad commented at 3:31 pm on May 17, 2012:

True, this is done because CaughtUp is used extensively in other branches/pull requests and doing this at this stage i think helps things to merge easier.

Diapolo commented at 3:37 pm on May 17, 2012:

OK, but you could at least remove the variable I guess.

rebroad commented at 4:30 pm on May 17, 2012:

ah. I see what you mean now. fixed. thanks.

in src/main.cpp: in cf79c8a9e8 outdated

2405+        pfrom->PushGetBlocks(pindexBest, uint256(0));
2406+        NodeSummary();
2407+    }
2408+
2409+
2410+    if (strCommand == "version") ;

Diapolo commented at 3:36 pm on May 17, 2012:

What use has this line?

rebroad commented at 3:39 pm on May 17, 2012:

It’s needed since the AskForBlocks code is now outside the code for the version message. It runs immediatelly after it, so for all the elses to work this line is needed, otherwise the unknown command section is executed.

Diapolo commented at 3:41 pm on May 17, 2012:

But in the end this pull is not mergable like it is?

rebroad commented at 3:42 pm on May 17, 2012:

It’r rebased to upstream master, so it should be. Why do you think it isn’t?

rebroad commented at 3:44 pm on May 17, 2012:

I’d say it’s not ready to pull until i add the code to check for stuck block downloads. I wasn’t sure if a pull request was the correct way to go about soliciting comments or if there was a better way. Stuck code has been written and tested for about a month but not combined with this code yet.

Diapolo commented at 3:47 pm on May 17, 2012:

I missread this file and had the impression the code for == “version” was moved or removed … I was wrong, sorry :).

Diapolo commented at 3:48 pm on May 17, 2012:

May I suggest when answering from a mobile don’t quote the inital message :D. This makes things unreadable here.

rebroad commented at 4:20 pm on May 17, 2012:

ah… I didn’t realise it was doing that! Thanks for pointing that out :)

gmaxwell commented at 11:10 pm on May 17, 2012: contributor

I haven’t had a chance to review this yet— though I see you are using GetNumBlocksOfPeers()…

It’s perfectly possible for nodes to send insane values for the heights and some have done so. It should be treated as untrustworthy. This function is currently only used in the GUI… and I think it should probably stay there. At the very least every caller needs to give consideration to the possibility that a majority of your peers may be malicious and may give values chosen to make your day maximally bad.

rebroad closed this on May 18, 2012

rebroad commented at 8:18 am on May 18, 2012: contributor

I’ve closed this pull for now, as it was opened for comments, but it’s not ready to pull. It still needs: 1) stuck management, and 2a) either a separate AskFor function for txs and blocks, as as it’s currently written, it’ll getdata the same tx from many nodes at the same time, or 2b) the same treatment of txs, i.e. don’t download the same tx from another peer.

2b isn’t so ideal as it will produce more getdatas as with the way it’s written, it’ll only have 1 inv per every getdata. This is ok for blocks (IMHO), but perhaps not so suitable for txs, so I’d be inclined to go with 2a rather than 2b.

Feedback much appreciated. @gmaxwell, re your last comment, no worries, it’s not finished anyway, so perhaps it’s better to wait until I’ve completed bits 1 and 2a first - although your comments on whether 2a or 2b or 3c (something else) are suitable would be welcome.

Re CaughtUp() not being reliable. Well, true, but if one is connected to misbehaving peers such as this, then one’d (!) probably not want to be downloading from them anyway. The reason I created CaughtUp() is so that it can be refined to be more reliable. It serves a different purpose to IsInitialDownload() from what I’ve been told by others that that function is for. I do think CaughtUp(), based on your comments above, does need refining given the risk you mention. Perhaps it can factor in the timestamp of the last block received also as a sanity check.

rebroad commented at 1:41 pm on May 29, 2012: contributor

https://github.com/rebroad/bitcoin/commit/f11b01d6c3f0c8eeeca06ba210af08a51887acbc is an update of this pull request, and for some reason has “detached” from this pull request, so I’ll have to raise another for this commit once I’ve done enough testing….

suprnurd referenced this in commit 8b03216353 on Dec 5, 2017

lateminer referenced this in commit 6bbd575860 on May 6, 2020

DrahtBot locked this on Sep 8, 2021

Parallel block download #1326