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.

Sign up for GitHub

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

Fix/Improve syncing (3x): Adding ping mechanism to TaskManager for replacing StartHeight and PendingKnownHashes strategy #899

Merged
merged 33 commits into from
Dec 1, 2019
Merged

Fix/Improve syncing (3x): Adding ping mechanism to TaskManager for replacing StartHeight and PendingKnownHashes strategy #899

Changes from 9 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
99ca3f2
Drop the duplicate NewTasks for the TaskManagerMailbox
yongjiema Jul 9, 2019
2048e7b
Try to get headers from remote nodes if the current header is not upd…
yongjiema Jul 9, 2019
079a74b
Merge branch 'master' into fix-sync-stuck
shargon Jul 9, 2019
755ab15
Update TaskManager.cs
shargon Jul 9, 2019
4ed0e22
Update TaskManager.cs
shargon Jul 9, 2019
4b410ad
Merge branch 'master' into fix-sync-stuck
vncoelho Jul 11, 2019
64dd8bd
Merge branch 'master' into fix-sync-stuck
shargon Jul 30, 2019
58d2ff0
Merge branch 'master' into fix-sync-stuck
vncoelho Aug 7, 2019
9e271e1
Merge branch 'master' into fix-sync-stuck
vncoelho Aug 9, 2019
7b32b10
Merge branch 'master' into fix-sync-stuck
shargon Aug 10, 2019
ae46f80
Merge branch 'master' into fix-sync-stuck
shargon Aug 12, 2019
70b2072
Merge branch 'master' into fix-sync-stuck
vncoelho Aug 12, 2019
b44e1eb
Try to get headers from remote nodes even the current header height i…
yongjiema Aug 13, 2019
bf01bab
Correct the timestamp usage according to NEO3
yongjiema Aug 16, 2019
261e7ff
Merge branch 'master' into fix-sync-stuck
erikzhang Aug 18, 2019
152a7cb
Merge branch 'master' into fix-sync-stuck
vncoelho Aug 19, 2019
cd5cb5f
Merge branch 'master' into fix-sync-stuck
vncoelho Aug 28, 2019
692a9ad
Merge branch 'master' into fix-sync-stuck
vncoelho Sep 24, 2019
11e811c
Merge branch 'master' into fix-sync-stuck
lock9 Sep 26, 2019
8196dff
Improve syncing
yongjiema Sep 29, 2019
0e21d78
Merge branch 'master' into fix-sync-stuck
vncoelho Oct 23, 2019
bd3b3fc
Merge branch 'master' into fix-sync-stuck
vncoelho Nov 4, 2019
fe2d98a
Merge branch 'master' into fix-sync-stuck
vncoelho Nov 10, 2019
100b30d
Merge branch 'master' into fix-sync-stuck
vncoelho Nov 13, 2019
ebe25e2
Merge branch 'master' into fix-sync-stuck
vncoelho Nov 13, 2019
fd0e712
Merge branch 'master' into fix-sync-stuck
lock9 Nov 13, 2019
9a73bf8
Merge branch 'master' into fix-sync-stuck
erikzhang Nov 25, 2019
04fdfa8
Update ProtocolHandler.cs
erikzhang Nov 25, 2019
1d985e0
Update ProtocolHandler.cs
erikzhang Nov 25, 2019
daf7141
move projects
erikzhang Nov 26, 2019
75f4470
Merge branch 'master' into fix-sync-stuck
erikzhang Nov 26, 2019
e18184e
Merge branch 'master' into fix-sync-stuck
vncoelho Nov 29, 2019
98f11f8
Merge branch 'master' into fix-sync-stuck
erikzhang Dec 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion neo/Network/P2P/TaskManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using Neo.Ledger;
using Neo.Network.P2P.Payloads;
using System;
using System.Collections;
using System.Collections.Generic;
using System.Linq;
using System.Runtime.CompilerServices;
Expand Down Expand Up @@ -221,7 +222,11 @@ private void RequestTasks(TaskSession session)
return;
}
}
if ((!HasHeaderTask || globalTasks[HeaderTaskHash] < MaxConncurrentTasks) && Blockchain.Singleton.HeaderHeight < session.StartHeight)
if ((!HasHeaderTask || globalTasks[HeaderTaskHash] < MaxConncurrentTasks)
&& (Blockchain.Singleton.HeaderHeight < session.StartHeight
|| (Blockchain.Singleton.HeaderHeight == session.StartHeight
&& Blockchain.Singleton.HeaderHeight == sessions.Select(x => x.Value.StartHeight).Max()
&& TimeProvider.Current.UtcNow.ToTimestamp() - 60 >= Blockchain.Singleton.GetBlock(Blockchain.Singleton.CurrentHeaderHash)?.Timestamp)))
{
session.Tasks[HeaderTaskHash] = DateTime.UtcNow;
IncrementGlobalTask(HeaderTaskHash);
Expand Down Expand Up @@ -266,5 +271,13 @@ internal protected override bool IsHighPriority(object message)
return false;
}
}

internal protected override bool ShallDrop(object message, IEnumerable queue)
{
if (!(message is TaskManager.NewTasks tasks)) return false;
// Remove duplicate tasks
if (queue.OfType<TaskManager.NewTasks>().Any(x => x.Payload.Type == tasks.Payload.Type && x.Payload.Hashes.SequenceEqual(tasks.Payload.Hashes))) return true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should think more about this line.

Node A sends an inv for a block, and node B sends the same. In this case, the second message will be dropped. But if node A doesn't send block, we will stop sync.

Copy link
Member

@vncoelho vncoelho Aug 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

O.o, makes sense, @erikzhang. Nice catch and visualization!
We are destroying master2x... Just kidding, I think it will be ok there for now! ahueahuea

Can we personalize this IEnumerable queue a little bit more? If we set timestamps for the objects. Maybe also some improvements could come on the management of ProtocolHandler.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then maybe we need the duplicates, i think that with the FIFO's patch and the own messages patch, will be enought, it was tested with this both patches and without this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am testing a change for known hashes behaviour in ProtocolHandler related it, will share the code after the testing is done.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the PR #1024 which is for 2x.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think we should remove this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, let's remove this and open it in another PR.

I think that we added it before on master2.x. But we can separate it here and analyse in another moment.

Copy link
Member

@vncoelho vncoelho Nov 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should think more about this line.

Node A sends an inv for a block, and node B sends the same. In this case, the second message will be dropped. But if node A doesn't send block, we will stop sync.

@erikzhang, however, maybe, with PendingHashes it will not happen infinitely, since the hash will expire.

return false;
}
}
}