More test and fix a border case.

rsksmart · Sep 20, 2018 · 8f39eec · 8f39eec
1 parent 4789718
commit 8f39eec
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 7 deletions.
diff --git a/rskj-core/src/main/java/co/rsk/core/bc/PendingState.java b/rskj-core/src/main/java/co/rsk/core/bc/PendingState.java
@@ -94,18 +94,20 @@ public static final List<Transaction> sortByPriceTakingIntoAccountSenderAndNonce
 
         long txsCount = transactions.size();
 
+        //Priority heap, and list of transactions are ordered according this comparator.
+        Comparator<Transaction> compHigherPrice  = (Transaction tx1, Transaction tx2) -> (tx2.getGasPrice().compareTo(tx1.getGasPrice()));
+
         //First create a map to separate txs by each sender.
         Map<RskAddress, List<Transaction>> mapSenderToTxs = transactions.stream().collect(Collectors.groupingBy(Transaction::getSender));
 
-        //For each sender list order all txs by nonce and then by hash
+        //For each sender list order all txs by nonce and then by hash,
+        //finally we order by price in cases where nonce are equal, and then for hash to disambiguate
         mapSenderToTxs.entrySet().stream().forEach(x -> x.getValue()
                 .sort(Comparator.<Transaction>comparingLong(tx -> ByteUtil.byteArrayToLong(tx.getNonce()))
-                .thenComparing(Transaction::getHash)));
+                .thenComparing(compHigherPrice).thenComparing(Transaction::getHash)));
 
-        //Priority heap is ordered according this comparator.
-        Comparator<Transaction> comp = (Transaction tx1, Transaction tx2) -> (tx2.getGasPrice().compareTo(tx1.getGasPrice()));
 
-        PriorityQueue<Transaction> treeTxs = new PriorityQueue(comp);
+        PriorityQueue<Transaction> treeTxs = new PriorityQueue(compHigherPrice);
         List<Transaction> retOrderTxs = new ArrayList();
 
         //Add first transaction of each list of senders to the heap.

diff --git a/rskj-core/src/test/java/co/rsk/mine/MinerUtilsTest.java b/rskj-core/src/test/java/co/rsk/mine/MinerUtilsTest.java
@@ -159,6 +159,7 @@ public void harmfulTransactionTest() {
     public void getAllTransactionsCheckOrderTest() {
         TransactionPool transactionPool = Mockito.mock(TransactionPool.class);
 
+        Transaction tx0 = Mockito.mock(Transaction.class);
         Transaction tx1 = Mockito.mock(Transaction.class);
         Transaction tx2 = Mockito.mock(Transaction.class);
         Transaction tx3 = Mockito.mock(Transaction.class);
@@ -168,6 +169,10 @@ public void getAllTransactionsCheckOrderTest() {
         byte[] nonce2 = ByteUtil.cloneBytes( BigInteger.valueOf(2).toByteArray());
 
         byte[] addressBytes = ByteUtil.leftPadBytes(BigInteger.valueOf(new Random(0).nextLong()).toByteArray(), 20);
+        Mockito.when(tx0.getSender()).thenReturn(new RskAddress(addressBytes));
+        Mockito.when(tx0.getNonce()).thenReturn(ByteUtil.cloneBytes(nonce0));
+        Mockito.when(tx0.getGasPrice()).thenReturn(Coin.valueOf(10));
+
         Mockito.when(tx1.getSender()).thenReturn(new RskAddress(addressBytes));
         Mockito.when(tx1.getNonce()).thenReturn(ByteUtil.cloneBytes(nonce0));
         Mockito.when(tx1.getGasPrice()).thenReturn(Coin.valueOf(1));
@@ -182,13 +187,27 @@ public void getAllTransactionsCheckOrderTest() {
 
         List<Transaction> txs = new LinkedList<>();
 
+        //Test order with same nonce different price
+        txs.add(tx0);
+        txs.add(tx1);
+
+        Mockito.when(transactionPool.getPendingTransactions()).thenReturn(txs);
+
+        List<Transaction> res = new MinerUtils().getAllTransactions(transactionPool);
+
+        Assert.assertEquals(2, res.size());
+        Assert.assertEquals(res.get(0).getGasPrice(), Coin.valueOf(10));
+        Assert.assertEquals(res.get(1).getGasPrice(), Coin.valueOf(1));
+
+        //Test order with same sender, different nonce, different price
+        txs = new LinkedList<>();
         txs.add(tx3);
         txs.add(tx1);
         txs.add(tx2);
 
         Mockito.when(transactionPool.getPendingTransactions()).thenReturn(txs);
 
-        List<Transaction> res = new MinerUtils().getAllTransactions(transactionPool);
+        res = new MinerUtils().getAllTransactions(transactionPool);
 
         Assert.assertEquals(3, res.size());
         Assert.assertEquals(res.get(0).getNonce(), tx1.getNonce());
@@ -198,7 +217,7 @@ public void getAllTransactionsCheckOrderTest() {
         Assert.assertEquals(res.get(1).getGasPrice(), Coin.valueOf(10));
         Assert.assertEquals(res.get(2).getGasPrice(), Coin.valueOf(100));
 
-        // Add new transactions
+        // Test order with different sender, nonce and price
         Transaction tx4 = Mockito.mock(Transaction.class);
         Transaction tx5 = Mockito.mock(Transaction.class);
         Transaction tx6 = Mockito.mock(Transaction.class);
@@ -216,6 +235,7 @@ public void getAllTransactionsCheckOrderTest() {
         Mockito.when(tx6.getNonce()).thenReturn(ByteUtil.cloneBytes(nonce2));
         Mockito.when(tx6.getGasPrice()).thenReturn(Coin.valueOf(1));
 
+        // Test another sender.
         txs.add(tx6);
         txs.add(tx5);
         txs.add(tx4);