Neo: Mempool Reverification Model (MRM): Optimizing our nodes

[vncoelho]

Summary
Currently, mempool reverification model for NEO3 is not completely finished.

Imagine the scenario that you have 5 GAS in your address:

• You send 5 valid txs (tx_1, ..., tx_2) that uses 1 GAS each (sysfees + netfees)
• all 5 txs enter in the mempool
• tx_1 enter in the next block

Current implementation of NEO3:

1. all 4 remaining txs are revalidated and kept on the mempool

This may lead nodes to keep, in their mempool, txs that do not have GAS to be anymore spent. Thus, txs that can not pay its sysfee and netfees.

Edited:
This description has been updated since currently we move all txs to unverified and slowly reverify in batches.

Possibilities:

1. tx_ 1 uses 4 GAS in its Application (transferring to other address), thus all 4 remaining txs shall be discarded
2. tx_ 1 uses 3 GAS in its Application (transferring to other address), thus, 1 out of 4 of the remaining txs shall be discarded

Do you have any solution you want to propose?
With @igormcoelho, we were addressing this design https://github.com/neo-project/specification/pull/3 some months ago.

• add a new field to payable transactions, in which it says the limit of GAS it can spend on Application
• with such field, we would design a mechanism that tracks GAS pre-balance of senders in the current mempool

Where in the software does this update applies to?

• MemPool
• Ledger
• Transaction

[lock9]

Hi @vncoelho, I think we should wait for at least 2 approvals before adding something to the neo 3 milestone. If everyone start adding or removing issues to Neo 3 milestone we are surely going into conflicts.
I will leave this added to the milestone, but next time please wait for more approvals.

[vncoelho]

I see, @lock9, makes sense. I added this one because the current implementation for mempool is not completed. Thus, in any case, we need to pass though this.

[vncoelho]

@igormcoelho, how are the current possibilities for an invocation to spend GAS of other address, if the Witness is passed as a parameter of the call?

@lock9, this is not really related to network-policy. It is a cached mechanism for mempool to reverify transactions after block persistence (ledger) and also a new field to transactions, which would guide the limit of GAS to be spent.

[lock9]

@vncoelho this is related to how the network will process the transactions, right? For me, this is a network policy issue.
Note that the network policy does not relate only to the native network contract, it also refers to the way node process transactions, how we prioritize and revalidate them in the memory pool seems to be a network policy issue for me

[vncoelho]

Makes sense, @lock9, however, just be aware that the logic does not change. It just would just add new field to payable txs themselves.

[lock9]

Sorry, @vncoelho I had forgotten to add the ledger tag too.
Do you think it is an enhancement or a new feature?

[vncoelho]

I think it is a basic functionality, but it is a feature because it would need a new field on tx....ahauaha

The current mempool reverification is not yet fully completed, thus, it is already expected to be part of the development.

[lock9]

Maybe it only relates to the ledger, if this is the default way and doesn't change

[vncoelho]

The only point would be an extra field of allowance of GAS, which is really a negative point.

However, we still did not see another way to quick revalidate.
In long term, this might also be good for users, which could safely sign different txs in parallel if limits are respected.

Currently, we can do that, but it costs more for our revalidation, which is a critical thing, because all seed nodes do that all the time.
In addition, this is a possibility for us that have block dBFT and block finality. Other paragdims would not have such lucky.

Considering that, in favor of scalability, I keep this as a priority in terms of design.

We can keep compatibility by saying that null allows all gas, such as nowadays. But, then, we would lock sender to multiple sign.

Currently, we have open possibilities for consensus node attacks, as @belane & @shargon once sent a report.
Maybe nodes could accept the tx without checking the limit, but deleting them after block persistance.
However, currently, the attack is still possible with a sender varying txs and fees. (I will open another issue for that)

[shargon]

Completely necessary

[vncoelho]

@shargon, I am going to start a draft of this PR as soon as possible.
However, if you fell that you are with the design in your mind, fell free to open it when you can.

[vncoelho]

@shargon @igormcoelho, should the field on the transaction be an independent one or an TransactionAttribute ?

[vncoelho]

