-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Improving the request of tasks of TaskManager (2x) #900
Changes from 6 commits
f16cd47
42d6b88
f9cef24
ee95314
eb5ec67
0dfca83
e4f106d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -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; | ||||||||
|
@@ -227,7 +228,11 @@ private void RequestTasks(TaskSession session) | |||||||
return; | ||||||||
} | ||||||||
} | ||||||||
if ((!HasHeaderTask || globalTasks[HeaderTaskHash] < MaxConncurrentTasks) && Blockchain.Singleton.HeaderHeight < session.Version.StartHeight) | ||||||||
if ((!HasHeaderTask || globalTasks[HeaderTaskHash] < MaxConncurrentTasks) | ||||||||
&& (Blockchain.Singleton.HeaderHeight < session.Version.StartHeight | ||||||||
|| (Blockchain.Singleton.Height == Blockchain.Singleton.HeaderHeight | ||||||||
&& Blockchain.Singleton.HeaderHeight >= sessions.Select(x => x.Value.Version.StartHeight).Max() | ||||||||
&& TimeProvider.Current.UtcNow.ToTimestamp() - 60 >= Blockchain.Singleton.GetBlock(Blockchain.Singleton.CurrentHeaderHash)?.Timestamp))) | ||||||||
{ | ||||||||
session.Tasks[HeaderTaskHash] = DateTime.UtcNow; | ||||||||
IncrementGlobalTask(HeaderTaskHash); | ||||||||
|
@@ -272,5 +277,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; | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am worried that this will lead to blockage. Can we just limit the length of the mailbox, and if it exceeds a certain length, we will abandon the new package directly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about create a merkle tree for make this faster? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In NEO3, we can make each In NEO2, we urgently need to solve the empty block and the sync stuck problem, so I think that no matter what method can be used as long as it can solve the problem. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My proposal is use this patch, but with a merkle tree There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There are some lines that use this multiple hashes neo/neo/Consensus/ConsensusService.cs Line 445 in cd99489
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shargon, currently this will cause also an attack, even with this fix PR, right? |
||||||||
return false; | ||||||||
} | ||||||||
} | ||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yongjiema, maybe add this case before the removal of duplicated tasks on line 285:
check if
message
is a task withtasks.Payload.Type == InventoryType.Consensus
.Because I believe that we do not need the next check for
blocks
, correct?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean adding the below code between line 283 and line 285?
But I think it doesn't matter for
Consensus
orBlock
as both are high priority, I think you can tryTX
, but I think it may slow down the transaction broadcast.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean the contrary
if (tasks.Payload.Type != InventoryType.Consensus) return false;
aehauheuaea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can try it as most duplicated tasks are consensus, but it still got a chance to make the sync stuck or the empty block happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yongjiema, what I am saying is exactly in this line of reasoning, to avoid the check for blocks, since the most duplicated are consensus. Thus, we would save some efficiency be not needing to check the Queue when it is not Consensus.