From 298117628d47630659bbcf314a519fbe03c98df5 Mon Sep 17 00:00:00 2001
From: Vasil Dimov <vd@freebsd.org>
Date: Wed, 18 Sep 2019 23:35:38 +0200
Subject: [PATCH] fix(core-transaction-pool): sort by fee, nonce (#2937)

The merge 3923c3755 wrongly removed the method sortAll() from memory.ts.
This method was sorting transactions in the pool by fee, nonce (sorted
by highest fee first, but transactions from the same sender must be
sorted lowest nonce first).

Restore the sortAll() method as of before the merge and adapt it to use
the calculateTransactionExpiration() method for expiration sorting.
---
 .../core-transaction-pool/connection.test.ts  |   3 +-
 packages/core-transaction-pool/src/memory.ts  | 103 +++++++++++++-----
 2 files changed, 75 insertions(+), 31 deletions(-)

diff --git a/__tests__/unit/core-transaction-pool/connection.test.ts b/__tests__/unit/core-transaction-pool/connection.test.ts
index d75f871828..b1f3f667da 100644
--- a/__tests__/unit/core-transaction-pool/connection.test.ts
+++ b/__tests__/unit/core-transaction-pool/connection.test.ts
@@ -1044,8 +1044,7 @@ describe("Connection", () => {
             expect(topFeesReceived).toEqual(topFeesExpected);
         });
 
-        // TODO: @vd connection.getTransactions(...) doesn't return sorted by nonce
-        it.skip("sort by fee, nonce", async () => {
+        it("sort by fee, nonce", async () => {
             const nTransactions = 1000;
             const nDifferentSenders = 100;
 
diff --git a/packages/core-transaction-pool/src/memory.ts b/packages/core-transaction-pool/src/memory.ts
index 2373539c61..e90883e691 100644
--- a/packages/core-transaction-pool/src/memory.ts
+++ b/packages/core-transaction-pool/src/memory.ts
@@ -31,35 +31,7 @@ export class Memory {
 
     public allSortedByFee(): Interfaces.ITransaction[] {
         if (!this.allIsSorted) {
-            const currentHeight: number = this.currentHeight();
-            const expirationContext = {
-                blockTime: Managers.configManager.getMilestone(currentHeight).blocktime,
-                currentHeight,
-                now: Crypto.Slots.getTime(),
-            };
-
-            this.all.sort((a, b) => {
-                const feeA: Utils.BigNumber = a.data.fee;
-                const feeB: Utils.BigNumber = b.data.fee;
-
-                if (feeA.isGreaterThan(feeB)) {
-                    return -1;
-                }
-
-                if (feeA.isLessThan(feeB)) {
-                    return 1;
-                }
-
-                const expirationA: number = this.calculateTransactionExpiration(a, expirationContext);
-                const expirationB: number = this.calculateTransactionExpiration(b, expirationContext);
-
-                if (expirationA !== null && expirationB !== null) {
-                    return expirationA - expirationB;
-                }
-
-                return 0;
-            });
-
+            this.sortAll();
             this.allIsSorted = true;
         }
 
@@ -286,6 +258,79 @@ export class Memory {
         return removed;
     }
 
+    /**
+     * Sort `this.all` by fee (highest fee first) with the exception that transactions
+     * from the same sender must be ordered lowest `nonce` first.
+     */
+    private sortAll(): void {
+        const currentHeight: number = this.currentHeight();
+        const expirationContext = {
+            blockTime: Managers.configManager.getMilestone(currentHeight).blocktime,
+            currentHeight,
+            now: Crypto.Slots.getTime(),
+        };
+
+        this.all.sort((a, b) => {
+            const feeA: Utils.BigNumber = a.data.fee;
+            const feeB: Utils.BigNumber = b.data.fee;
+
+            if (feeA.isGreaterThan(feeB)) {
+                return -1;
+            }
+
+            if (feeA.isLessThan(feeB)) {
+                return 1;
+            }
+
+            const expirationA: number = this.calculateTransactionExpiration(a, expirationContext);
+            const expirationB: number = this.calculateTransactionExpiration(b, expirationContext);
+
+            if (expirationA !== null && expirationB !== null) {
+                return expirationA - expirationB;
+            }
+
+            return 0;
+        });
+
+        const indexBySender = {};
+        for (let i = 0; i < this.all.length; i++) {
+            const transaction: Interfaces.ITransaction = this.all[i];
+
+            if (transaction.data.version < 2) {
+                continue;
+            }
+
+            const sender: string = transaction.data.senderPublicKey;
+            if (indexBySender[sender] === undefined) {
+                indexBySender[sender] = [];
+            }
+            indexBySender[sender].push(i);
+
+            let nMoved = 0;
+
+            for (let j = 0; j < indexBySender[sender].length - 1; j++) {
+                const prevIndex: number = indexBySender[sender][j];
+                if (this.all[i].data.nonce.isLessThan(this.all[prevIndex].data.nonce)) {
+                    const newIndex = i + 1 + nMoved;
+                    this.all.splice(newIndex, 0, this.all[prevIndex]);
+                    this.all[prevIndex] = undefined;
+
+                    indexBySender[sender][j] = newIndex;
+
+                    nMoved++;
+                }
+            }
+
+            if (nMoved > 0) {
+                indexBySender[sender].sort((a, b) => a - b);
+            }
+
+            i += nMoved;
+        }
+
+        this.all = this.all.filter(t => t !== undefined);
+    }
+
     private currentHeight(): number {
         return app
             .resolvePlugin<State.IStateService>("state")