Drop the duplicate NewTasks for the TaskManagerMailbox (2x)

[yongjiema]

No description provided.

[lock9]

Hello,
We need to test in order to approve issues (from now on), and in your case, I didn't see any UT to test it.
@shargon how did you test this?

[vncoelho]

Great @yongjiema, I will work on this PR for us for trying to make all duplication to be finished.

We need this for protocol handler also, I think.

Lets make the title of the PR more generic.

[shargon]

It was tested in the previous Pull request, was splitted...

[lock9]

@shargon I will remove my request for changes, but I need to understand better the rules we have for the reviewers. I thought we had to test every PR before approving it, but without UT, there is no way to test it (except some cases where we use neo-cli).

I won't approve, but I won't stop other people from approving it.

[vncoelho]

No @shargon, look all my comments (ahauahah, I know it was too much). This did not solve the duplicated problem, check those logs.

I am still not in favor of this merge, in particular, after all experiments that were carried on.

I am going to focus on this for 3-5 more days, I agree with this merge when we really detect a difference in duplicated tasks. My experiments did not show that.

[vncoelho]

@yongjiema, I am opening other minor fix in another PR.

Give me 2-4 more days because I just want to be 99% sure that this is a needed fix.

[vncoelho]

@erikzhang,

who calls OnRelay of RemoteNode.cs (It is not clear to me who triggers it)?

        private void OnRelay(IInventory inventory)
        {
            if (Version?.Relay != true) return;
            if (inventory.InventoryType == InventoryType.TX)
            {
                if (bloom_filter != null && !bloom_filter.Test((Transaction)inventory))
                    return;
            }
            if (inventory.InventoryType == InventoryType.Consensus)
            {
                Console.WriteLine($"OnRelay(RN): {inventory.Hash}");
            }
            EnqueueMessage("inv", InvPayload.Create(inventory.InventoryType, inventory.Hash));
        }

Edited:

I think that I found.
I saw this only at LocalNode:

        private void OnRelayDirectly(IInventory inventory)
        {
            Connections.Tell(new RemoteNode.Relay { Inventory = inventory });
        }

Which is called here:

private RelayResultReason OnNewConsensus(ConsensusPayload payload)

Which was triggered from LocalNode OnRelay

        private void OnRelay(IInventory inventory)
        {
            if (inventory is Transaction transaction)
                system.Consensus?.Tell(transaction);

            if (inventory is ConsensusPayload)
                Console.WriteLine($"OnRelay(LN): {inventory.Hash}");
            
            system.Blockchain.Tell(inventory);
        }

[vncoelho]

@shargon, @igormcoelho, @erikzhang, @yongjiema,

I think another source of the problem has been found.

        internal readonly RelayCache RelayCache = new RelayCache(100);

This cached is quite small and might be causing the request of the same consensus multiple times.

[vncoelho]

In fact, this RelayDirect is just rebroadcasting to the Connections of the network, right?

[vncoelho]

TM-ShallDrop: Droping message Neo.Network.P2P.TaskManager+NewTasks Block Neo.UInt256[]
TM-ShallDrop: Droping message 0xb1654fb940d2da5702bf780213bfc3e38baf82e6466a4b808f1354c8e88c0d50
OnRelay(RN): 0x26eeb2582a272f4f117821ed489bda38debceef3a9993ce07c108eb67adc7548
OnRelay(RN): 0x48872dc58d5875f2ed93af2f4ab218f1f5c8087faee4b0bbfc8e9009dec0afd1
OnRelay(RN): 0x26eeb2582a272f4f117821ed489bda38debceef3a9993ce07c108eb67adc7548
OnRelay(RN): 0x48872dc58d5875f2ed93af2f4ab218f1f5c8087faee4b0bbfc8e9009dec0afd1
OnRelay(RN): 0x26eeb2582a272f4f117821ed489bda38debceef3a9993ce07c108eb67adc7548
OnRelay(RN): 0x48872dc58d5875f2ed93af2f4ab218f1f5c8087faee4b0bbfc8e9009dec0afd1
OnRelay(RN): 0x26eeb2582a272f4f117821ed489bda38debceef3a9993ce07c108eb67adc7548
OnRelay(RN): 0x48872dc58d5875f2ed93af2f4ab218f1f5c8087faee4b0bbfc8e9009dec0afd1
[23:59:55.721] relay block: height=6 hash=0xb1654fb940d2da5702bf780213bfc3e38baf82e6466a4b808f1354c8e88c0d50 tx=1

It is working also for blocks

[vncoelho]

and it is also working nice for consensus

