Investigating mempool behavior with similar transactions

[vncoelho]

Summary
As previously reported by @belane and @shargon, currently, it was wondered if someone could possibly spam different speakers with similar txs, which would make backups reject such block and deteriorate performance, among others...

@belane and @shargon, could you please double check. I am not sure if this problem actually exists, because OnTransaction event of ConsensusService looks independent from the OnNewTransaction (which will probably discard the transaction here public virtual bool Reverify(Snapshot snapshot, IEnumerable<Transaction> mempool)). What do you think, @erikzhang?

Do you have any solution you want to propose?
Most would happen due to a transaction becoming invalid in terms of network and system fees.
In this sense, we need to track senders and their GAS. By doing that we could still accept transactions proposed by `Speaker.

This feature could be implemented together with the possible PR of #1140.

Where in the software does this update applies to?

• Consensus
• Ledger
• P2P (TCP)

[vncoelho]

Perhaps it will fail on Consensus OnTransaction event because of

neo/neo/Network/P2P/Payloads/Transaction.cs

Line 142 in 2877178

fee += mempool.Where(p => p != this && p.Sender.Equals(Sender)).Select(p => (BigInteger)(p.SystemFee + p.NetworkFee)).Sum();

Consensus Service AddTransaction calls if (verify && !tx.Verify(context.Snapshot, context.Transactions.Values)), which calls Reverify.

In this sense, perhaps we need a better strategy for the CN to temporally accept that TX only for Consensus purposes, not adding it to its own mempool, but trying to verify the proposed block.
Consequently, if the block is persisted its mempool will be clear for that Sender.

Do you agree with the description, @shargon @belane?
If so, maybe the proposed solution may work.

[vncoelho]

In fact, I think not!

I just noticed that Consensus Service simulates the mempool in its call of Verify.

[shargon]

I need more time to review this, sorry @vncoelho

[vncoelho]

@shargon, this is not critical, it is mostly a discussion.

I my opinion, the problem does not occurs and we are resilient enough with the current design of CN transaction checking.

However, let's make it clear with code comments and variable renaming. It is not good that we do not clearly see that.