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

Avoid processing own messages (2x) #1010

Merged

Conversation

vncoelho
Copy link
Member

@vncoelho vncoelho commented Aug 8, 2019

Replication of #1009 on master2x.

It can be seen as a minor bug fix or an improvement. Thus, fell free to close.
But this may help us in the hunt for avoiding duplicated tasks processing and probably, consequently, solving P2P minor problems that are bordering our nodes.

@vncoelho vncoelho requested a review from erikzhang August 8, 2019 20:44
@vncoelho
Copy link
Member Author

vncoelho commented Aug 8, 2019

OnInvMessageReceived(PH):
      printingPayloadHashes(PH)-1978586386-1565297075: 0x9090abff17aa3b88d2fd35d51f514e68395b30657d6590acc91fa08d9ace5c04
      printingHashesFiltered(PH)-1978586386-1565297075: 0x9090abff17aa3b88d2fd35d51f514e68395b30657d6590acc91fa08d9ace5c04
TM-IHP: Consensus I AM...
OnInvMessageReceived (PH): hashes=0 sentByMe=False
OnNewTasks(TM)-BGN-842930168-1565297075
OnNewTasks(TM)-BKS-842930168-1565297075:
       NewTaskHashesOnTM(TM)-842930168-1565297075: 0x9090abff17aa3b88d2fd35d51f514e68395b30657d6590acc91fa08d9ace5c04
OnNewTasks(TM)-AKS-842930168-1565297075-count: 0
OnNewTasks(TM)-AGT-842930168-1565297075-count: 0
OnInvMessageReceived (PH): hashes=1 sentByMe=False
OnInvMessageReceived(PH):
      printingPayloadHashes(PH)-1053130781-1565297075: 0x75bb0c087793e08aa85258851cc70f722455801888ffbf418ba94a6af2515bb7
      printingHashesFiltered(PH)-1053130781-1565297075: 0x75bb0c087793e08aa85258851cc70f722455801888ffbf418ba94a6af2515bb7
TM-IHP: Consensus I AM...
OnNewTasks(TM)-BGN-1303385166-1565297075
OnNewTasks(TM)-BKS-1303385166-1565297075:
       NewTaskHashesOnTM(TM)-1303385166-1565297075: 0x75bb0c087793e08aa85258851cc70f722455801888ffbf418ba94a6af2515bb7
OnNewTasks(TM)-AKS-1303385166-1565297075-count: 0
OnNewTasks(TM)-AGT-1303385166-1565297075-count: 0
OnInvMessageReceived (PH): hashes=0 sentByMe=False
OnInvMessageReceived (PH): hashes=1 sentByMe=True
OnInvMessageReceived (PH): hashes=1 sentByMe=True
OnInvMessageReceived (PH): hashes=0 sentByMe=True
OnInvMessageReceived (PH): hashes=0 sentByMe=True
OnInvMessageReceived (PH): hashes=1 sentByMe=False
OnInvMessageReceived (PH): hashes=1 sentByMe=False
OnInvMessageReceived(PH):
      printingPayloadHashes(PH)-1357786701-1565297094: 0x6254eacb6733db25e01ef599b1b5cbc03745337e4000f29b49385dc2deff2b30
      printingHashesFiltered(PH)-1357786701-1565297094: 0x6254eacb6733db25e01ef599b1b5cbc03745337e4000f29b49385dc2deff2b30

Check how many messages were discarded in a simple 10min example with 4CN + 1 RPC.

@vncoelho
Copy link
Member Author

vncoelho commented Aug 8, 2019

@erikzhang, please double check, just to be sure that nothing wrong is being done.

senthashes is updated upon a receive of getdata

        private void OnGetDataMessageReceived(InvPayload payload)
        {
            UInt256[] hashes = payload.Hashes.Where(p => sentHashes.Add(p)).ToArray();
            (....)
            Context.Parent.Tell(Message.Create("consensus", inventory));

The Parent of the ProtocolHandler actor will receive a "consensus" as response, right?
Thus, the own node should not receive this message and, consequently, call OnInventoryReceived

@shargon
Copy link
Member

shargon commented Aug 8, 2019

maybe we can add the sentHashes into knownHashes

@vncoelho
Copy link
Member Author

vncoelho commented Aug 8, 2019

Could be another option, @shargon, sounds good as well but makes us duplicate the local storage in the local memory.

@vncoelho vncoelho requested a review from shargon August 8, 2019 21:20
@vncoelho
Copy link
Member Author

vncoelho commented Aug 8, 2019

This will surely save many system.TaskManager.Tell(new TaskManager.NewTasks { Payload = InvPayload.Create(payload.Type, hashes) }, Context.Parent); of consensus!

@vncoelho
Copy link
Member Author

vncoelho commented Aug 8, 2019

@yongjiema, I am close to accept...aehauheuaea
Some more experiments.

@vncoelho vncoelho requested a review from shargon August 8, 2019 23:29
@shargon
Copy link
Member

shargon commented Aug 9, 2019

I will test the behavior, give me one day

@@ -253,7 +253,7 @@ private void OnInventoryReceived(IInventory inventory)

private void OnInvMessageReceived(InvPayload payload)
{
UInt256[] hashes = payload.Hashes.Where(p => knownHashes.Add(p)).ToArray();
UInt256[] hashes = payload.Hashes.Where(p => knownHashes.Add(p) && !sentHashes.Contains(p)).ToArray();
Copy link
Contributor

@igormcoelho igormcoelho Aug 9, 2019

Choose a reason for hiding this comment

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

This will naturally add to knownHashes, as Add will run before sentHashes is verified... interesting.
If this is not intended, we need to reverse this && here...

Copy link
Member Author

Choose a reason for hiding this comment

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

The intend is to verify this:

            bool sentByMe = payload.Hashes.Any(p => sentHashes.Contains(p));
            if (hashes.Length == 0 || sentByMe) return;

Check if there is Any of this hashes in the SentHashes.

Is this the same as it is? Shargon suggested this embed in a single line.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

And where is p => knownHashes.Add(p) part? hahahah

Copy link
Member Author

Choose a reason for hiding this comment

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

IO caching Fifoset

Copy link
Contributor

@igormcoelho igormcoelho left a comment

Choose a reason for hiding this comment

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

Well done!

@shargon shargon merged commit 3499643 into master-2.x Aug 10, 2019
@shargon shargon deleted the fixing-duplicated-processing-of-messages-sent-by-node branch August 10, 2019 19:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants