Research/parallel-tx/Refactor validblock (#1794)

* Refactor repeated code * Refactor repeated edges mocking * Modify test for too many edges - it was also hitting out of bounds * Refactor out of bounds validation If edges are in order, last edge decides if any was out of bounds * Rename test * Add case where first edge is zero First changed to prev = -1, then added test that failed, then set back prev = 0 * Refactor order verification * Research/refactoring code (#1817) * store and return a copy of txEdges * merged two ifs in the BlockHeaderBuilder * DummyTracker updated so it returns a copy of the maps * Unused hashmap import deleted from ProgramTraceProcessor * BlockResult returns and stores a copy of txEdges * big refactor in BlockExecutor * Refactor ParallelizeTransactionHandler * renaming Bucket to Sublist * renaming txExecutionListsEdges to txExecutionSublistEdges * simplification of Handler's code * InterruptedException solved properly in the BlockExecutor * Research/parallel-tx/optimize writes (#1811) * Avoid tracking write for add balance 0 * Avoid tracking when writing the same value * Rename assertion * Remove condition for add balance zero It is considered in "write the same value" * Revert breaks * Add test refactor back * Make remasc transaction not be in a parallelized sublist * Research/parallel-tx/Fix isRemascTransaction validation and pass correct index (#1822) * Fix isRemascTransaction validation and pass correct index * Remove -1 * Rename local data * Add tests for isRemascTransaction * Add tests for remasc transaction * Fix txindex Added test for block with rejected tx * Add missing txindex++ * Fix merge error Co-authored-by: julianlen <[email protected]>
rsksmart · Nov 16, 2022 · ecb2a65 · ecb2a65
1 parent 29b92e2
commit ecb2a65
Show file tree

Hide file tree

Showing 6 changed files with 297 additions and 43 deletions.
diff --git a/rskj-core/src/main/java/co/rsk/core/bc/BlockExecutor.java b/rskj-core/src/main/java/co/rsk/core/bc/BlockExecutor.java
@@ -601,7 +601,7 @@ private BlockResult executeForMiningAfterRSKIP144(
 
             TransactionExecutor txExecutor = transactionExecutorFactory.newInstance(
                     tx,
-                    txindex++,
+                    txindex,
                     block.getCoinbase(),
                     track,
                     block,
@@ -618,6 +618,7 @@ private BlockResult executeForMiningAfterRSKIP144(
                 }
 
                 loggingDiscardedBlock(block, tx);
+                txindex++;
                 continue;
             }
 
@@ -633,6 +634,7 @@ private BlockResult executeForMiningAfterRSKIP144(
                     return getBlockResultAndLogExecutionInterrupted(block, metric, tx);
                 }
                 loggingDiscardedBlock(block, tx);
+                txindex++;
                 continue;
             }
 
@@ -661,6 +663,7 @@ private BlockResult executeForMiningAfterRSKIP144(
             loggingExecuteTxAndReceipt(block, i, tx);
 
             i++;
+            txindex++;
 
             receiptsByTx.put(tx, receipt);
 

diff --git a/rskj-core/src/main/java/co/rsk/validators/ValidTxExecutionSublistsEdgesRule.java b/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;
     }
 }
diff --git a/rskj-core/src/main/java/org/ethereum/core/Transaction.java b/rskj-core/src/main/java/org/ethereum/core/Transaction.java
@@ -545,7 +545,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;
         }
 

