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

Advice for preventing consensus node from Garbage transactions #478

Closed
superboyiii opened this issue Nov 27, 2018 · 9 comments · Fixed by #500
Closed

Advice for preventing consensus node from Garbage transactions #478

superboyiii opened this issue Nov 27, 2018 · 9 comments · Fixed by #500
Labels
Discussion Initial issue state - proposed but not yet accepted

Comments

@superboyiii
Copy link
Member

As I see, I find there's sorting mechanism in consensus nodes' mempool for transactions order by network fee but it doesn't work effectively when the mainnet is crowded by massive free transactions. I think it's
mostly because of that every ordinary node has no mechanism for sorting transaction by network fee in it's local when transactions are crowded in TCP/IP piping. Every transaction now is only received, verified, relayed by nodes to their peers, finally sent to consenus nodes' mempool. So if the transctions could be sorted before relayed, block stagnation could be solved.

@jsolman
Copy link
Contributor

jsolman commented Nov 27, 2018

They actually are sorted after each block is persisted and added back to the chain in the order by fee per byte:

https://github.com/neo-project/neo/blob/master/neo/Ledger/Blockchain.cs#L390-L397

@erikzhang erikzhang added the Discussion Initial issue state - proposed but not yet accepted label Nov 27, 2018
@Li-Tian
Copy link

Li-Tian commented Nov 28, 2018

Superboyiii refers to the fact that although transactions are prioritized when they are filled into a block, they are not prioritized when they are transmitted through the network. Therefore, when the network is extremely busy, the transaction with transaction fee cannot reach the consensus node preferentially. The improved solution is that transactions are sorted according to network fees when they are relayed. Although this improvement does not bring any benefits when the network is idle, it can show changes when the network is extremely busy.

@jsolman
Copy link
Contributor

jsolman commented Nov 29, 2018

Wouldn’t you have to batch incoming transactions and delay relaying them slightly in order to prioritize them. How would that work exactly?

Maybe there should be a pool before the mempool where transactions are sorted by fee and have a lesser lightweight verification done that doesn’t require fully reverifying the tx on each block so that transactions that won’t make it into the next block don’t get pointlessly reverified wasting resources on node that could be used to do better sorting before relaying new transactions.

@Li-Tian
Copy link

Li-Tian commented Nov 29, 2018

The mem-pool is a Dictionary<UInt256, Transaction>. It does not hold sorted information. Everytime it is sorted, the transactions over 50,000 records will be discarded. After that all transactions are still disordered.
When a transaction is received, it will be relayed immediately, no matter the importance of the transaction.
The suggested solution is to put the received transactions into a queue. Once a new transaction comes, it will be added to the queue and inserted to the correct place according to its network fee.
And there will be a relaying worker thread, which pull the first transaction in the queue and relay it.
The implementation works slightly slower than current implementation when network is idle.
But when network is extremely busy, the transactions with higher fee will be relayed and reach consensus node before other transactions.
The verification job can be done at the point a transaction is pulled out of the queue before they are relayed. The transactions with less fee can wait in the queue without being verified.

@jsolman
Copy link
Contributor

jsolman commented Nov 29, 2018

I thought about changing the mempool data structure to be a sorted structure (a balanced tree would likely be better); not sure a queue is the best though. I haven’t had time to work on such changes, but it is one of the things that should be done.

@jsolman
Copy link
Contributor

jsolman commented Nov 29, 2018

Also, as it is all transactions that make it into the mempool are verified; I’m pretty sure other parts of the code need anything that make it into the mempool to be verified; seems like a pool before the mempool that uses a SortedSet/SortedDictionary could possibly work.

@Li-Tian
Copy link

Li-Tian commented Nov 29, 2018

A sorted set would be faster than a queue when adding new transaction. I use a queue to describe the idea. Does not have to be a queue.

@jsolman
Copy link
Contributor

jsolman commented Nov 30, 2018

If I get time I will work on rewriting the way the mempool works to reduce load and be able to improve how transactions propagate.

@jsolman
Copy link
Contributor

jsolman commented Dec 3, 2018

While the transaction messages in the Blockchain actor queue that get added back after a block is persisted are in order by fee, there is a problem because the actor's message queue only has high and low priorities and is not a true priority queue that is considering the priority of the transactions. This means that new transactions that get added will end up getting processed at the end instead of getting processed based on the priority they should have. If we switch the PriorityMailbox being used for Blockchain messages to something that would order transaction messages in the priority mailbox by fee per byte then when new messages get added they would process ahead of the lower priority ones that were re-added after a block was persisted and the inversion of priority problem that @superboyiii is hinting at would be solved. One other way to solve it besides changing the way the Blockchain actor's messages are sorted would be to delegate to the MemoryPool to handle verifying the transactions in a background task (though it might be a slight break from the Akka model).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Initial issue state - proposed but not yet accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants