Remove the case of GetData in ProtocolHandlerMailbox#ShallDrop in 2.x

[doubiliu]

port to 2.x
Remove the case of GetData in ProtocolHandlerMailbox#ShallDrop has merged in 3.0

[vncoelho]

I am not 100% confident of merging this PR to master2x.

Check my comment: #1201 (comment)

I would be more certain if the ShalDrop was just for the ConsensusGetData type and not all GetData. That just would require was to create this new command.

[Tommo-L]

I would be more certain if the ShallDrop was just for the ConsensusGetData type and not all GetData.

It's true, but RestartTasks appears less frequently under normal circumstances.

[shargon]

I am not 100% confident of merging this PR to master2x.

What is the difference with neo3?

[lock9]

These are the same changes applied on #1201.
@belane Do we need to test this again for neo 2? By reading @vncoelho comments, it seems that this change can be exploited by a malicious user.

[vncoelho]

What is the difference with neo3?

Neo3 is under development, @shargon, we can still have time to test it more.
I believe we need this only for consensus GetData and not all GetData.
Do you agree, @Tommo-L? If not, let me know and I can think more about it.

[Tommo-L]

I believe we need this only for consensus GetData and not all GetData.

I agree.
It's ok for me to create a GetConsensusData type.

[vncoelho]

Hi @Tommo-L, how is it going? Should we create this type and merge this? Or better to wait for a stable NEO3-preview for testing it there?

[Tommo-L]

😂 We haven't added this type, we hope that Neo2.x remains as functionally unchanged as possible. The introduction of new types will affect consensus modules.

[vncoelho]

AHueahuea, then we need to decide if we merge this, wait for NEO3x preview to see results or close...

[Tommo-L]

We can wait for the result of Neo3 preview, considering that the next stable version of 2.x will take some days

[cloud8little]

Test result of this PR: Fail
Scenario One: Passed
send 48225 valid request, include fee added, and free nep5 transaction in 50 minutes. when transactions are all processed via consensus node, checked all txs are on-chain.

Scenario Two: Failed
Network Topology

It found that not all transactions are on-chain when pool is 0 in the end. the loss tx rate is about 10%.
It needs further investigation.
nep5contract.zip

[doubiliu]

@cloud8little
As we discussed, in the default configuration, the upper limit of the memory pool is 50,000 transactions, so if the test pressure is greater than the upper limit, it is normal to lose transactions.In addition, another cause of transaction loss is that the 2.0 version is UTXO model, and paid transactions will be limited.Therefore, the ideal test method is to test only free transactions. If the transactions are not lost in a large amount below the upper limit of the memory pool, it is feasible.

[cloud8little]

@doubiliu thanks very much for the detailed explanations. Here are the test result.
Network topology is the same as above. while the total number of transactions is different.

1. free fee + pay fee transaction; number of transactions > 50000; tx loss, all loss txs are pay fee transaction.
2. All free fee transaction; number of transactions = 47000+ < 50000; no tx loss.
3. 2/3 free fee + 1/3 pay fee transaction; number of transactions = 18000 in 10minutes < 50000; about 5% tx loss.
   So I thought this PR aims to improve the TPS, instead of solving tx loss problem.

[vncoelho]

Nice discussions.

@cloud8little, where there gains in performance?

@doubiliu, can you check this comment again, please? #1201 (comment)

I am still not sure because this could be a path to attacks.
What do you think, @shargon? Not too much real risks? I saw that you already approved.

[shargon]

If it's an non required optimization, and it could produce some errors, I prefer to not merge this.

[Tommo-L]

We'll close this PR, as we don't want to add non-repair features to 2.x.