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

Sync enhancement: first version of Lightning sync #635

Merged
merged 38 commits into from Oct 2, 2018

Conversation

Projects
None yet
5 participants
@AlexandraRoatis
Copy link
Contributor

commented Sep 13, 2018

Description

The first version of Lightning sync that improves the block syncing functionality by requesting blocks out of order ahead of import time and storing them temporarily to disk until they can be imported.

This implementation leverages the fact that under normal circumstances there is a lot of repeated work being done due to requesting the same blocks multiple times from different peers.

Experimental results show that the full sync time is reduced by a factor of up to 10x when the kernel has a large number of peers. It achieves around 2x to 5x speedup with 10 to 40 nodes.

Fixes Issue #27.

Type of change

Insert x into the following checkboxes to confirm (eg. [x]):

  • Bug fix.
  • New feature.
  • Enhancement.
  • Unit test.
  • Breaking change (a fix or feature that causes existing functionality to not work as expected).
  • Requires documentation update.

Testing

Please describe the tests you used to validate this pull request. Provide any relevant details for test configurations as well as any instructions to reproduce these results.

  • Existing suite of tests, plus unit tests for the critical functionality introduced by this implementation.

Verification

Insert x into the following checkboxes to confirm (eg. [x]):

  • I have self-reviewed my own code and conformed to the style guidelines of this project.
  • New and existing tests pass locally with my changes.
  • I have added tests for my fix or feature.
  • I have made appropriate changes to the corresponding documentation.
  • My code generates no new warnings.
  • Any dependent changes have been made.
@AlexandraRoatis

This comment has been minimized.

Copy link
Contributor Author

commented Sep 13, 2018

Note: one of the unit tests requires persistence which is why the LevelDB jar is added to the build file. I took note of this issue #636 and plan for an implementation of a persistent mock database in the future.

@AionJayT AionJayT added this to the 0.3.2 milestone Sep 13, 2018

@@ -70,22 +75,31 @@
this.log = log;
}

/** Checks that the peer's total difficulty is higher than the local chain. */
private boolean isHigherTotalDifficulty(INode n) {
return n.getTotalDifficulty() != null && n.getTotalDifficulty().compareTo(this.selfTd) >= 0;

This comment has been minimized.

Copy link
@AionJayT

AionJayT Sep 17, 2018

Collaborator

is the ">=" an expecting behavior? cause the method name is higher than TD

This comment has been minimized.

Copy link
@aion-kelvin

aion-kelvin Sep 20, 2018

Collaborator

Maybe "isTotalDifficultyOrHigher" is a more correct name because of >=

This comment has been minimized.

Copy link
@AlexandraRoatis

AlexandraRoatis Sep 24, 2018

Author Contributor

Technically, since there is a time delay between receiving the peer status and performing this check, a peer with a total difficulty equal to the local one has likely already advanced to a higher TD.
Still, I will rename the method to make it less ambiguous.

This comment has been minimized.

Copy link
@AlexandraRoatis

AlexandraRoatis Sep 25, 2018

Author Contributor

updated in 22a2d69

}

@Override
public void run() {
Thread.currentThread().setPriority(Thread.MAX_PRIORITY);
while (start.get()) {

BlocksWrapper bw;
try {
bw = downloadedBlocks.take();
} catch (InterruptedException ex) {
return;

This comment has been minimized.

Copy link
@AionJayT

AionJayT Sep 17, 2018

Collaborator

Add error log in this exception.

This comment has been minimized.

Copy link
@AlexandraRoatis

AlexandraRoatis Sep 24, 2018

Author Contributor

Please check if you agree with my change from b5bbd67. I am logging the error only when shutdown was not called since it is to be expected that this will get interrupted at shutdown.

t2 - t1);
if (log.isDebugEnabled()) {
// printing sync mode only when debug is enabled
log.info(

This comment has been minimized.

Copy link
@AionJayT

AionJayT Sep 17, 2018

Collaborator

since it's a debugging log. should put it as the debug level log.

This comment has been minimized.

Copy link
@AlexandraRoatis

AlexandraRoatis Sep 25, 2018

Author Contributor

updated in b5bbd67


batch++;

if (last <= b.getNumber()) {

This comment has been minimized.

Copy link
@AionJayT

AionJayT Sep 17, 2018

Collaborator

has any change to import more blocks when last < b.getNumber()?

This comment has been minimized.

Copy link
@AlexandraRoatis

AlexandraRoatis Sep 25, 2018

Author Contributor

you're right, last will be at most equal to the block number; correction made in b5bbd67


p2pLOG.info(status);

if (p2pLOG.isDebugEnabled()) {

This comment has been minimized.

Copy link
@AionJayT

AionJayT Sep 17, 2018

Collaborator

Should it be a TRACE level? Cause this information can be huge.

This comment has been minimized.

Copy link
@AlexandraRoatis

AlexandraRoatis Sep 24, 2018

Author Contributor

It's similar to the p2p printout which is placed in debug. I don't think it belongs in trace logging, because it's infrequent. But if you have a strong preference for moving it to trace, I will make the change.

@AionJayT
Copy link
Collaborator

left a comment

few small questions during the review. some of the comments are the suggestion. Will take another look for the fast syncing mechanism,

AlexandraRoatis added some commits Sep 7, 2018

@AlexandraRoatis AlexandraRoatis force-pushed the sync-enhancement branch from 504aa36 to 4d4f3f6 Oct 1, 2018

@AlexandraRoatis AlexandraRoatis removed request for iamyulong and aionick Oct 2, 2018

@qoire

qoire approved these changes Oct 2, 2018

Copy link
Collaborator

left a comment

LGTM


private long start;

private long startBlock;

private double avgBlocksPerSec;

SyncStatics(long _startBlock){
SyncStats(long _startBlock){

This comment has been minimized.

Copy link
@qoire

qoire Oct 2, 2018

Collaborator

Not a comment for this issue, but we would do well to expand on sync statistics. Gives us some greater insight into the behaviour of nodes on the network. For example:

  • The percentage of requests for each peer, and who is giving you the majority of blocks (top seeds)
  • Who is requesting blocks from you, (top leeches)
  • Average response time between sending blocks out, and that peer responding
  • (Average blocks per second is good, we already have that yay!)

This comment has been minimized.

Copy link
@AlexandraRoatis

AlexandraRoatis Oct 2, 2018

Author Contributor

Candidate for a bounty?

@AionJayT AionJayT merged commit 67e953e into master-pre-merge Oct 2, 2018

1 check passed

continuous-integration/jenkins/pr-merge This commit looks good
Details

@AionJayT AionJayT deleted the sync-enhancement branch Oct 2, 2018

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.