Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement simple IBD #190

Merged
merged 6 commits into from Apr 11, 2019

Conversation

Projects
None yet
3 participants
@longfin
Copy link
Member

commented Apr 10, 2019

This PR addresses #187. the following changes were made for this purpose.

  • Added TipIndex as current chain tip Index to Pong to determine peer that has the longest chain.
  • Made Swarm.StartAsync() to compare other chain height using Pong.TipIndex and download blocks if necessary before starts other tasks.

@longfin longfin force-pushed the longfin:feature/ibd-187 branch from 8dc1805 to b98e70c Apr 10, 2019

@codecov

This comment has been minimized.

Copy link

commented Apr 10, 2019

Codecov Report

Merging #190 into master will decrease coverage by 2.79%.
The diff coverage is 77.14%.

@@            Coverage Diff            @@
##           master     #190     +/-   ##
=========================================
- Coverage   87.04%   84.25%   -2.8%     
=========================================
  Files          72       72             
  Lines        3266     3296     +30     
=========================================
- Hits         2843     2777     -66     
- Misses        423      519     +96
Impacted Files Coverage Δ
Libplanet/Net/Messages/Pong.cs 100% <100%> (ø) ⬆️
Libplanet/Net/Swarm.cs 82.55% <74.73%> (-4.35%) ⬇️
Libplanet/Net/IceServer.cs 0% <0%> (-100%) ⬇️
Libplanet/Net/IceServerException.cs 0% <0%> (-100%) ⬇️
Libplanet/Net/NetworkStreamProxy.cs 0% <0%> (-80.77%) ⬇️
@dahlia
Copy link
Member

left a comment

The build seems to fail.

Show resolved Hide resolved CHANGES.md Outdated
Show resolved Hide resolved CHANGES.md Outdated
Show resolved Hide resolved Libplanet/Net/Swarm.cs Outdated
Show resolved Hide resolved Libplanet/Net/Swarm.cs Outdated
Show resolved Hide resolved Libplanet/Net/Swarm.cs Outdated
{
longestPeer = peer;
longestTipIndex = pong.TipIndex.Value;
}

This comment has been minimized.

Copy link
@dahlia

dahlia Apr 11, 2019

Member

Instead of manually comparing the longest peer in the loop, how about making a simple array of pairs like (Peer, long)[] peerLengths and determining the longest peer after the loop ends like (Peer longestPeer, long longestTipIndex) = peerLengths.Max(pair => pair.Item2);?

This comment has been minimized.

Copy link
@longfin

longfin Apr 11, 2019

Author Member

Interesting code. but I'm not sure that there is a big advantage enough to managing a separate array and looping it one more time.

This comment has been minimized.

Copy link
@dahlia

dahlia Apr 11, 2019

Member

At first glance, it was hard to guess the intention, because code is not that short, and finding the peer who has the longest blockchain takes more lines than actual “dialing to existing peers” (as the method name says). If we stick with the current version I suggest to write an explanation as a short comment.

This comment has been minimized.

Copy link
@longfin

longfin Apr 11, 2019

Author Member

Okay, I'll leave comments and/or consider renaming DialToExistingPeers().

@longfin longfin force-pushed the longfin:feature/ibd-187 branch from b98e70c to a54defe Apr 11, 2019

@longfin

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

I've amend to reflect reviews.

Okay, I'll leave comments and/or consider renaming DialToExistingPeers().

At first I tried to have a descriptive name, but it seemed more intuitive to split the method that dials into the known peer and the method that receives the block from the longest chain.

@longfin longfin force-pushed the longfin:feature/ibd-187 branch from a54defe to 4f2678e Apr 11, 2019

Show resolved Hide resolved Libplanet/Net/Swarm.cs Outdated
Show resolved Hide resolved CHANGES.md Outdated

@longfin longfin force-pushed the longfin:feature/ibd-187 branch from 4f2678e to e81ff21 Apr 11, 2019

@longfin

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

Amended. PTAL @dahlia @earlbread

Show resolved Hide resolved CHANGES.md Outdated
Show resolved Hide resolved CHANGES.md Outdated
Show resolved Hide resolved CHANGES.md
Apply suggestions from code review
Co-Authored-By: longfin <longfinfunnel@gmail.com>

@longfin longfin dismissed stale reviews from earlbread and earlbread via 6a45824 Apr 11, 2019

@longfin longfin requested review from dahlia and earlbread Apr 11, 2019

@dahlia

dahlia approved these changes Apr 11, 2019

@longfin longfin merged commit c25a4ca into planetarium:master Apr 11, 2019

3 of 5 checks passed

codecov/patch 77.14% of diff hit (target 87.04%)
Details
codecov/project 84.25% (-2.8%) compared to 078efe7
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
changelog This contains self-describing changelog.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.