diff --git a/rskj-core/src/test/java/co/rsk/core/TransactionIsRemascTest.java b/rskj-core/src/test/java/co/rsk/core/TransactionIsRemascTest.java
@@ -0,0 +1,154 @@
+/*
+ * 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.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+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);
+
+        Assertions.assertTrue(tx.isRemascTransaction(txPosition, txsSize));
+    }
+
+    private void assertIsNotRemascTransaction(
+        Transaction tx,
+        int txPosition,
+        int txsSize
+    ) {
+        Assertions.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
+    void validRemascTransactionNullData() {
+        assertIsRemascTransaction(destination, data, value, gasPrice, gasLimit, txPosition, txsSize);
+    }
+
+    @Test
+    void validRemascTransactionEmptyData() {
+        byte[] data = {};
+        assertIsRemascTransaction(destination, data, value, gasPrice, gasLimit, txPosition, txsSize);
+    }
+
+    @Test
+    void notRemascTransactionNotLastTx() {
+        int txPosition = 3;
+        assertIsNotRemascTransaction(destination, data, value, gasPrice, gasLimit, txPosition, txsSize);
+    }
+
+    @Test
+    void notRemascTransactionNotEmptyData() {
+        byte[] data = { 1, 2, 3, 4 };
+        assertIsNotRemascTransaction(destination, data, value, gasPrice, gasLimit, txPosition, txsSize);
+    }
+
+    @Test
+    void notRemascTransactionNotNullSig() {
+        byte[] senderPrivateKey = HashUtil.keccak256("cow".getBytes());
+        Transaction tx = buildTx(destination, data, value, gasPrice, gasLimit);
+        tx.sign(senderPrivateKey);
+
+        assertIsNotRemascTransaction(tx, txPosition, txsSize);
+    }
+
+    @Test
+    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
+    void notRemascTransactionValueIsNotZero() {
+        Coin value = Coin.valueOf(10);
+        assertIsNotRemascTransaction(destination, data, value, gasPrice, gasLimit, txPosition, txsSize);
+    }
+
+
+    @Test
+    void notRemascTransactionGasPriceIsNotZero() {
+        byte[] gasPrice = { 10 };
+        assertIsNotRemascTransaction(destination, data, value, gasPrice, gasLimit, txPosition, txsSize);
+    }
+
+
+    @Test
+    void notRemascTransactionGasLimitIsNotZero() {
+        byte[] gasLimit = { 10 };
+        assertIsNotRemascTransaction(destination, data, value, gasPrice, gasLimit, txPosition, txsSize);
+    }
+}
+
diff --git a/rskj-core/src/test/java/co/rsk/core/bc/BlockExecutorTest.java b/rskj-core/src/test/java/co/rsk/core/bc/BlockExecutorTest.java
@@ -55,7 +55,6 @@
 import org.ethereum.vm.program.invoke.ProgramInvokeFactoryImpl;
 import org.junit.jupiter.api.Assertions;
 import org.junit.jupiter.api.BeforeEach;
-import org.junit.jupiter.api.Test;
 import org.junit.jupiter.params.ParameterizedTest;
 import org.junit.jupiter.params.provider.ValueSource;
 
@@ -334,10 +333,8 @@ void executeAndFillBlockWithOneTransaction(boolean activeRskip144) {
         Assertions.assertEquals(3000000, new BigInteger(1, block.getGasLimit()).longValue());
     }
 
-    @ParameterizedTest
-    @ValueSource(booleans = {true, false})
-    void executeAndFillBlockWithTxToExcludeBecauseSenderHasNoBalance(boolean activeRskip144) {
-        doReturn(activeRskip144).when(activationConfig).isActive(eq(ConsensusRule.RSKIP144), anyLong());
+
+    private Block createBlockWithExcludedTransaction(boolean withRemasc, boolean activeRskip144) {
         TrieStore trieStore = new TrieStoreImpl(new HashMapDB());
         Repository repository = new MutableRepository(new MutableTrieImpl(trieStore, new Trie(trieStore)));
 
@@ -379,6 +376,16 @@ void executeAndFillBlockWithTxToExcludeBecauseSenderHasNoBalance(boolean activeR
         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);
@@ -388,16 +395,27 @@ void executeAndFillBlockWithTxToExcludeBecauseSenderHasNoBalance(boolean activeR
 
         executor.executeAndFill(block, genesis.getHeader());
 
+        Assertions.assertEquals(tx, block.getTransactionsList().get(0));
+        Assertions.assertArrayEquals(
+                calculateTxTrieRoot(expectedTxList, block.getNumber()),
+                block.getTxTrieRoot()
+        );
+
+        return block;
+    }
+
+    @ParameterizedTest
+    @ValueSource(booleans = {true, false})
+    void executeAndFillBlockWithTxToExcludeBecauseSenderHasNoBalance(boolean activeRskip144) {
+        doReturn(activeRskip144).when(activationConfig).isActive(eq(ConsensusRule.RSKIP144), anyLong());
+        Block block = createBlockWithExcludedTransaction(false, activeRskip144);
+
         short[] expectedEdges = activeRskip144 ? new short[]{(short) block.getTransactionsList().size()} : null;
 
         Assertions.assertArrayEquals(expectedEdges, block.getHeader().getTxExecutionSublistsEdges());
         // Check tx2 was excluded
+
         Assertions.assertEquals(1, block.getTransactionsList().size());
-        Assertions.assertEquals(tx, block.getTransactionsList().get(0));
-        Assertions.assertArrayEquals(
-                calculateTxTrieRoot(Collections.singletonList(tx), block.getNumber()),
-                block.getTxTrieRoot()
-        );
 
         Assertions.assertEquals(3141592, new BigInteger(1, block.getGasLimit()).longValue());
     }
@@ -720,6 +738,60 @@ void executeParallelBlockTwice(boolean activeRskip144) {
         Assertions.assertArrayEquals(block1.getHash().getBytes(), block2.getHash().getBytes());
     }
 
+    private void testBlockWithTxTxEdgesMatchAndRemascTxIsAtLastPosition (int txAmount, short [] expectedSublistsEdges, Boolean activeRskip144) {
+        Block block = getBlockWithNIndependentTransactions(txAmount, BigInteger.valueOf(21000), true);
+
+        assertBlockResultHasTxEdgesAndRemascAtLastPosition(block, txAmount, expectedSublistsEdges, activeRskip144);
+    }
+
+    private void assertBlockResultHasTxEdgesAndRemascAtLastPosition (Block block, int txAmount, short [] expectedSublistsEdges, Boolean activeRskip144) {
+        Block parent = blockchain.getBestBlock();
+        BlockExecutor executor = buildBlockExecutor(trieStore, activeRskip144, RSKIP_126_IS_ACTIVE);
+        BlockResult blockResult = executor.executeAndFill(block, parent.getHeader());
+
+        int expectedTxSize = txAmount + 1;
+
+        Assertions.assertArrayEquals(expectedSublistsEdges, blockResult.getTxEdges());
+        Assertions.assertEquals(expectedTxSize, blockResult.getExecutedTransactions().size());
+        Assertions.assertTrue(blockResult.getExecutedTransactions().get(txAmount).isRemascTransaction(txAmount, expectedTxSize));
+    }
+
+    @ParameterizedTest
+    @ValueSource(booleans = {true, false})
+    void blockWithOnlyRemascShouldGoToSequentialSublist (boolean activeRskip144) {
+        if (!activeRskip144) return;
+        testBlockWithTxTxEdgesMatchAndRemascTxIsAtLastPosition(0, new short[]{}, activeRskip144);
+    }
+
+    @ParameterizedTest
+    @ValueSource(booleans = {true, false})
+    void blockWithOneTxRemascShouldGoToSequentialSublist (boolean activeRskip144) {
+        if (!activeRskip144) return;
+        testBlockWithTxTxEdgesMatchAndRemascTxIsAtLastPosition(1, new short[]{ 1 }, activeRskip144);
+    }
+
+    @ParameterizedTest
+    @ValueSource(booleans = {true, false})
+    void blockWithManyTxsRemascShouldGoToSequentialSublist (boolean activeRskip144) {
+        if (!activeRskip144) return;
+        testBlockWithTxTxEdgesMatchAndRemascTxIsAtLastPosition(3, new short[]{ 1, 2, 3 }, activeRskip144);
+    }
+
+    @ParameterizedTest
+    @ValueSource(booleans = {true, false})
+    void blockWithMoreThanThreadsTxsRemascShouldGoToSequentialSublist (boolean activeRskip144) {
+        if (!activeRskip144) return;
+        testBlockWithTxTxEdgesMatchAndRemascTxIsAtLastPosition(5, new short[]{ 2, 3, 4, 5 }, activeRskip144);
+    }
+
+    @ParameterizedTest
+    @ValueSource(booleans = {true, false})
+    void blockWithExcludedTransactionHasRemascInSequentialSublist (boolean activeRskip144) {
+        if (!activeRskip144) return;
+        Block block = createBlockWithExcludedTransaction(true, activeRskip144);
+        assertBlockResultHasTxEdgesAndRemascAtLastPosition(block, 0, new short[]{}, activeRskip144);
+    }
+
     @ParameterizedTest
     @ValueSource(booleans = {true, false})
     void validateStateRootWithRskip126DisabledAndValidStateRoot(boolean activeRskip144) {