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

/Sia

IBD improvements #1013

Merged
merged 17 commits into from Mar 11, 2016

Conversation

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

VoidingWarranties commented Mar 10, 2016

Not complete. Still needs manual timeouts (muxado doesn't timeout correctly nor does it return proper net.Error errors) and tests.
Show outdated Hide outdated modules/consensus/synchronize.go
modules/consensus/consensusset.go
go func() {
// Sync with the network. Don't sync if we are testing because typically we
// don't have any mock peers to synchronize with in testing.
// TODO: figure out a better way to conditionally do IBD. Otherwise this block will never be tested.

This comment has been minimized.

Show comment
Hide comment
@DavidVorick

DavidVorick Mar 10, 2016

Member

one thing you could do is pass a flag in, but that could get pretty annoying.
Copy link
@DavidVorick

DavidVorick Mar 10, 2016

Member

one thing you could do is pass a flag in, but that could get pretty annoying.
Show outdated Hide outdated modules/consensus/synchronize.go
Show outdated Hide outdated modules/consensus/synchronize.go
Show outdated Hide outdated modules/consensus/synchronize.go
Show outdated Hide outdated modules/consensus/synchronize.go

VoidingWarranties added some commits Feb 27, 2016

@VoidingWarranties
Add Inbound field to modules.Peer 
and embed modules.Peer into gateway.peer
and update API docs
43f1dbb
@VoidingWarranties
Set a timeout for SendBlocks RPC
bf63eb6
@VoidingWarranties
Add initial blockchain download logic 
IBD is kicked off in consensus.New.
a1be37d
@VoidingWarranties
Add test for IBD method
e43c300
@VoidingWarranties
Move SetDeadline from rpcSendBlocks to threadedRecvBlocks 
If SetDeadline is called in rpcSendBlocks, peers < v0.5.2 won't be able
to do IBD.
2231c91
@VoidingWarranties
Check for new outbound peers instead of more outbound peers 
The previous implementation wouldn't sync with new peers if the same
number of peers were added as were disconnected from.
ee7e9a0
@VoidingWarranties
Add missing Close()s
8f7fb41
@VoidingWarranties
Adjust synchronize constants as per reviewer suggestions 
MaxCatchUpBlocks is 50 for dev because dev networks have smaller blocks
and are on local networks.

sendBlocksTimeout is 40 seconds for dev because blocks are so small
e0b6734
@VoidingWarranties
Simplify IBD done conditions 
IBD is now done when there are >= minNumOutbound peers synced
OR
there is >= 1 peer synced and 10 minutes have passed since beginning IBD
01ffbb4
@VoidingWarranties
Add integration test to verify the SendBlocks RPC stalls
acabc5f
@VoidingWarranties

This comment has been minimized.

Show comment
Hide comment
@VoidingWarranties

VoidingWarranties Mar 11, 2016

Collaborator

How long is too long for a long test? TestInitialBlockchainDownloadDoneRules takes 44 seconds.
Collaborator

VoidingWarranties commented Mar 11, 2016

How long is too long for a long test? TestInitialBlockchainDownloadDoneRules takes 44 seconds.
@DavidVorick

This comment has been minimized.

Show comment
Hide comment
@DavidVorick

DavidVorick Mar 11, 2016

Member

44 seconds is fine, keep the whole thing under 5 minutes if possible. But even 10 minutes isn't too bad.
Member

DavidVorick commented Mar 11, 2016

44 seconds is fine, keep the whole thing under 5 minutes if possible. But even 10 minutes isn't too bad.
Show outdated Hide outdated modules/consensus.go
Show outdated Hide outdated modules/consensus/subscribe.go
Show outdated Hide outdated modules/consensus/synchronize.go
@VoidingWarranties

This comment has been minimized.

Show comment
Hide comment
@VoidingWarranties

VoidingWarranties Mar 11, 2016

Collaborator

TestInitialBlockchainDownloadDoneRules only takes so long because minIBDWaitTime is 10 seconds during testing. It needs to be 10 seconds so that large reorg tests have enough time to complete. However, this specific test doesn't involve any block transfers so we could manually set minIBDWaitTime to 1s for just this test. That would shorten this test to just 5 seconds.
Collaborator

VoidingWarranties commented Mar 11, 2016

TestInitialBlockchainDownloadDoneRules only takes so long because minIBDWaitTime is 10 seconds during testing. It needs to be 10 seconds so that large reorg tests have enough time to complete. However, this specific test doesn't involve any block transfers so we could manually set minIBDWaitTime to 1s for just this test. That would shorten this test to just 5 seconds.
@DavidVorick

This comment has been minimized.

Show comment
Hide comment
@DavidVorick

DavidVorick Mar 11, 2016

Member

No need to worry about it. Since we use t.Parallel() (or at least, we're migrating to using it) there shouldn't be any issues with long sleeps. As long as the whole suite (per package) is finishing inside of 5 minutes, there's nothing to worry about.
Member

DavidVorick commented Mar 11, 2016

No need to worry about it. Since we use t.Parallel() (or at least, we're migrating to using it) there shouldn't be any issues with long sleeps. As long as the whole suite (per package) is finishing inside of 5 minutes, there's nothing to worry about.

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

@DavidVorick
Merge pull request #1013 from NebulousLabs/ibd 
IBD improvements
9897d3f

@DavidVorick DavidVorick merged commit 9897d3f into master Mar 11, 2016
1 check passed

1 check passed

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

This was referenced Mar 11, 2016

Closed
Merged

@lukechampine lukechampine deleted the ibd branch Mar 17, 2016

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