@erikzhang, I started the implementation as a new field, such as Sender, NetworkFee, SystemFee, etc...
The field was:

        /// <summary>
        /// Max allowed amount of GAS that the transaction is allowing to be transfered from Sender
        /// If null, not passed, no parallel transactions will be accepted on the mempool since all remaining GAS would be allowed
        /// </summary>
        public long MaxTransferableGAS;

However, as a field like this I am finding it hard to make it optional.
The idea of being optional is good because it allows compatibility with already implemented wallets, that would happen since they would not need to add an extra field if they do not want.

• However, if they do so, during Transaction Deserialization we would set MaxTransferableGAS = Sender_Balance - (SystemFee + NetworkFee). By doing so, the Sender parallel transaction will be rejected by the mempool.

In such case:

• Just those interesting in sending parallel transaction of a Sender would need to care about that field.

Maybe the best way is like a TransactionAttribute.

Edited:
I think that Cosigners can be empty, right? Thus, maybe we can just follow its template.

[erikzhang]

I'm not sure it is needed.

[vncoelho]

It would be good if we could manage without this additional field.

What is your idea, @erikzhang? How could we quickly Reverify the mempool without waiting for all transactions to be persisted and balances updated?
Currently, we rely on ReverifyTransactions(_sortedTransactions, _unverifiedSortedTransactions, _maxTxPerBlock, MaxMillisecondsToReverifyTx, snapshot);, which keep doing that slowly.
I believe this is not the perfect solution for scalability. However, I agree that it is working one.

@shargon, my description of the issue was not fully correct, in fact, as @erikzhang says, it currently work, because we move all txs to unverified and, then, reverify then in batches.
However, this is surely a bottleneck for scalability.

PS: However, this additional field will be optional, as described. Just for those that want the Sender to send multiple transactions.

[vncoelho]

Another pending question is:

@igormcoelho @shargon @erikzhang, will it be possible for a transaction to transfer GAS of another address (not only the Sender) ? Let`s say that a cosigner or a witness is attached to the transaction?
If that is the case we are going to reach an even more complex scenario.

[vncoelho]

@erikzhang, I just talked to @igormcoelho and he explained me that currently only Sender can spend GAS during an execution of a transaction.

In this sense, this proposed solution is quite suitable and should be implemented, it looks like to be a good path for scalability.

Since it is only the Sender that may spend GAS during the Application the solution will not be so complex and the gains are honorable. Imagine a mempool with 50k,30k or more txs?

[erikzhang]

I think a cosigner can send GAS also.

[vncoelho]

@erikzhang, perhaps send for systemfee and netfee, right? Igor mentioned that not spent cosigners GAS during Application.

[doubiliu]

@vncoelho I think "payable" attribute is very useful attribute. Currently we move all txs to unverified and slowly reverify after block persisted.We @eryeer have done a test that this process takes up 40% of the entire block persist process.If we add "payable" attribute,we will not reverify txs,this will save a lot of time. Tps will increase significantly.

[vncoelho]

It is great to hear that from you, @doubiliu, and @eryeer, as well as that you are already investigating that.
Nice job.

After we merge #1183 it might be good time to push this one.

@doubiliu, I believe that we can do the "payable" attribute as optional, thus, light wallets/clients implementation do not need necessarily to change.

When the "payable" is null we can block all parallel transactions of that sender, since he may spend all GAS during execution.
On the other hand, for those interested in using that flag "payable" and define a limitExecutionGas, mempool would accept parallel transactions until balance is reached.

[vncoelho]

@doubiliu @Qiao-Jin @eryeer, can you help implementing this field "payable" (limitExecutionGas)?
Otherwise, I may try to start the PR. But it looks like I am not the best one to do it...aheuahuea

[eryeer]

@vncoelho I think we can discuss the detail implementation scheme before implement this. Maybe there are some detail issue we need to take care of, such as how to solve cosigner's transfer.

[vncoelho]

@eryeer, last time I talked to @igormcoelho he mentioned that cosigners were not allowed to spend GAS during Application.
Maybe let's double check.

[eryeer]

Is there such a constraint? I'm not quite sure about this, where is this GAS spending limitation?
In addition, after our tests, Reverify is a lightweight call. Even under high-pressure tests, Reverify of transactions after block persistence is not very time-consuming. 😂

Average tx reverify time: 1.8e-5s
When TPS is 2400, reverify tx amount is 44600, reverify time is 0.8s