OnInvMessageReceived(PH): hashes=1 sentByMe=False
OnInvMessageReceived(PH):
printingPayloadHashes(PH)-1906654075-1565309258: 0xfc24939bc65e9d8e0103d9614f9d399d236e67ddca6830cc3af2bc7558ca9292
printingHashesFiltered(PH)-1906654075-1565309258: 0xfc24939bc65e9d8e0103d9614f9d399d236e67ddca6830cc3af2bc7558ca9292
TM-IHP: Consensus I AM...
OnInReceive-OnInvMessageReceived(PH):
OnInvMessageReceived(PH): hashes=1 sentByMe=False
OnInvMessageReceived(PH):
printingPayloadHashes(PH)-1633004902-1565309258: 0x2e3376034c8d04c58e168c919c69c0b0f337da3109c87a6ad586dddba3979cfa
printingHashesFiltered(PH)-1633004902-1565309258: 0x2e3376034c8d04c58e168c919c69c0b0f337da3109c87a6ad586dddba3979cfa
TM-IHP: Consensus I AM...
OnInReceive-OnInvMessageReceived(PH):
OnInvMessageReceived(PH): hashes=1 sentByMe=False
OnInvMessageReceived(PH):
printingPayloadHashes(PH)-811169209-1565309258: 0xfc24939bc65e9d8e0103d9614f9d399d236e67ddca6830cc3af2bc7558ca9292
printingHashesFiltered(PH)-811169209-1565309258: 0xfc24939bc65e9d8e0103d9614f9d399d236e67ddca6830cc3af2bc7558ca9292
TM-ShallDrop: Droping message Neo.Network.P2P.TaskManager+NewTasks Consensus Neo.UInt256[]
TM-ShallDrop: Droping message 0xfc24939bc65e9d8e0103d9614f9d399d236e67ddca6830cc3af2bc7558ca9292

[vncoelho]

OnInvMessageReceived(PH):
      printingPayloadHashes(PH)-1881779158-1565309319: 0xb080a4fe6c2db5308ab63f8423a795bb30082e094f3938e4aad936522b05794d
      printingHashesFiltered(PH)-1881779158-1565309319: 0xb080a4fe6c2db5308ab63f8423a795bb30082e094f3938e4aad936522b05794d
TM-IHP: Consensus I AM...
      printingHashesFiltered(PH)-1488921154-1565309319: 0xc144f4eeb1e27890e1c047d77d76f5646990531f7f91fa1c597d9ce811d3b11d
TM-ShallDrop: Droping message Neo.Network.P2P.TaskManager+NewTasks Consensus Neo.UInt256[]
TM-ShallDrop: Droping message 0xc144f4eeb1e27890e1c047d77d76f5646990531f7f91fa1c597d9ce811d3b11d
OnInReceive-OnInvMessageReceived(PH):
OnInvMessageReceived(PH): hashes=0 sentByMe=False
OnInReceive-OnInvMessageReceived(PH):
OnInvMessageReceived(PH): hashes=1 sentByMe=False
OnInvMessageReceived(PH):
      printingPayloadHashes(PH)-2070483538-1565309319: 0xb080a4fe6c2db5308ab63f8423a795bb30082e094f3938e4aad936522b05794d
OnInReceive-OnInvMessageReceived(PH):
OnInvMessageReceived(PH): hashes=1 sentByMe=False
OnInvMessageReceived(PH):
      printingPayloadHashes(PH)-648271233-1565309319: 0xb080a4fe6c2db5308ab63f8423a795bb30082e094f3938e4aad936522b05794d
      printingHashesFiltered(PH)-648271233-1565309319: 0xb080a4fe6c2db5308ab63f8423a795bb30082e094f3938e4aad936522b05794d
TM-ShallDrop: Droping message Neo.Network.P2P.TaskManager+NewTasks Consensus Neo.UInt256[]
TM-ShallDrop: Droping message 0xb080a4fe6c2db5308ab63f8423a795bb30082e094f3938e4aad936522b05794d
      printingHashesFiltered(PH)-2070483538-1565309319: 0xb080a4fe6c2db5308ab63f8423a795bb30082e094f3938e4aad936522b05794d
TM-ShallDrop: Droping message Neo.Network.P2P.TaskManager+NewTasks Consensus Neo.UInt256[]
TM-ShallDrop: Droping message 0xb080a4fe6c2db5308ab63f8423a795bb30082e094f3938e4aad936522b05794d

In the log printingPayloadHashes(PH)-1881779158-1565309319 the second number is a timestamp.

As can be seen all OnInvMessageReceived arrives at the same ms time in a localprivatenet.

The mechanism is working nice because it cuts from the queue of newtasks.

@yongjiema, do you think that a similar PR could be done for taskscompleted or for the OnInventoryReceived of ProtocolHandler?

[vncoelho]

Approved, @yongjiema.

Great job and sorry for the delay. I was testing and debugging each step.
I am confident that this is surely helping the network, in particular, in privatenet, because these removal is happening on the Akka queue level.

However, something similar should probably be done on ProtocolHandler for dealing with OnInventoryReceived of type "consensus".

This PR aligned with #1010 will surely bring some benefits and remove natural overload and duplicated execution of payloads.

Thanks for the insight and finding @yongjiema, this triggered several discussions and investigations.

Again, I apologize for all the delay and confusion.

[vncoelho]

@yongjiema, I am confident that this will also be interesting for master3x.