Improving the request of tasks of TaskManager (2x) #900
Conversation
|
Please @yongjiema , take a look here yongjiema#2 |
neo/Network/P2P/TaskManager.cs
Outdated
| { | ||
| 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.
| if (queue.OfType<TaskManager.NewTasks>().Any(x => x.Payload.Type == tasks.Payload.Type && x.Payload.Hashes.SequenceEqual(tasks.Payload.Hashes))) return true; | |
| if (queue.OfType<TaskManager.NewTasks>().AsParallel().Any(x => x.Payload.Type == tasks.Payload.Type && x.Payload.Hashes.SequenceEqual(tasks.Payload.Hashes))) return true; |
|
Great job and PR! Congrulations for tackling this issue. TM-IHP: Consensus I AM...
PH-IHP: Consensus...
PH-IHP: Consensus...
TM-IHP: Consensus I AM...
TM-IHP: Consensus I AM...
[15:21:20.168] hash=0x6141e5d06780698b3b6022bd8f8fc58620efce8c3c82b46c692e05d27c6b8dc0
[15:21:20.169] OnPrepareResponseReceived: height=2 view=0 index=3 hash=0x6141e5d06780698b3b6022bd8f8fc58620efce8c3c82b46c692e05d27c6b8dc0
TM-IHP: Consensus I AM...
PH-IHP: Consensus...
TM-IHP: Consensus I AM...
[15:21:20.171] hash=0xd38d383d3f10ce2ce15cf194f1b8a6793f98c61074dbd87e64b4eb59eb966755
PH-IHP: Consensus...
OnGetDataMessageReceived(PH): 0x6141e5d06780698b3b6022bd8f8fc58620efce8c3c82b46c692e05d27c6b8dc0
OnGetDataMessageReceived(PH): 0x6141e5d06780698b3b6022bd8f8fc58620efce8c3c82b46c692e05d27c6b8dc0
TM-IHP: Consensus I AM...
TM-IHP: Consensus I AM...
[15:21:20.181] hash=0xdb101a414c8a09ad77e96b6bebac843918c35946df4cdcda87ebddecb3e1a6ca
[15:21:20.181] OnPrepareResponseReceived: height=2 view=0 index=1 hash=0xdb101a414c8a09ad77e96b6bebac843918c35946df4cdcda87ebddecb3e1a6ca
[15:21:20.183] send commit
PH-IHP: Consensus...
PH-IHP: Consensus...
TM-IHP: Consensus I AM...
OnGetDataMessageReceived(PH): 0x6141e5d06780698b3b6022bd8f8fc58620efce8c3c82b46c692e05d27c6b8dc0Trying to understand this a lit bit more before merging. Let's extend the review for some more couple of days. |
PH-IHP: Consensus...
OnInvMessageReceived(PH): Neo.UInt256[] Neo.UInt256[]
printingPayloadHashes(PH): 0xfd5543305bb114f20390c66d84e12f5a594baae8b5d4db4a50a812ac5d0a6714
printingHashesFiltered(PH): 0xfd5543305bb114f20390c66d84e12f5a594baae8b5d4db4a50a812ac5d0a6714
TM-IHP: Consensus I AM...
OnInvMessageReceived(PH): Neo.UInt256[] Neo.UInt256[]
printingPayloadHashes(PH): 0xfd5543305bb114f20390c66d84e12f5a594baae8b5d4db4a50a812ac5d0a6714
printingHashesFiltered(PH): 0xfd5543305bb114f20390c66d84e12f5a594baae8b5d4db4a50a812ac5d0a6714
TM-IHP: Consensus I AM...
[15:43:39.249] hash=0xfd5543305bb114f20390c66d84e12f5a594baae8b5d4db4a50a812ac5d0a6714
OnInvMessageReceived(PH): Neo.UInt256[] Neo.UInt256[]
printingPayloadHashes(PH): 0xfd5543305bb114f20390c66d84e12f5a594baae8b5d4db4a50a812ac5d0a6714
printingHashesFiltered(PH): 0xfd5543305bb114f20390c66d84e12f5a594baae8b5d4db4a50a812ac5d0a6714
TM-IHP: Consensus I AM...
[15:43:39.284] OnRecoveryRequestReceived: height=1 index=1 view=0 hash=0xfd5543305bb114f20390c66d84e12f5a594baae8b5d4db4a50a812ac5d0a6714
[15:43:39.284] send recovery: view=0
OnInvMessageReceived(PH): Neo.UInt256[] Neo.UInt256[]
printingPayloadHashes(PH): 0xc6c1944ae7da001698b6cdf73d7dcc1d930769767a8f7a7c61961ab7793615de
printingHashesFiltered(PH): 0xc6c1944ae7da001698b6cdf73d7dcc1d930769767a8f7a7c61961ab7793615de
TM-IHP: Consensus I AM...
OnInvMessageReceived(PH): Neo.UInt256[] Neo.UInt256[]
printingPayloadHashes(PH): 0xc6c1944ae7da001698b6cdf73d7dcc1d930769767a8f7a7c61961ab7793615de
printingHashesFiltered(PH): 0xc6c1944ae7da001698b6cdf73d7dcc1d930769767a8f7a7c61961ab7793615de
TM-IHP: Consensus I AM...
OnInvMessageReceived(PH): Neo.UInt256[] Neo.UInt256[]
printingPayloadHashes(PH): 0xc6c1944ae7da001698b6cdf73d7dcc1d930769767a8f7a7c61961ab7793615de
printingHashesFiltered(PH): 0xc6c1944ae7da001698b6cdf73d7dcc1d930769767a8f7a7c61961ab7793615de
TM-IHP: Consensus I AM...
PH-IHP: Consensus...
[15:43:39.332] hash=0xc6c1944ae7da001698b6cdf73d7dcc1d930769767a8f7a7c61961ab7793615de
[15:43:39.346] OnRecoveryMessageReceived: height=1 view=0 index=2 hash=0xc6c1944ae7da001698b6cdf73d7dcc1d930769767a8f7a7c61961ab7793615de |
|
@vncoelho please test it with my patch yongjiema#2, all works fine ? |
|
Great, Shargon, these logs above where without this PR as well, still from the original code in master2x. |
|
I think that my patch break the consensus..... |
|
Check this patch, #904 known hashes maybe was not working well |
|
Sounds like a great finding, Shargon! |
|
@yongjiema, were your tests done on last master-2x or using a released client? Please check with this last patches. |
@vncoelho Tested on last master-2x, my local already have the fix which is same with the patch. |
|
@KickSeason Could you confirm if this fixes the issue? |
|
There are more duplicates after the known hashes fix? |
|
@shargon are you saying that the known hashes actually made this worse? 🤔 |
|
not worse, only asking about if is still necessary |
|
Exactly, as @shargon is highlighting, we need the release 2.9.3 before merging this, being sure that this is a necessary change for master-2x. If that is good for 2x we are also going to apply for 3x. |
|
@yongjiema I've applied this PR on one of Mainnet seed nodes and Testnet2 consensus nodes. Seems the empty block bug has been solved. But not sure it'll fix the sync stuck since I found the mainnet seed node stop syncing yesterday for several minutes but finally it returned back. And I found CPU and memory cost was increasing continuoulsy until reached a certain value. Then it became stable. I'll watch it for three more days to ensure if it fix sync stuck. Here's the log: |
Good to hear that!
How long is it? Do you the result of
How much is the total memory? 4GB? |
How long is it? Do you the result of show state command during that period? Is the system time correct at that node?Not sure. I use the monitor (https://seed.ngd.network/) to watch seed nodes' situation. I applied your version on seed8 for about 1 week and yesterday when I took a look, I found seed8 was about hundreds of blocks falling behind. However, in this morning, I found it was back to the latest. This monitor is using RPC request (getblockcount) to get block height. How much is the total memory? 4GB?8GB. |
|
I find when most seed nodes falled behind, your version still worked well. The sync stuck only appeared for a while and they soon catched up. Seems a good performance. The cost of CPU and MEM doesn't increase anymore. |
Then it should be OK, this is a case this PR try to handle it by commit (42d6b88), but we can introduce a faster solution in the future as I just want to do a small change previously. |
…s higher than them if it is not updated for a while
|
@superboyiii I just pushed a commit (eb5ec67) which can handle another sync problem. |
|
@yongjiema, we pushed a PR with this last commit before, but at that time it was not accepted and kept with start height. We need to remember the reason. Maybe something related to spam the network naturally. Not sure anymore, because other changes came after that. But, in general, I agree, use the start height does not look so good without the ping. |
There was a problem hiding this comment.
@yongjiema, @shargon, @erikzhang
The check && x.Payload.Hashes.SequenceEqual(tasks.Payload.Hashes) checks the entire sequence of the Payload, forcing a drop of natural rebroadcasted tasks, right?
However, an attack would still be possible if someone modifies the sequence of hashes, correct?
Yes, or creating a new ones |
@vncoelho This PR is not trying to resolve the existing potential network attacks, I think the solution of network attacks should be a basic strategy for all connections. |
@vncoelho I will wait for your review before merging. |
neo/Network/P2P/TaskManager.cs
Outdated
|
|
||
| internal protected override bool ShallDrop(object message, IEnumerable queue) | ||
| { | ||
| if (!(message is TaskManager.NewTasks tasks)) return false; |
There was a problem hiding this comment.
@yongjiema, maybe add this case before the removal of duplicated tasks on line 285:
check if message is a task with tasks.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.
Do you mean adding the below code between line 283 and line 285?
if (tasks.Payload.Type == InventoryType.Consensus) return false;But I think it doesn't matter for Consensus or Block as both are high priority, I think you can try TX, but I think it may slow down the transaction broadcast.
There was a problem hiding this comment.
I mean the contrary if (tasks.Payload.Type != InventoryType.Consensus) return false;
aehauheuaea
There was a problem hiding this comment.
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.
@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.
|
Please take a look at this log, the problem still persists, OnReceive-CP-OnInventoryReceived(PH): **0xa1650e7ae34ff408e6b2122417e71857ab079ab38c254c2dfeacaefbd2a05799** // Message arrives in Protocol Handler, goes to the switch and case `consensus`
OnInventoryReceived - Task **0xa1650e7ae34ff408e6b2122417e71857ab079ab38c254c2dfeacaefbd2a05799** completed (PH) // It becomes IIventory of type Consensus and Task Manager and Local Nodes are notified
OnNewTasks(TM)-BGN-1147100768
OnNewTasks(TM)-BKS-1147100768:
NewTaskHashesOnTM(TM)-1147100768: 0xae2edbad1dcc4754f85d1b073ae3b0e06f5c3cc7162341e61082718231acd4f5
OnNewTasks(TM)-AKS-1147100768-count: 0
OnNewTasks(TM)-AGT-1147100768-count: 0
OnInvMessageReceived(PH):
OnInvMessageReceived(PH):
printingPayloadHashes(PH)-1803982457: 0x52971036eaca911a5982169cddd04d50dfc13e52b07c36fc491aa44a35f4282c
printingPayloadHashes(PH)-847339451: 0x52971036eaca911a5982169cddd04d50dfc13e52b07c36fc491aa44a35f4282c
printingHashesFiltered(PH)-847339451: 0x52971036eaca911a5982169cddd04d50dfc13e52b07c36fc491aa44a35f4282c
printingHashesFiltered(PH)-1803982457: 0x52971036eaca911a5982169cddd04d50dfc13e52b07c36fc491aa44a35f4282c
TM-IHP: Consensus I AM...
TM-IHP: Consensus I AM...
OnInvMessageReceived(PH):
printingPayloadHashes(PH)-2077882442: 0xfcef703bd9f60a77e15e8a3b3d05e20fb6db24c100ad92857a164a159dbca62c
OnInvMessageReceived(PH):
printingPayloadHashes(PH)-1532328650: 0xae2edbad1dcc4754f85d1b073ae3b0e06f5c3cc7162341e61082718231acd4f5
printingHashesFiltered(PH)-1532328650: 0xae2edbad1dcc4754f85d1b073ae3b0e06f5c3cc7162341e61082718231acd4f5
TM-IHP: Consensus I AM...
printingHashesFiltered(PH)-2077882442: 0xfcef703bd9f60a77e15e8a3b3d05e20fb6db24c100ad92857a164a159dbca62c
TM-IHP: Consensus I AM...
OnInvMessageReceived(PH):
printingPayloadHashes(PH)-1048543181: **0xa1650e7ae34ff408e6b2122417e71857ab079ab38c254c2dfeacaefbd2a05799** // OnInvMessageReceived of Protocol Handler notifies an InvPayload of Consensus
OnNewTasks(TM)-BGN-316305475 // TaskManager consequently receives it I
printingHashesFiltered(PH)-1048543181: **0xa1650e7ae34ff408e6b2122417e71857ab079ab38c254c2dfeacaefbd2a05799** // Filtered from PH, almost the same as the previous msg from it, it is out of order
TM-IHP: Consensus I AM...
OnInvMessageReceived(PH):
OnNewTasks(TM)-BKS-316305475: // TaskManager consequently receives it II
NewTaskHashesOnTM(TM)-316305475: 0x52971036eaca911a5982169cddd04d50dfc13e52b07c36fc491aa44a35f4282c // TaskManager consequently receives it III BUT THE Hash is different: 0x52971036eaca911a5982169cddd04d50dfc13e52b07c36fc491aa44a35f4282c
printingPayloadHashes(PH)-2019427263: 0xfcef703bd9f60a77e15e8a3b3d05e20fb6db24c100ad92857a164a159dbca62c
printingHashesFiltered(PH)-2019427263: 0xfcef703bd9f60a77e15e8a3b3d05e20fb6db24c100ad92857a164a159dbca62c
OnNewTasks(TM)-AKS-316305475-count: 0 // NO Consensus Payload was detected to be processed, it has been filtered
OnNewTasks(TM)-AGT-316305475-count: 0 // NO Consensus Payload was detected to be processed, it has been filtered, OnNewTasks EXIT without reaching its END
OnRelay(LN): **0xa1650e7ae34ff408e6b2122417e71857ab079ab38c254c2dfeacaefbd2a05799**
OnNewTasks(TM)-BGN-187516306
OnInvMessageReceived(PH):
printingPayloadHashes(PH)-1403265557: **0xa1650e7ae34ff408e6b2122417e71857ab079ab38c254c2dfeacaefbd2a05799** // OnInvMessageReceived of Protocol Handler notifies an InvPayload of Consensus
printingHashesFiltered(PH)-1403265557: **0xa1650e7ae34ff408e6b2122417e71857ab079ab38c254c2dfeacaefbd2a05799**
OnNewTasks(TM)-BKS-187516306:
NewTaskHashesOnTM(TM)-187516306: 0x52971036eaca911a5982169cddd04d50dfc13e52b07c36fc491aa44a35f4282c
OnNewTasks(TM)-AKS-187516306-count: 0
OnNewTasks(TM)-AGT-187516306-count: 0
OnNewTasks(TM)-BGN-95397302
OnNewTasks(TM)-BKS-95397302:
NewTaskHashesOnTM(TM)-95397302: 0xae2edbad1dcc4754f85d1b073ae3b0e06f5c3cc7162341e61082718231acd4f5
OnNewTasks(TM)-AKS-95397302-count: 0
OnNewTasks(TM)-AGT-95397302-count: 0
OnNewTasks(TM)-BGN-2023390691
OnNewTasks(TM)-BKS-2023390691:
NewTaskHashesOnTM(TM)-2023390691: 0xfcef703bd9f60a77e15e8a3b3d05e20fb6db24c100ad92857a164a159dbca62c
OnNewTasks(TM)-AKS-2023390691-count: 1
OnNewTasks(TM)-AGT-2023390691-count: 1
OnNewTasks(TM)-END-2023390691:
OnNewTasks(TM)-BGN-41171727 // TASK MANAGER BEGINS
OnNewTasks(TM)-BKS-41171727:
NewTaskHashesOnTM(TM)-41171727: **0xa1650e7ae34ff408e6b2122417e71857ab079ab38c254c2dfeacaefbd2a05799**
OnNewTasks(TM)-AKS-41171727-count: 1
OnNewTasks(TM)-AGT-41171727-count: 1
OnNewTasks(TM)-END-41171727: // TASK MANAGER END, THIS TIME WITH SUCESS
[10:41:33.478] OnConsensusPayload hash=0x52971036eaca911a5982169cddd04d50dfc13e52b07c36fc491aa44a35f4282c
[10:41:33.479] OnCommitReceived: height=6 view=0 index=0 nc=1 nf=0 hash=0x52971036eaca911a5982169cddd04d50dfc13e52b07c36fc491aa44a35f4282c
PH-IHP: Consensus...
OnReceive-CP-OnInventoryReceived(PH): 0xfcef703bd9f60a77e15e8a3b3d05e20fb6db24c100ad92857a164a159dbca62c
OnInventoryReceived - Task 0xfcef703bd9f60a77e15e8a3b3d05e20fb6db24c100ad92857a164a159dbca62c completed (PH)
OnInvMessageReceived(PH):
printingPayloadHashes(PH)-2064377315: **0xa1650e7ae34ff408e6b2122417e71857ab079ab38c254c2dfeacaefbd2a05799** // OnInvMessageReceived of Protocol Handler notifies an InvPayload of Consensus
printingHashesFiltered(PH)-2064377315: **0xa1650e7ae34ff408e6b2122417e71857ab079ab38c254c2dfeacaefbd2a05799**
TM-IHP: Consensus I AM...
OnRelay(LN): 0xfcef703bd9f60a77e15e8a3b3d05e20fb6db24c100ad92857a164a159dbca62c
OnInvMessageReceived(PH):
printingPayloadHashes(PH)-56733811: 0xfcef703bd9f60a77e15e8a3b3d05e20fb6db24c100ad92857a164a159dbca62c
printingHashesFiltered(PH)-56733811: 0xfcef703bd9f60a77e15e8a3b3d05e20fb6db24c100ad92857a164a159dbca62c
TM-IHP: Consensus I AM...
OnInvMessageReceived(PH):
printingPayloadHashes(PH)-1727359454: 0x177a5167f86afe4f08b25a1e89c53cffc5d50d08ded669c83fc64941ff88e934
printingHashesFiltered(PH)-1727359454: 0x177a5167f86afe4f08b25a1e89c53cffc5d50d08ded669c83fc64941ff88e934
TM-IHP: Consensus I AM...
OnInvMessageReceived(PH):
printingPayloadHashes(PH)-376189730: **0xa1650e7ae34ff408e6b2122417e71857ab079ab38c254c2dfeacaefbd2a05799** // OnInvMessageReceived of Protocol Handler notifies an InvPayload of Consensus
printingHashesFiltered(PH)-376189730: **0xa1650e7ae34ff408e6b2122417e71857ab079ab38c254c2dfeacaefbd2a05799**
OnInvMessageReceived(PH):
printingPayloadHashes(PH)-1908722687: 0xfcef703bd9f60a77e15e8a3b3d05e20fb6db24c100ad92857a164a159dbca62c
printingHashesFiltered(PH)-1908722687: 0xfcef703bd9f60a77e15e8a3b3d05e20fb6db24c100ad92857a164a159dbca62c
OnInvMessageReceived(PH):
printingPayloadHashes(PH)-43468792: 0x177a5167f86afe4f08b25a1e89c53cffc5d50d08ded669c83fc64941ff88e934
printingHashesFiltered(PH)-43468792: 0x177a5167f86afe4f08b25a1e89c53cffc5d50d08ded669c83fc64941ff88e934
OnNewTasks(TM)-BGN-1070228700
OnNewTasks(TM)-BKS-1070228700:
NewTaskHashesOnTM(TM)-1070228700: **0xa1650e7ae34ff408e6b2122417e71857ab079ab38c254c2dfeacaefbd2a05799** // OnInvMessageReceived of Protocol Handler notifies an InvPayload of ConsensusWhile I was commenting the screen flushed...aheuahuea There is something really strange, please check the logs and comments attached @shargon @igormcoelho @erikzhang |
|
@yongjiema, look how this is strange: PH-IHP: Consensus... I
OnReceive-CP-OnInventoryReceived(PH) - II: 0x030f32bb25d352a5358669c87d68659b839f4c6e5248924bda5b04692e54d5f6
OnInventoryReceived - Task Completed will be called (PH): 0x030f32bb25d352a5358669c87d68659b839f4c6e5248924bda5b04692e54d5f6
OnInventoryReceived - Local Node Relay will be called (PH): 0x030f32bb25d352a5358669c87d68659b839f4c6e5248924bda5b04692e54d5f6
OnRelay(LN): 0x030f32bb25d352a5358669c87d68659b839f4c6e5248924bda5b04692e54d5f6
OnInvMessageReceived(PH):
printingPayloadHashes(PH)-866702590: 0x030f32bb25d352a5358669c87d68659b839f4c6e5248924bda5b04692e54d5f6
printingHashesFiltered(PH)-866702590: 0x030f32bb25d352a5358669c87d68659b839f4c6e5248924bda5b04692e54d5f6
OnInvMessageReceived(PH):
printingPayloadHashes(PH)-384175155: 0x030f32bb25d352a5358669c87d68659b839f4c6e5248924bda5b04692e54d5f6
printingHashesFiltered(PH)-384175155: 0x030f32bb25d352a5358669c87d68659b839f4c6e5248924bda5b04692e54d5f6
OnInvMessageReceived(PH):
printingPayloadHashes(PH)-1779624940: 0x030f32bb25d352a5358669c87d68659b839f4c6e5248924bda5b04692e54d5f6
printingHashesFiltered(PH)-1779624940: 0x030f32bb25d352a5358669c87d68659b839f4c6e5248924bda5b04692e54d5f6
TM-IHP: Consensus I AM...
TM-IHP: Consensus I AM...
TM-IHP: Consensus I AM...
OnNewTasks(TM)-BGN-1796313429
OnNewTasks(TM)-BKS-1796313429:
NewTaskHashesOnTM(TM)-1796313429: 0x030f32bb25d352a5358669c87d68659b839f4c6e5248924bda5b04692e54d5f6
OnNewTasks(TM)-AKS-1796313429-count: 0
OnNewTasks(TM)-AGT-1796313429-count: 0
OnNewTasks(TM)-BGN-1931910902
OnNewTasks(TM)-BKS-1931910902:
NewTaskHashesOnTM(TM)-1931910902: 0x030f32bb25d352a5358669c87d68659b839f4c6e5248924bda5b04692e54d5f6
OnNewTasks(TM)-AKS-1931910902-count: 0
OnNewTasks(TM)-AGT-1931910902-count: 0
OnNewTasks(TM)-BGN-950499347
OnNewTasks(TM)-BKS-950499347:
NewTaskHashesOnTM(TM)-950499347: 0x030f32bb25d352a5358669c87d68659b839f4c6e5248924bda5b04692e54d5f6
OnNewTasks(TM)-AKS-950499347-count: 0
OnNewTasks(TM)-AGT-950499347-count: 0
[12:56:09.026] OnConsensusPayload hash=0x030f32bb25d352a5358669c87d68659b839f4c6e5248924bda5b04692e54d5f6
OnRelay(RN): 0x030f32bb25d352a5358669c87d68659b839f4c6e5248924bda5b04692e54d5f6
OnRelay(RN): 0x030f32bb25d352a5358669c87d68659b839f4c6e5248924bda5b04692e54d5f6
OnRelay(RN): 0x030f32bb25d352a5358669c87d68659b839f4c6e5248924bda5b04692e54d5f6
[12:56:09.031] OnChangeViewReceived: height=1 view=0 index=0 nv=1 hash=0x030f32bb25d352a5358669c87d68659b839f4c6e5248924bda5b04692e54d5f6I think that we are still missing a drop on protocol handler. |
There was a problem hiding this comment.
[13:29:16.554] OnConsensusPayload hash=0xb385ad6ddb6c91f6a5bfbeee1b376eb23cb2ef72cd95b75a1298033ecde68e47
[13:29:16.555] OnCommitReceived: height=96 view=0 index=0 nc=2 nf=1 hash=0xb385ad6ddb6c91f6a5bfbeee1b376eb23cb2ef72cd95b75a1298033ecde68e47
OnReceive-CP-OnInventoryReceived(PH) - II: 0x262a68def5be892b70bfc5220a31f7b2845e3873f9920077d73d86a9656b3643
OnInventoryReceived - Task Completed will be called (PH): 0x262a68def5be892b70bfc5220a31f7b2845e3873f9920077d73d86a9656b3643
OnInventoryReceived - Local Node Relay will be called (PH): 0x262a68def5be892b70bfc5220a31f7b2845e3873f9920077d73d86a9656b3643
OnReceive-CP-OnInventoryReceived(PH) - II: 0x859a3bf0f60930fd64f8af36dbf43adf24991690639b0c315feb5d5922475b42
OnInventoryReceived - Task Completed will be called (PH): 0x859a3bf0f60930fd64f8af36dbf43adf24991690639b0c315feb5d5922475b42
OnInventoryReceived - Local Node Relay will be called (PH): 0x859a3bf0f60930fd64f8af36dbf43adf24991690639b0c315feb5d5922475b42
OnReceive-CP-OnInventoryReceived(PH) - II: 0xb385ad6ddb6c91f6a5bfbeee1b376eb23cb2ef72cd95b75a1298033ecde68e47
OnInventoryReceived - Task Completed will be called (PH): 0xb385ad6ddb6c91f6a5bfbeee1b376eb23cb2ef72cd95b75a1298033ecde68e47
OnInventoryReceived - Local Node Relay will be called (PH): 0xb385ad6ddb6c91f6a5bfbeee1b376eb23cb2ef72cd95b75a1298033ecde68e47
OnRelay(LN): 0x262a68def5be892b70bfc5220a31f7b2845e3873f9920077d73d86a9656b3643
OnRelay(LN): 0x859a3bf0f60930fd64f8af36dbf43adf24991690639b0c315feb5d5922475b42
OnRelay(LN): 0xb385ad6ddb6c91f6a5bfbeee1b376eb23cb2ef72cd95b75a1298033ecde68e47
OnRelay(RN): 0xb385ad6ddb6c91f6a5bfbeee1b376eb23cb2ef72cd95b75a1298033ecde68e47
OnRelay(RN): 0xb385ad6ddb6c91f6a5bfbeee1b376eb23cb2ef72cd95b75a1298033ecde68e47
OnRelay(RN): 0xb385ad6ddb6c91f6a5bfbeee1b376eb23cb2ef72cd95b75a1298033ecde68e47
OnRelay(RN): 0xb385ad6ddb6c91f6a5bfbeee1b376eb23cb2ef72cd95b75a1298033ecde68e47
OnRelay(RN): 0xb385ad6ddb6c91f6a5bfbeee1b376eb23cb2ef72cd95b75a1298033ecde68e47
[13:29:16.562] relay block: height=96 hash=0x31b1cd7104a0a9f15e1a308fde6822dc5678f724552fbf9f862a980dd1177aba tx=1
[13:29:16.563] OnConsensusPayload hash=0x03a899c6ba7478a1d767a7b298f88bf6216b7be3e0b2d272ce528aee01e960f8
[13:29:16.566] OnConsensusPayload hash=0x262a68def5be892b70bfc5220a31f7b2845e3873f9920077d73d86a9656b3643
OnRelay(RN): 0x03a899c6ba7478a1d767a7b298f88bf6216b7be3e0b2d272ce528aee01e960f8
OnRelay(RN): 0x03a899c6ba7478a1d767a7b298f88bf6216b7be3e0b2d272ce528aee01e960f8
OnRelay(RN): 0x03a899c6ba7478a1d767a7b298f88bf6216b7be3e0b2d272ce528aee01e960f8
OnRelay(RN): 0x262a68def5be892b70bfc5220a31f7b2845e3873f9920077d73d86a9656b3643
OnRelay(RN): 0x03a899c6ba7478a1d767a7b298f88bf6216b7be3e0b2d272ce528aee01e960f8
[13:29:16.570] OnConsensusPayload hash=0x03a899c6ba7478a1d767a7b298f88bf6216b7be3e0b2d272ce528aee01e960f8
[13:29:16.572] OnConsensusPayload hash=0x262a68def5be892b70bfc5220a31f7b2845e3873f9920077d73d86a9656b3643
[13:29:16.572] OnConsensusPayload hash=0x859a3bf0f60930fd64f8af36dbf43adf24991690639b0c315feb5d5922475b42
OnRelay(RN): 0x262a68def5be892b70bfc5220a31f7b2845e3873f9920077d73d86a9656b3643
OnRelay(RN): 0x262a68def5be892b70bfc5220a31f7b2845e3873f9920077d73d86a9656b3643
OnRelay(RN): 0x03a899c6ba7478a1d767a7b298f88bf6216b7be3e0b2d272ce528aee01e960f8
OnRelay(RN): 0x262a68def5be892b70bfc5220a31f7b2845e3873f9920077d73d86a9656b3643
OnRelay(RN): 0x262a68def5be892b70bfc5220a31f7b2845e3873f9920077d73d86a9656b3643
[13:29:16.573] OnConsensusPayload hash=0xb385ad6ddb6c91f6a5bfbeee1b376eb23cb2ef72cd95b75a1298033ecde68e47- Consensus nodes are still processing a
ConsensusPayloadtwice. - There is still a leak or on
TaskManageror atProtocolHandler, which is allowing tasks to be processed twice. - In particular,
ProtocolHandleris still processing severalOnInvMessageReceived
In this sense, I am not in favor of the the merge of this fix until we really find how to remove the source of all these duplicated packages being processed.
@yongjiema, let's try to find this all together.
You already triggered us to investigate this. Thus, let's move on.
|
I am also thinking about renaming the issue to be more precise on what we are looking for. |
vncoelho
changed the title
|
@yongjiema, @igormcoelho, @erikzhang, @shargon, maybe we can merge this. The first change is:
OR
(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)The first condition:
Could you create another PR with just this change of line 230 and then we try to find, completely, how to avoid duplicated tasks, invpayload and, consequently, single processing of ConsesusPayloads (by hash)? I believe that there is no problem in merging this first part for both 2.x and 3.x. |
|
Please, take a look at this log and search for hash: This was obtained with only 170 blocks of 20 seconds in a privatenet with 5 nodes (4CN + 1 RPC). I do not know why OnReceive-CP-OnInventoryReceived(PH) - II: 0x55673eb0cf0c939ee8e1e588b56d2429a7f1a6c6db682e4c41c9d6ce98b279e9
printingPayloadHashes(PH)-2081283276: 0x4911277fca18e38445d760edb7980725a66d03ae5033476c06e1ca7f8d8809d9
printingPayloadHashes(PH)-1610240410: 0x97389c43ad730b6fa0868f86b3c600d17c68282c27b3d6c845557d003744cc75
OnInventoryReceived - Task Completed will be called (PH): 0x55673eb0cf0c939ee8e1e588b56d2429a7f1a6c6db682e4c41c9d6ce98b279e9And then we still keep processing different Maybe the logs are out of order, but I do not think so, I also included random number for helping us in this hunt. I believe that you solved the TaskManager @yongjiema, maybe we just need to kind of replicate this idea on |
|
@shargon, private void OnInvMessageReceived(InvPayload payload)
{
UInt256[] hashes = payload.Hashes.Where(p => knownHashes.Add(p)).ToArray();
if (hashes.Length == 0) return;This should be enough for do not processing an |
|
@shargon, we do not have the |
|
I am not sure if was tested with the last patch, maybe only without the knownhash patch, anyway, this PR is good. |
|
@vncoelho Hashes are possible duplicated at |
vncoelho
changed the title
There was a problem hiding this comment.
I modified the PR to just include the improvement on tasks requests.
During this week I will keep analyzing the other part of the PR in order that we completely solve these duplicated tasks.
In this sense, could you open another PR with this part of the code:
using System.Collections;
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;
return false;
}
|
@erikzhang, this is ready to merge. The other one I still want to investigate more regarding duplicated tasks. |
lock9
dismissed
their stale review
The rule is that reviewers must test before approving a PR. This PR does not have UT and cannot be tested locally, however, I won't stop other people from approving it.
* Add some questions * Fixes (neo-project#900) * update English files and Chinese files * update
Fix #841