Improve Large MemoryPool Performance - Sort + intelligent TX reverification

[jsolman]

Update: Fairly extensive testing has been performed on this PR and it is now considered stable and ready for merging.

Background
When a large amount of transactions were broadcast to the chain, the overhead in the current design would cause performance issues. Also, new transactions with high fees, could be delayed. In the existing code base, after each block, transactions were removed and re-sent to the Blockchain mailbox in the order by highest fee. This could cause mailbox transaction messages to pile up. Also, since the Blockchain mailbox treated these transaction messages all at the same priority level, so new transactions added with a high fee would still need to wait for those ahead of it in the mailbox to be re-verified before getting to them.

Root Cause
A very large MemoryPool would lead to way too many transactions potentially needing to be re-verified each block. A lot of the re-verification is wasted effort also since the consensus will only take a subset (500) of such transactions per block. The ideal scenario would only be to reverify those that can make it into the next block.

Summary of Change
This change only re-verifies as many transactions as necessary to start off with a pool containing as many transactions as can be accepted in a block. To facilitate this, it is best to keep the transactions sorted so it can easily iterate the top transactions to be re-verified after persisting a block. Since it keeps transactions now sorted based on their priority by fee per byte, then fee, then TX hash, it can easily verify the transactions with minimal required effort. It also only attempts to re-verify transactions if it is at the header height. Additionally, it limits the time it will spend verifying transactions to add them back to the next block.

The mempool Count now includes all transactions that had been verified at some point that haven't become known to be invalid. The VerifiedCount and UnverifiedCount are also now exposed.

Closes #478
Closes #427
Closes #353
Closes #358
Closes #329
Closes #443
Closes #539
Closes #408 - May be debatable whether this should close this without a third category (aside from high and low priority) allowing a few completely free transactions to be persisted each block; but since we should not really promote hash war, I think it closes it.
Closes #428

[vncoelho]

This will be surely a great change, Jeff.

[erikzhang]

Maybe we should create a background thread to specifically validate transactions instead of porting the policy plugin.

[jsolman]

I thought about having another thread for verifying transactions, but it may require some locking and add some additional complexity. I’ll think about it some more while I do some testing on this.

This didn’t really port the whole policy plugin, but it does need access to how many transactions can be verified per block and how many of those are low priority. It still can end up with more transactions verified than the policy accepts by the time the consensus happens though, so the policy is still necessary with this as the changes currently stand.

[jsolman]

Whether I add an additional thread for verifying transactions or not, I do think it make sense to verify more transactions after PersistCompleted in the case it runs out of time before it could add the desired amount. On a powerful machine it should always finish probably, but since there will still be 8+ seconds (with 15 sec blocks) or so before the next consensus it makes sense to verify any more that can fit in with the remaining time available, and is a good case for a separate thread to be able push max allowed TX per block to the max. I will make an adjustment in some way to handle that.

Options:

1. send a new VerifyTXBatch message back to self that upon completion sends another VerifyTXBatch message while verified transactions are below the desired amount. This gives a chance for new transactions to come in between the batches.
2. use a background thread for verifying messages; it has to pause to allow verifying a new TX that comes in so that it can respond with relay reason in a timely fashion
3. Switch the Blockchain mailbox to have a real priority queue that for transactions considers their fees in order of message delivery — I don’t really like this because it’s not a good idea to leave a message hanging without a response to whether the transaction was relayed and why.

Option 1 is probably the easiest to implement and I may go that route for this PR. There aren’t that many other messages the Blockchain actor is processing while waiting for a new block and transaction verification is taking place, so there doesn’t seem like there would be a significant benefit for another thread for the verification purpose, but let me think about it some more.

[erikzhang]

Option 1 is fine, we can send an Idle message to the actor and then handle some things.

[erikzhang]

Send Idle message to actors when they are idle: #502

[jsolman]

Thanks for the quick addition of the Idle message. I’ll update it to use the Idle message when it is working hours for me again.

[jsolman]

I updated call-outs in my PR overview to account for my latest changes that re-verify unverified transactions if necessary when the Blockchain actor is idle.

[jsolman]

@shargon thanks for that optimization

[erikzhang]

Better Idle message: #505

[erikzhang]

What about reordering transactions each time? Our memory pool can only contain up to 50,000 transactions, and reordering takes only 44 milliseconds.

