This repository has been archived by the owner. It is now read-only.

/Sia

Sync bug fixes #1002

Merged
merged 6 commits into from Mar 3, 2016

Conversation

Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@VoidingWarranties @lukechampine @DavidVorick
@VoidingWarranties
Collaborator

VoidingWarranties commented Mar 1, 2016

This PR fixes 2 major and 1 minor bugs.

Major bugs:

  • SendBlocks was sending the entire blockchain every time (tested in TestRPCSendBlockSendsOnlyNecessaryBlocks)
  • SendBlocks would sometimes fail to send the last block (tested in TestRPCSendBlocks)

Minor bug:

  • blockHistory included the genesis block twice sometimes (when height == step). This doesn't cause any problems, but no reason to not fix it. Even with this fix the genesis block is still included twice (once at blockHistory[0] and once at blockHistory[31]) if the only block is the genesis block, which should only happen during initial blockchain download.

This should dramatically improve syncing performance.
@lukechampine

This comment has been minimized.

Show comment
Hide comment
@lukechampine

lukechampine Mar 1, 2016

Member

neat. I have no comments as the changes are all pretty simple. David may have concerns about the tests though.
This might reduce load on the explorer, so we'll want to update that binary as soon as this is merged.
Member

lukechampine commented Mar 1, 2016

neat. I have no comments as the changes are all pretty simple. David may have concerns about the tests though.
This might reduce load on the explorer, so we'll want to update that binary as soon as this is merged.
@VoidingWarranties

This comment has been minimized.

Show comment
Hide comment
@VoidingWarranties

VoidingWarranties Mar 1, 2016

Collaborator

Unfortunately the explorer won't benefit until its peers are also using this version of SendBlocks.
The explorer will won't fully benefit until its peers are also using this version of SendBlocks. It should hopefully reduce the load at least a little.
Collaborator

VoidingWarranties commented Mar 1, 2016

Unfortunately the explorer won't benefit until its peers are also using this version of SendBlocks.
The explorer will won't fully benefit until its peers are also using this version of SendBlocks. It should hopefully reduce the load at least a little.
modules/consensus/synchronize.go
@@ -176,6 +176,7 @@ func (cs *ConsensusSet) sendBlocks(conn modules.PeerConn) error {
found = true
// Start from the child of the common block.
start = pb.Height + 1
break

This comment has been minimized.

Show comment
Hide comment
@DavidVorick

DavidVorick Mar 1, 2016

Member

T.T
Copy link
@DavidVorick

DavidVorick Mar 1, 2016

Member

T.T

This comment has been minimized.

Show comment
Hide comment
@DavidVorick

DavidVorick Mar 1, 2016

Member

how many gigabytes do you think this bug has eaten? I'm going to guess 25,000.
Copy link
@DavidVorick

DavidVorick Mar 1, 2016

Member

how many gigabytes do you think this bug has eaten? I'm going to guess 25,000.

This comment has been minimized.

Show comment
Hide comment
@VoidingWarranties

VoidingWarranties Mar 1, 2016

Collaborator

Let's assume an average block size of 100kB, with an average network size of 100 peers. There are ~40,000 blocks. Let's also assume constant block size and number of peers.

100kB * 100 peers * sum of integers from 0 to 40,000 ~= 8PB

Wow. Did I get my math wrong?
Copy link
@VoidingWarranties

VoidingWarranties Mar 1, 2016

Collaborator

Let's assume an average block size of 100kB, with an average network size of 100 peers. There are ~40,000 blocks. Let's also assume constant block size and number of peers.

100kB * 100 peers * sum of integers from 0 to 40,000 ~= 8PB

Wow. Did I get my math wrong?

This comment has been minimized.

Show comment
Hide comment
@DavidVorick

DavidVorick Mar 2, 2016

Member

yes you did :)

Average peer has about 10 friends during IBD. Each peer does IBD probably, once, though total we've probably done 10,000 IBDs. Assuming that they're downloading 35,000 blocks on average, and they catch up from block 0, and I know that blocks don't average 100kb, probably closer to 10kb (especially the early blocks are mostly empty). I think the size of the whole blockchain is around 500MB.

500MB * 10 peers * 10,000 IBDs is 50,000GB. But, that's not where the bug comes in. The bug happens because when there's a new block you ask all 10 peers for their blocks again, and they send them to you from height 0. So assuming it takes 3 hours to IBD, that's 18 blocks. So you download the entire consensus set as much as 180 times. Though really, these connections fail a lot so you probably get cut off.

So, somewhere between 50TB and 500TB, though I'm going to go ahead and expand that number because we did a lot of assuming. So, between 10TB and 1PB.
Copy link
@DavidVorick

DavidVorick Mar 2, 2016

Member

yes you did :)

Average peer has about 10 friends during IBD. Each peer does IBD probably, once, though total we've probably done 10,000 IBDs. Assuming that they're downloading 35,000 blocks on average, and they catch up from block 0, and I know that blocks don't average 100kb, probably closer to 10kb (especially the early blocks are mostly empty). I think the size of the whole blockchain is around 500MB.

500MB * 10 peers * 10,000 IBDs is 50,000GB. But, that's not where the bug comes in. The bug happens because when there's a new block you ask all 10 peers for their blocks again, and they send them to you from height 0. So assuming it takes 3 hours to IBD, that's 18 blocks. So you download the entire consensus set as much as 180 times. Though really, these connections fail a lot so you probably get cut off.

So, somewhere between 50TB and 500TB, though I'm going to go ahead and expand that number because we did a lot of assuming. So, between 10TB and 1PB.
Show outdated Hide outdated modules/consensus/synchronize_test.go
Show outdated Hide outdated modules/consensus/synchronize_test.go
modules/consensus/synchronize_test.go
// TestRPCSendBlockSendsOnlyNecessaryBlocks tests that the SendBlocks RPC only
// sends blocks that the caller does not have and that are part of the longest
// chain.
func TestRPCSendBlockSendsOnlyNecessaryBlocks(t *testing.T) {

This comment has been minimized.

Show comment
Hide comment
@DavidVorick

DavidVorick Mar 1, 2016

Member

ah man. Where was this test 7 months ago?
Copy link
@DavidVorick

DavidVorick Mar 1, 2016

Member

ah man. Where was this test 7 months ago?
Show outdated Hide outdated modules/consensus/synchronize_test.go

DavidVorick added a commit that referenced this pull request Mar 3, 2016

@DavidVorick
Merge pull request #1002 from NebulousLabs/sync-bug-fix 
Sync bug fixes
8b95726

@DavidVorick DavidVorick merged commit 8b95726 into master Mar 3, 2016
1 check passed

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@VoidingWarranties VoidingWarranties deleted the sync-bug-fix branch Mar 7, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.