Research/parallel-tx/Refactor validblock

rskj-core/src/main/java/co/rsk/core/bc/BlockExecutor.java

@@ -608,7 +608,7 @@ private BlockResult executeForMiningAfterRSKIP144(
 
             TransactionExecutor txExecutor = transactionExecutorFactory.newInstance(
                     tx,
-                    txindex++,
+                    txindex,
                     block.getCoinbase(),
                     track,
                     block,
@@ -625,6 +625,7 @@ private BlockResult executeForMiningAfterRSKIP144(
                 }
 
                 loggingDiscardedBlock(block, tx);
+                txindex++;
                 continue;
             }
 
@@ -640,6 +641,7 @@ private BlockResult executeForMiningAfterRSKIP144(
                     return getBlockResultAndLogExecutionInterrupted(block, metric, tx);
                 }
                 loggingDiscardedBlock(block, tx);
+                txindex++;
                 continue;
             }
 
@@ -668,6 +670,7 @@ private BlockResult executeForMiningAfterRSKIP144(
             loggingExecuteTxAndReceipt(block, i, tx);
 
             i++;
+            txindex++;
 
             receiptsByTx.put(tx, receipt);
 

rskj-core/src/main/java/co/rsk/validators/ValidTxExecutionSublistsEdgesRule.java

@@ -19,29 +19,34 @@ public class ValidTxExecutionSublistsEdgesRule implements BlockValidationRule {
     @Override
     public boolean isValid(Block block) {
         short[] edges = block.getHeader().getTxExecutionSublistsEdges();
-        int nTxs = block.getTransactionsList().size();
 
-        if (edges == null) {
+        if (edges == null || edges.length == 0) {
             return true;
         }
+
         if (edges.length > Constants.getTransactionExecutionThreads()) {
             logger.warn("Invalid block: number of execution lists edges is greater than number of execution threads ({} vs {})",
                         edges.length, Constants.getTransactionExecutionThreads());
             return false;
         }
-        short prev = 0;
 
-        for (short edge : edges) {
-            if (edge <= prev) {
+        if (edges[0] <= 0) {
+            logger.warn("Invalid block: execution list edge is out of bounds");
+            return false;
+        }
+
+        for (int i = 0; i < edges.length - 1; i++) {
+            if (edges[i] >= edges[i + 1]) {
                 logger.warn("Invalid block: execution lists edges are not in ascending order");
                 return false;
             }
-            if (edge > nTxs) {
-                logger.warn("Invalid block: execution list edge is out of bounds: {}", edge);
-                return false;
-            }
-            prev = edge;
         }
+
+        if (edges[edges.length-1] > block.getTransactionsList().size() - 1) {
+            logger.warn("Invalid block: execution list edge is out of bounds");
+            return false;
+        }
+
         return true;
     }
 }

rskj-core/src/main/java/org/ethereum/core/Transaction.java

@@ -559,7 +559,9 @@ private boolean checkRemascAddress() {
     }
 
     private boolean checkRemascTxZeroValues() {
-        if (null != getData() || null != getSignature()) {
+        byte[] currentData = getData();
+
+        if ((null != currentData && currentData.length != 0) || null != getSignature()) {
             return false;
         }
 

rskj-core/src/test/java/co/rsk/core/TransactionIsRemascTest.java

@@ -0,0 +1,156 @@
+/*
+ * This file is part of RskJ
+ * Copyright (C) 2017 RSK Labs Ltd.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Lesser General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+package co.rsk.core;
+
+import org.bouncycastle.util.encoders.Hex;
+import org.ethereum.core.*;
+import org.ethereum.crypto.ECKey;
+import org.ethereum.crypto.HashUtil;
+import org.ethereum.util.ByteUtil;
+import org.ethereum.util.RLP;
+import org.ethereum.vm.PrecompiledContracts;
+import org.junit.Test;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class TransactionIsRemascTest {
+    int txPosition = 6;
+    int txsSize = 7;
+    RskAddress destination = PrecompiledContracts.REMASC_ADDR;
+    byte[] data = null;
+    Coin value = Coin.ZERO;
+    byte[] gasPrice = Hex.decode("00");
+    byte[] gasLimit = Hex.decode("00");
+
+    private Transaction buildTx(
+        RskAddress destination,
+        byte[] data,
+        Coin value,
+        byte[] gasPrice,
+        byte[] gasLimit
+    ) {
+        return Transaction.builder()
+            .destination(destination)
+            .data(data)
+            .value(value)
+            .gasPrice(gasPrice)
+            .gasLimit(gasLimit)
+            .build();
+    }
+
+    private void assertIsRemascTransaction(
+        RskAddress destination,
+        byte[] data,
+        Coin value,
+        byte[] gasPrice,
+        byte[] gasLimit,
+        int txPosition,
+        int txsSize
+    ) {
+        Transaction tx = buildTx(destination, data, value, gasPrice, gasLimit);
+
+        assertTrue(tx.isRemascTransaction(txPosition, txsSize));
+    }
+
+    private void assertIsNotRemascTransaction(
+        Transaction tx,
+        int txPosition,
+        int txsSize
+    ) {
+        assertFalse(tx.isRemascTransaction(txPosition, txsSize));
+    }
+
+    private void assertIsNotRemascTransaction(
+        RskAddress destination,
+        byte[] data,
+        Coin value,
+        byte[] gasPrice,
+        byte[] gasLimit,
+        int txPosition,
+        int txsSize
+    ) {
+        Transaction tx = buildTx(destination, data, value, gasPrice, gasLimit);
+
+        assertIsNotRemascTransaction(tx, txPosition, txsSize);
+    }
+
+    @Test
+    public void validRemascTransactionNullData() {
+        assertIsRemascTransaction(destination, data, value, gasPrice, gasLimit, txPosition, txsSize);
+    }
+
+    @Test
+    public void validRemascTransactionEmptyData() {
+        byte[] data = {};
+        assertIsRemascTransaction(destination, data, value, gasPrice, gasLimit, txPosition, txsSize);
+    }
+
+    @Test
+    public void notRemascTransactionNotLastTx() {
+        int txPosition = 3;
+        assertIsNotRemascTransaction(destination, data, value, gasPrice, gasLimit, txPosition, txsSize);
+    }
+
+    @Test
+    public void notRemascTransactionNotEmptyData() {
+        byte[] data = { 1, 2, 3, 4 };
+        assertIsNotRemascTransaction(destination, data, value, gasPrice, gasLimit, txPosition, txsSize);
+    }
+
+    @Test
+    public void notRemascTransactionNotNullSig() {
+        byte[] senderPrivateKey = HashUtil.keccak256("cow".getBytes());
+        Transaction tx = buildTx(destination, data, value, gasPrice, gasLimit);
+        tx.sign(senderPrivateKey);
+
+        assertIsNotRemascTransaction(tx, txPosition, txsSize);
+    }
+
+    @Test
+    public void notRemascTransactionReceiverIsNotRemasc() {
+        byte[] privateKey = HashUtil.keccak256("cat".getBytes());
+        ECKey ecKey = ECKey.fromPrivate(privateKey);
+        RskAddress destination =  RLP.parseRskAddress(ByteUtil.cloneBytes(ecKey.getAddress()));
+
+        assertIsNotRemascTransaction(destination, data, value, gasPrice, gasLimit, txPosition, txsSize);
+    }
+
+
+    @Test
+    public void notRemascTransactionValueIsNotZero() {
+        Coin value = Coin.valueOf(10);
+        assertIsNotRemascTransaction(destination, data, value, gasPrice, gasLimit, txPosition, txsSize);
+    }
+
+
+    @Test
+    public void notRemascTransactionGasPriceIsNotZero() {
+        byte[] gasPrice = { 10 };
+        assertIsNotRemascTransaction(destination, data, value, gasPrice, gasLimit, txPosition, txsSize);
+    }
+
+
+    @Test
+    public void notRemascTransactionGasLimitIsNotZero() {
+        byte[] gasLimit = { 10 };
+        assertIsNotRemascTransaction(destination, data, value, gasPrice, gasLimit, txPosition, txsSize);
+    }
+}
+

rskj-core/src/test/java/co/rsk/core/bc/BlockExecutorTest.java

@@ -315,8 +315,7 @@ public void executeAndFillBlockWithOneTransaction() {
         Assert.assertEquals(3000000, new BigInteger(1, block.getGasLimit()).longValue());
     }
 
-    @Test
-    public void executeAndFillBlockWithTxToExcludeBecauseSenderHasNoBalance() {
+    private Block createBlockWithExcludedTransaction(boolean withRemasc) {
         TrieStore trieStore = new TrieStoreImpl(new HashMapDB());
         Repository repository = new MutableRepository(new MutableTrieImpl(trieStore, new Trie(trieStore)));
 
@@ -358,6 +357,16 @@ public void executeAndFillBlockWithTxToExcludeBecauseSenderHasNoBalance() {
         txs.add(tx);
         txs.add(tx2);
 
+
+        List<Transaction> expectedTxList = new ArrayList<Transaction>();
+        expectedTxList.add(tx);
+
+        if (withRemasc) {
+            Transaction remascTx = new RemascTransaction(1L);
+            txs.add(remascTx);
+            expectedTxList.add(remascTx);
+        }
+
         List<BlockHeader> uncles = new ArrayList<>();
 
         BlockGenerator blockGenerator = new BlockGenerator(Constants.regtest(), activationConfig);
@@ -367,16 +376,24 @@ public void executeAndFillBlockWithTxToExcludeBecauseSenderHasNoBalance() {
 
         executor.executeAndFill(block, genesis.getHeader());
 
+        Assert.assertEquals(tx, block.getTransactionsList().get(0));
+        Assert.assertArrayEquals(
+                calculateTxTrieRoot(expectedTxList, block.getNumber()),
+                block.getTxTrieRoot()
+        );
+
+        return block;
+    }
+
+    @Test
+    public void executeAndFillBlockWithTxToExcludeBecauseSenderHasNoBalance() {
+        Block block = createBlockWithExcludedTransaction(false);
+
         short[] expectedEdges = activeRskip144 ? new short[]{(short) block.getTransactionsList().size()} : null;
 
         Assert.assertArrayEquals(expectedEdges, block.getHeader().getTxExecutionSublistsEdges());
         // Check tx2 was excluded
         Assert.assertEquals(1, block.getTransactionsList().size());
-        Assert.assertEquals(tx, block.getTransactionsList().get(0));
-        Assert.assertArrayEquals(
-                calculateTxTrieRoot(Collections.singletonList(tx), block.getNumber()),
-                block.getTxTrieRoot()
-        );
 
         Assert.assertEquals(3141592, new BigInteger(1, block.getGasLimit()).longValue());
     }
@@ -660,6 +677,54 @@ public void executeParallelBlockTwice() {
         Assert.assertArrayEquals(block1.getHash().getBytes(), block2.getHash().getBytes());
     }
 
+    private void testBlockWithTxTxEdgesMatchAndRemascTxIsAtLastPosition (int txAmount, short [] expectedSublistsEdges) {
+        Block block = getBlockWithNIndependentTransactions(txAmount, BigInteger.valueOf(21000), true);
+
+        assertBlockResultHasTxEdgesAndRemascAtLastPosition(block, txAmount, expectedSublistsEdges);
+    }
+
+    private void assertBlockResultHasTxEdgesAndRemascAtLastPosition (Block block, int txAmount, short [] expectedSublistsEdges) {
+        Block parent = blockchain.getBestBlock();
+        BlockResult blockResult = executor.executeAndFill(block, parent.getHeader());
+
+        int expectedTxSize = txAmount + 1;
+
+        Assert.assertArrayEquals(expectedSublistsEdges, blockResult.getTxEdges());
+        Assert.assertEquals(expectedTxSize, blockResult.getExecutedTransactions().size());
+        Assert.assertTrue(blockResult.getExecutedTransactions().get(txAmount).isRemascTransaction(txAmount, expectedTxSize));
+    }
+
+    @Test
+    public void blockWithOnlyRemascShouldGoToSequentialSublist () {
+        if (!activeRskip144) return;
+        testBlockWithTxTxEdgesMatchAndRemascTxIsAtLastPosition(0, new short[]{});
+    }
+
+    @Test
+    public void blockWithOneTxRemascShouldGoToSequentialSublist () {
+        if (!activeRskip144) return;
+        testBlockWithTxTxEdgesMatchAndRemascTxIsAtLastPosition(1, new short[]{ 1 });
+    }
+
+    @Test
+    public void blockWithManyTxsRemascShouldGoToSequentialSublist () {
+        if (!activeRskip144) return;
+        testBlockWithTxTxEdgesMatchAndRemascTxIsAtLastPosition(3, new short[]{ 1, 2, 3 });
+    }
+
+    @Test
+    public void blockWithMoreThanThreadsTxsRemascShouldGoToSequentialSublist () {
+        if (!activeRskip144) return;
+        testBlockWithTxTxEdgesMatchAndRemascTxIsAtLastPosition(5, new short[]{ 2, 3, 4, 5 });
+    }
+
+    @Test
+    public void blockWithExcludedTransactionHasRemascInSequentialSublist () {
+        if (!activeRskip144) return;
+        Block block = createBlockWithExcludedTransaction(true);
+        assertBlockResultHasTxEdgesAndRemascAtLastPosition(block, 0, new short[]{});
+    }
+
     @Test
     public void validateStateRootWithRskip126DisabledAndValidStateRoot() {
         TrieStore trieStore = new TrieStoreImpl(new HashMapDB());