void TestSort()
{
    Random rand = new Random();
    byte[] buffer = new byte[sizeof(long)];
    long[] values = new long[50000];
    for (int i = 0; i < values.Length; i++)
    {
        rand.NextBytes(buffer);
        values[i] = BitConverter.ToInt64(buffer, 0);
    }
    Stopwatch stopwatch = Stopwatch.StartNew();
    values = values.OrderBy(p => p).ToArray();
    stopwatch.Stop();
    Console.WriteLine($"{stopwatch.ElapsedMilliseconds} ms"); // 44 ms
}

[jsolman]

Sorting on persist may be faster; I'll look tomorrow.

[jsolman]

Now testing filling the pool with high priority transactions. I have to reduce from allowing 500 per block to be able to test well in my PrivateNet or it verifies them as fast as I send them, and have trouble filling it up. It's because I'm sending by scripting a full node though; if I was using Rpc to send it would send them much faster. I might switch to sending that way later.

[jsolman]

I set MaxTransactionsPerBlock to 25 and low priority to 5 to make testing faster.

Some interesting logs from testing as we hit max tx count for the pool filling with high priority transactions, the high priority TX start pushing out low priority from the pool (as expected):

2019-01-10 21:54:29.962 T  Neo.Ledger.Blockchain: OnNewBlock received block Index=699
2019-01-10 21:54:30.004 D  Neo.Ledger.MemoryPool: After Persist counts:
 _unsortedTransactions.Count: 25
 _sortedLowPrioTransactions.Count: 5
 _sortedHighPrioTransactions.Count: 20
 _unverifiedTransactions.Count: 4959
 _unverifiedSortedHighPriorityTransactions.Count: 2859
 _unverifiedSortedLowPriorityTransactions.Count: 2100

2019-01-10 21:54:47.035 T  Neo.Ledger.Blockchain: OnNewBlock received block Index=700
2019-01-10 21:54:47.069 T  Neo.Network.P2P.LocalNode: Telling blockchain about relayed block w/ index: 700.
2019-01-10 21:54:47.090 D  Neo.Ledger.MemoryPool: After Persist counts:
 _unsortedTransactions.Count: 25
 _sortedLowPrioTransactions.Count: 5
 _sortedHighPrioTransactions.Count: 20
 _unverifiedTransactions.Count: 4951
 _unverifiedSortedHighPriorityTransactions.Count: 2887
 _unverifiedSortedLowPriorityTransactions.Count: 2064

Reduced TX per block again again, continuing to push out low priority transactions

2019-01-10 22:18:11.822 T  Neo.Ledger.Blockchain: OnNewBlock received block Index=789
2019-01-10 22:18:11.853 D  Neo.Ledger.MemoryPool: After Persist counts:
 _unsortedTransactions.Count: 10
 _sortedLowPrioTransactions.Count: 3
 _sortedHighPrioTransactions.Count: 7
 _unverifiedTransactions.Count: 4984
 _unverifiedSortedHighPriorityTransactions.Count: 4527
 _unverifiedSortedLowPriorityTransactions.Count: 457

Nearly all pushed out:

2019-01-10 22:23:52.376 D  Neo.Ledger.MemoryPool: After Persist counts:
 _unsortedTransactions.Count: 10
 _sortedLowPrioTransactions.Count: 3
 _sortedHighPrioTransactions.Count: 7
 _unverifiedTransactions.Count: 4975
 _unverifiedSortedHighPriorityTransactions.Count: 4922
 _unverifiedSortedLowPriorityTransactions.Count: 53

All out:

2019-01-10 22:25:11.391 T  Neo.Ledger.Blockchain: OnNewBlock received block Index=815
2019-01-10 22:25:11.449 D  Neo.Ledger.MemoryPool: After Persist counts:
 _unsortedTransactions.Count: 7
 _sortedLowPrioTransactions.Count: 0
 _sortedHighPrioTransactions.Count: 7
 _unverifiedTransactions.Count: 4987
 _unverifiedSortedHighPriorityTransactions.Count: 4987
 _unverifiedSortedLowPriorityTransactions.Count: 0

Summary of what was verified:

• Pool doesn't grow beyond the max count
  • tested with pool full of only low priority TX
  • tested with a combination of high and low priority TX in the pool
  • tested with the pool full of only high priority TX
• High priority transactions push out low priority transactions once max is hit
  • Tested continuing to send high priority transactions until the pool had only high priority transactions
• Verified receiving low priority transactions when the pool is full won't push out any high priority transactions
• Consensus continues verifying transactions approximately as expected during high transaction load

[jsolman]

@vncoelho

When you have the final version notify us and we can stress it again.

Final version is ready.

[vncoelho]

Great job and report, my friend. Congratulations.

[ThomasLobker]

Thanks Jeff, this is great work! It will provide a lot of improved stability.