From 0cafdaac01cceff673b13b540f076bd790cb7fd3 Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Thu, 19 Sep 2019 23:16:24 +0300 Subject: [PATCH 01/23] repro pruning failure Signed-off-by: Ratan Rai Sur --- .../besu/ethereum/worldstate/MarkSweepPrunerTest.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java index efa0c089b98..2737b42b511 100644 --- a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java +++ b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java @@ -111,12 +111,12 @@ private void testPrepareMarkAndSweep( // Prepare pruner.prepare(); - // Mark + // Choose mark block final BlockHeader markBlock = blockchain.getBlockHeader(markBlockNumber).get(); - pruner.mark(markBlock.getStateRoot()); - // Generate more blocks that should be kept generateBlockchainData(numBlocks - blockCountBeforeMarkedBlock, accountsPerBlock); + // Mark + pruner.mark(markBlock.getStateRoot()); // Collect the nodes we expect to keep final Set expectedNodes = collectWorldStateNodes(markBlock.getStateRoot()); From f724461675d0cfe7931593437a4a34e58ec94dee Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Thu, 19 Sep 2019 21:05:27 +0300 Subject: [PATCH 02/23] move where mark storage is cleared Signed-off-by: Ratan Rai Sur --- .../hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java index efa3b38a0eb..961807a87f9 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java @@ -97,6 +97,7 @@ public MarkSweepPruner( } public void prepare() { + markStorage.clear(); worldStateStorage.removeNodeAddedListener(nodeAddedListenerId); // Just in case. nodeAddedListenerId = worldStateStorage.addNodeAddedListener(this::markNewNodes); } @@ -107,7 +108,6 @@ public void cleanup() { public void mark(final Hash rootHash) { markOperationCounter.inc(); - markStorage.clear(); createStateTrie(rootHash) .visitAll( node -> { From ea1558903689b7209eb33639ea020deb5d069b4e Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Thu, 19 Sep 2019 17:31:13 +0300 Subject: [PATCH 03/23] check pending marks as well Signed-off-by: Ratan Rai Sur --- .../besu/ethereum/worldstate/MarkSweepPruner.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java index 961807a87f9..f4d84759a7f 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java @@ -54,7 +54,7 @@ public class MarkSweepPruner { private final Counter sweptNodesCounter; private volatile long nodeAddedListenerId; private final ReentrantLock markLock = new ReentrantLock(true); - private final Set pendingMarks = Collections.newSetFromMap(new ConcurrentHashMap<>()); + private final Set pendingMarks = Collections.newSetFromMap(new ConcurrentHashMap<>()); public MarkSweepPruner( final WorldStateStorage worldStateStorage, @@ -149,7 +149,9 @@ public void sweepBefore(final long markedBlockNumber) { } updater.commit(); // Sweep non-state-root nodes - prunedNodeCount += worldStateStorage.prune(markStorage::containsKey); + prunedNodeCount += + worldStateStorage.prune( + key -> pendingMarks.contains(Bytes32.wrap(key)) || markStorage.containsKey(key)); sweptNodesCounter.inc(prunedNodeCount); worldStateStorage.removeNodeAddedListener(nodeAddedListenerId); markStorage.clear(); From e040af83a353d91e896b577b46602b5802f82c5d Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Thu, 19 Sep 2019 17:31:20 +0300 Subject: [PATCH 04/23] additional logging Signed-off-by: Ratan Rai Sur --- .../org/hyperledger/besu/ethereum/worldstate/Pruner.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/Pruner.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/Pruner.java index b76a7978ebf..764212e6e09 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/Pruner.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/Pruner.java @@ -57,6 +57,7 @@ public Pruner( } public void start() { + LOG.info("Starting Pruner."); blockchain.observeBlockAdded((event, blockchain) -> handleNewBlock(event)); } @@ -100,7 +101,10 @@ private void mark(final BlockHeader header) { } private void sweep() { - LOG.info("Begin sweeping unused nodes for pruning. Retention period: {}", blocksRetained); + LOG.info( + "Begin sweeping unused nodes for pruning. Keeping full state for blocks {} to {}", + markBlockNumber, + markBlockNumber + blocksRetained); execute( () -> { pruningStrategy.sweepBefore(markBlockNumber); From 953fffd14f83abff32fe3f3fa736b5de439b2ef9 Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Thu, 19 Sep 2019 17:39:08 +0300 Subject: [PATCH 05/23] DRY Signed-off-by: Ratan Rai Sur --- .../ethereum/worldstate/MarkSweepPruner.java | 21 +++++++------------ 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java index f4d84759a7f..af090862a2b 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java @@ -99,7 +99,7 @@ public MarkSweepPruner( public void prepare() { markStorage.clear(); worldStateStorage.removeNodeAddedListener(nodeAddedListenerId); // Just in case. - nodeAddedListenerId = worldStateStorage.addNodeAddedListener(this::markNewNodes); + nodeAddedListenerId = worldStateStorage.addNodeAddedListener(this::markNodes); } public void cleanup() { @@ -184,10 +184,14 @@ private void processAccountState(final BytesValue value) { @VisibleForTesting void markNode(final Bytes32 hash) { - markedNodesCounter.inc(); + markNodes(Collections.singleton(hash)); + } + + private void markNodes(final Collection nodeHashes) { + markedNodesCounter.inc(nodeHashes.size()); markLock.lock(); try { - pendingMarks.add(hash); + pendingMarks.addAll(nodeHashes); maybeFlushPendingMarks(); } finally { markLock.unlock(); @@ -211,15 +215,4 @@ void flushPendingMarks() { markLock.unlock(); } } - - private void markNewNodes(final Collection nodeHashes) { - markedNodesCounter.inc(nodeHashes.size()); - markLock.lock(); - try { - pendingMarks.addAll(nodeHashes); - maybeFlushPendingMarks(); - } finally { - markLock.unlock(); - } - } } From 303fe32997178e00571969b0c688d09e5214b587 Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Fri, 20 Sep 2019 13:12:20 +0300 Subject: [PATCH 06/23] move test to integration Signed-off-by: Ratan Rai Sur --- .../hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename ethereum/core/src/{test => integration-test}/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java (100%) diff --git a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java b/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java similarity index 100% rename from ethereum/core/src/test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java rename to ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java From ec1435173c75b297471064ca9ddbde6fabf7d089 Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Sat, 21 Sep 2019 17:54:09 +0300 Subject: [PATCH 07/23] groovy spotless? Signed-off-by: Ratan Rai Sur --- build.gradle | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/build.gradle b/build.gradle index fe8b7877756..18003d96f0d 100644 --- a/build.gradle +++ b/build.gradle @@ -164,9 +164,7 @@ allprojects { } // Below this line are currently only license header tasks - format 'groovy', { - target '**/src/*/grovy/**/*.groovy' - } + format 'groovy', { target '**/src/*/grovy/**/*.groovy' } // Currently disabled due to referencetest issues // format 'bash', { From 33b0df029e50dd414da7a1bffb64021a7889285c Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Sat, 21 Sep 2019 17:54:37 +0300 Subject: [PATCH 08/23] run pruner instead of testing mark sweep pruner Signed-off-by: Ratan Rai Sur --- .../worldstate/MarkSweepPrunerTest.java | 150 +++++++++--------- .../besu/ethereum/worldstate/Pruner.java | 8 +- 2 files changed, 81 insertions(+), 77 deletions(-) diff --git a/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java b/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java index 2737b42b511..2177552160c 100644 --- a/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java +++ b/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java @@ -36,6 +36,7 @@ import org.hyperledger.besu.ethereum.trie.StoredMerklePatriciaTrie; import org.hyperledger.besu.metrics.noop.NoOpMetricsSystem; import org.hyperledger.besu.services.kvstore.InMemoryKeyValueStorage; +import org.hyperledger.besu.testutil.MockExecutorService; import org.hyperledger.besu.util.bytes.Bytes32; import org.hyperledger.besu.util.bytes.BytesValue; @@ -66,102 +67,99 @@ public class MarkSweepPrunerTest { private final MutableBlockchain blockchain = createInMemoryBlockchain(genesisBlock); @Test - public void prepareMarkAndSweep_smallState_manyOpsPerTx() { - testPrepareMarkAndSweep(3, 1, 2, 1000); + public void pruner_smallState_manyOpsPerTx() throws InterruptedException { + testPruner(3, 1, 4, 1000); } @Test - public void prepareMarkAndSweep_largeState_fewOpsPerTx() { - testPrepareMarkAndSweep(20, 5, 5, 5); + public void pruner_largeState_fewOpsPerTx() throws InterruptedException { + testPruner(2, 5, 6, 5); } @Test - public void prepareMarkAndSweep_emptyBlocks() { - testPrepareMarkAndSweep(10, 0, 5, 10); + public void pruner_emptyBlocks() throws InterruptedException { + testPruner(5, 0, 5, 10); } @Test - public void prepareMarkAndSweep_markChainhead() { - testPrepareMarkAndSweep(10, 2, 10, 20); + public void pruner_markChainhead() throws InterruptedException { + testPruner(4, 2, 10, 20); } - @Test - public void prepareMarkAndSweep_markGenesis() { - testPrepareMarkAndSweep(10, 2, 0, 20); - } - - @Test - public void prepareMarkAndSweep_multipleRounds() { - testPrepareMarkAndSweep(10, 2, 10, 20); - testPrepareMarkAndSweep(10, 2, 15, 20); - } - - private void testPrepareMarkAndSweep( - final int numBlocks, + private void testPruner( + final int numCycles, final int accountsPerBlock, - final int markBlockNumber, - final int opsPerTransaction) { - final MarkSweepPruner pruner = + final int numBlocksToKeep, + final int opsPerTransaction) + throws InterruptedException { + + final var markSweepPruner = new MarkSweepPruner( worldStateStorage, blockchain, markStorage, metricsSystem, opsPerTransaction); - final int chainHeight = (int) blockchain.getChainHead().getHeight(); - // Generate blocks up to markBlockNumber - final int blockCountBeforeMarkedBlock = markBlockNumber - chainHeight; - generateBlockchainData(blockCountBeforeMarkedBlock, accountsPerBlock); - - // Prepare - pruner.prepare(); - // Choose mark block - final BlockHeader markBlock = blockchain.getBlockHeader(markBlockNumber).get(); - // Generate more blocks that should be kept - generateBlockchainData(numBlocks - blockCountBeforeMarkedBlock, accountsPerBlock); - // Mark - pruner.mark(markBlock.getStateRoot()); - - // Collect the nodes we expect to keep - final Set expectedNodes = collectWorldStateNodes(markBlock.getStateRoot()); - for (int i = markBlockNumber; i <= blockchain.getChainHeadBlockNumber(); i++) { - final Hash stateRoot = blockchain.getBlockHeader(i).get().getStateRoot(); - collectWorldStateNodes(stateRoot, expectedNodes); - } - if (accountsPerBlock != 0 && markBlockNumber > 0) { - assertThat(hashValueStore.size()).isGreaterThan(expectedNodes.size()); // Sanity check - } - - // Sweep - pruner.sweepBefore(markBlock.getNumber()); + final var pruner = + new Pruner( + markSweepPruner, + blockchain, + new MockExecutorService(), + new PruningConfiguration(1, numBlocksToKeep)); + + pruner.start(); + + for (int cycle = 0; cycle < numCycles; ++cycle) { + int numBlockInCycle = + numBlocksToKeep + + 1; // +1 to get it to switch from MARKING_COMPLETE TO SWEEPING on each cycle + var fullyMarkedBlockNum = cycle * numBlockInCycle + 1; + + // This should cause a full mark and sweep cycle + assertThat(pruner.getState()).isEqualByComparingTo(Pruner.State.IDLE); + generateBlockchainData(numBlockInCycle, accountsPerBlock); + assertThat(pruner.getState()).isEqualByComparingTo(Pruner.State.IDLE); + + // Collect the nodes we expect to keep + final Set expectedNodes = new HashSet<>(); + for (int i = fullyMarkedBlockNum; i <= blockchain.getChainHeadBlockNumber(); i++) { + final Hash stateRoot = blockchain.getBlockHeader(i).get().getStateRoot(); + collectWorldStateNodes(stateRoot, expectedNodes); + } - // Assert that blocks from mark point onward are still accessible - for (int i = markBlockNumber; i <= blockchain.getChainHeadBlockNumber(); i++) { - final Hash stateRoot = blockchain.getBlockHeader(i).get().getStateRoot(); - assertThat(worldStateArchive.get(stateRoot)).isPresent(); - final WorldState markedState = worldStateArchive.get(stateRoot).get(); - // Traverse accounts and make sure all are accessible - final int expectedAccounts = accountsPerBlock * i; - final long accounts = markedState.streamAccounts(Bytes32.ZERO, expectedAccounts * 2).count(); - assertThat(accounts).isEqualTo(expectedAccounts); - // Traverse storage to ensure that all storage is accessible - markedState - .streamAccounts(Bytes32.ZERO, expectedAccounts * 2) - .forEach(a -> a.storageEntriesFrom(Bytes32.ZERO, 1000)); - } + if (accountsPerBlock != 0) { + assertThat(hashValueStore.size()) + .isGreaterThanOrEqualTo(expectedNodes.size()); // Sanity check + } - // All other state roots should have been removed - for (int i = 0; i < markBlockNumber; i++) { - final BlockHeader curHeader = blockchain.getBlockHeader(i + 1L).get(); - if (curHeader.getNumber() == markBlock.getNumber()) { - continue; + // Assert that blocks from mark point onward are still accessible + for (int i = fullyMarkedBlockNum; i <= blockchain.getChainHeadBlockNumber(); i++) { + final Hash stateRoot = blockchain.getBlockHeader(i).get().getStateRoot(); + assertThat(worldStateArchive.get(stateRoot)).isPresent(); + final WorldState markedState = worldStateArchive.get(stateRoot).get(); + // Traverse accounts and make sure all are accessible + final int expectedAccounts = accountsPerBlock * i; + final long accounts = + markedState.streamAccounts(Bytes32.ZERO, expectedAccounts * 2).count(); + assertThat(accounts).isEqualTo(expectedAccounts); + // Traverse storage to ensure that all storage is accessible + markedState + .streamAccounts(Bytes32.ZERO, expectedAccounts * 2) + .forEach(a -> a.storageEntriesFrom(Bytes32.ZERO, 1000)); } - if (!curHeader.getStateRoot().equals(Hash.EMPTY_TRIE_HASH)) { - assertThat(worldStateArchive.get(curHeader.getStateRoot())).isEmpty(); + + // All other state roots should have been removed + for (int i = 0; i < fullyMarkedBlockNum; i++) { + final BlockHeader curHeader = blockchain.getBlockHeader(i).get(); + if (!curHeader.getStateRoot().equals(Hash.EMPTY_TRIE_HASH)) { + assertThat(worldStateArchive.get(curHeader.getStateRoot())).isEmpty(); + } } + + // Check that storage contains only the values we expect + assertThat(hashValueStore.size()).isEqualTo(expectedNodes.size()); + assertThat(hashValueStore.values()) + .containsExactlyInAnyOrderElementsOf( + expectedNodes.stream().map(BytesValue::getArrayUnsafe).collect(Collectors.toSet())); } - // Check that storage contains only the values we expect - assertThat(hashValueStore.size()).isEqualTo(expectedNodes.size()); - assertThat(hashValueStore.values()) - .containsExactlyInAnyOrderElementsOf( - expectedNodes.stream().map(BytesValue::getArrayUnsafe).collect(Collectors.toSet())); + pruner.stop(); } @Test diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/Pruner.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/Pruner.java index 764212e6e09..054d065f74c 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/Pruner.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/Pruner.java @@ -23,6 +23,7 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicReference; +import com.google.common.annotations.VisibleForTesting; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -121,7 +122,12 @@ private void execute(final Runnable action) { } } - private enum State { + @VisibleForTesting + State getState() { + return state.get(); + } + + enum State { IDLE, MARK_BLOCK_CONFIRMATIONS_AWAITING, MARKING, From ce27ea6c644b4f2861b17fb77159481c242c209c Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Sat, 21 Sep 2019 18:00:53 +0300 Subject: [PATCH 09/23] rename test Signed-off-by: Ratan Rai Sur --- .../{MarkSweepPrunerTest.java => PruningIntegrationTest.java} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/{MarkSweepPrunerTest.java => PruningIntegrationTest.java} (99%) diff --git a/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java b/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PruningIntegrationTest.java similarity index 99% rename from ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java rename to ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PruningIntegrationTest.java index 2177552160c..419cef44992 100644 --- a/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java +++ b/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PruningIntegrationTest.java @@ -52,7 +52,7 @@ import org.junit.Test; import org.mockito.InOrder; -public class MarkSweepPrunerTest { +public class PruningIntegrationTest { private final BlockDataGenerator gen = new BlockDataGenerator(); private final NoOpMetricsSystem metricsSystem = new NoOpMetricsSystem(); From 253d0c777ac5d67ff43ef9c4310022b08f1e0bfb Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Sat, 21 Sep 2019 18:17:33 +0300 Subject: [PATCH 10/23] bring back MarkSweepPrunerTest for those unit tests Signed-off-by: Ratan Rai Sur --- .../worldstate/MarkSweepPrunerTest.java | 272 ++++++++++++++++++ 1 file changed, 272 insertions(+) create mode 100644 ethereum/core/src/test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java diff --git a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java new file mode 100644 index 00000000000..e71e7f4e475 --- /dev/null +++ b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java @@ -0,0 +1,272 @@ +/* + * Copyright ConsenSys AG. + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on + * an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the + * specific language governing permissions and limitations under the License. + * + * SPDX-License-Identifier: Apache-2.0 + */ +package org.hyperledger.besu.ethereum.worldstate; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.hyperledger.besu.ethereum.core.InMemoryStorageProvider.createInMemoryBlockchain; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.inOrder; +import static org.mockito.Mockito.spy; + +import org.hyperledger.besu.ethereum.chain.MutableBlockchain; +import org.hyperledger.besu.ethereum.core.Block; +import org.hyperledger.besu.ethereum.core.BlockDataGenerator; +import org.hyperledger.besu.ethereum.core.BlockDataGenerator.BlockOptions; +import org.hyperledger.besu.ethereum.core.BlockHeader; +import org.hyperledger.besu.ethereum.core.Hash; +import org.hyperledger.besu.ethereum.core.MutableWorldState; +import org.hyperledger.besu.ethereum.core.TransactionReceipt; +import org.hyperledger.besu.ethereum.core.WorldState; +import org.hyperledger.besu.ethereum.rlp.RLP; +import org.hyperledger.besu.ethereum.storage.keyvalue.WorldStateKeyValueStorage; +import org.hyperledger.besu.ethereum.storage.keyvalue.WorldStatePreimageKeyValueStorage; +import org.hyperledger.besu.ethereum.trie.MerklePatriciaTrie; +import org.hyperledger.besu.ethereum.trie.StoredMerklePatriciaTrie; +import org.hyperledger.besu.metrics.noop.NoOpMetricsSystem; +import org.hyperledger.besu.services.kvstore.InMemoryKeyValueStorage; +import org.hyperledger.besu.util.bytes.Bytes32; +import org.hyperledger.besu.util.bytes.BytesValue; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.function.Function; +import java.util.stream.Collectors; + +import org.junit.Test; +import org.mockito.InOrder; + +public class MarkSweepPrunerTest { + + private final BlockDataGenerator gen = new BlockDataGenerator(); + private final NoOpMetricsSystem metricsSystem = new NoOpMetricsSystem(); + private final Map hashValueStore = spy(new HashMap<>()); + private final InMemoryKeyValueStorage stateStorage = spy(new TestInMemoryStorage(hashValueStore)); + private final WorldStateStorage worldStateStorage = new WorldStateKeyValueStorage(stateStorage); + private final WorldStateArchive worldStateArchive = + new WorldStateArchive( + worldStateStorage, new WorldStatePreimageKeyValueStorage(new InMemoryKeyValueStorage())); + private final InMemoryKeyValueStorage markStorage = new InMemoryKeyValueStorage(); + private final Block genesisBlock = gen.genesisBlock(); + private final MutableBlockchain blockchain = createInMemoryBlockchain(genesisBlock); + + @Test + public void mark_marksAllExpectedNodes() { + final MarkSweepPruner pruner = + new MarkSweepPruner(worldStateStorage, blockchain, markStorage, metricsSystem); + + // Generate accounts and save corresponding state root + final int numBlocks = 15; + final int numAccounts = 10; + generateBlockchainData(numBlocks, numAccounts); + + final int markBlockNumber = 10; + final BlockHeader markBlock = blockchain.getBlockHeader(markBlockNumber).get(); + // Collect the nodes we expect to keep + final Set expectedNodes = collectWorldStateNodes(markBlock.getStateRoot()); + assertThat(hashValueStore.size()).isGreaterThan(expectedNodes.size()); // Sanity check + + // Mark and sweep + pruner.mark(markBlock.getStateRoot()); + pruner.sweepBefore(markBlock.getNumber()); + + // Assert that the block we marked is still present and all accounts are accessible + assertThat(worldStateArchive.get(markBlock.getStateRoot())).isPresent(); + final WorldState markedState = worldStateArchive.get(markBlock.getStateRoot()).get(); + // Traverse accounts and make sure all are accessible + final int expectedAccounts = numAccounts * markBlockNumber; + final long accounts = markedState.streamAccounts(Bytes32.ZERO, expectedAccounts * 2).count(); + assertThat(accounts).isEqualTo(expectedAccounts); + // Traverse storage to ensure that all storage is accessible + markedState + .streamAccounts(Bytes32.ZERO, expectedAccounts * 2) + .forEach(a -> a.storageEntriesFrom(Bytes32.ZERO, 1000)); + + // All other state roots should have been removed + for (int i = 0; i < numBlocks; i++) { + final BlockHeader curHeader = blockchain.getBlockHeader(i + 1L).get(); + if (curHeader.getNumber() == markBlock.getNumber()) { + continue; + } + assertThat(worldStateArchive.get(curHeader.getStateRoot())).isEmpty(); + } + + // Check that storage contains only the values we expect + assertThat(hashValueStore.size()).isEqualTo(expectedNodes.size()); + assertThat(hashValueStore.values()) + .containsExactlyInAnyOrderElementsOf( + expectedNodes.stream().map(BytesValue::getArrayUnsafe).collect(Collectors.toSet())); + } + + @Test + public void sweepBefore_shouldSweepStateRootFirst() { + final MarkSweepPruner pruner = + new MarkSweepPruner(worldStateStorage, blockchain, markStorage, metricsSystem, 1); + + // Generate accounts and save corresponding state root + final int numBlocks = 15; + final int numAccounts = 10; + generateBlockchainData(numBlocks, numAccounts); + + final int markBlockNumber = 10; + final BlockHeader markBlock = blockchain.getBlockHeader(markBlockNumber).get(); + + // Collect state roots we expect to be swept first + final List stateRoots = new ArrayList<>(); + for (int i = markBlockNumber - 1; i >= 0; i--) { + stateRoots.add(blockchain.getBlockHeader(i).get().getStateRoot()); + } + + // Mark and sweep + pruner.mark(markBlock.getStateRoot()); + pruner.sweepBefore(markBlock.getNumber()); + + // Check stateRoots are marked first + InOrder inOrder = inOrder(hashValueStore, stateStorage); + for (Bytes32 stateRoot : stateRoots) { + inOrder.verify(hashValueStore).remove(stateRoot); + } + inOrder.verify(stateStorage).removeAllKeysUnless(any()); + } + + @Test + public void sweepBefore_shouldNotRemoveMarkedStateRoots() { + final MarkSweepPruner pruner = + new MarkSweepPruner(worldStateStorage, blockchain, markStorage, metricsSystem, 1); + + // Generate accounts and save corresponding state root + final int numBlocks = 15; + final int numAccounts = 10; + generateBlockchainData(numBlocks, numAccounts); + + final int markBlockNumber = 10; + final BlockHeader markBlock = blockchain.getBlockHeader(markBlockNumber).get(); + + // Collect state roots we expect to be swept first + final List stateRoots = new ArrayList<>(); + for (int i = markBlockNumber - 1; i >= 0; i--) { + stateRoots.add(blockchain.getBlockHeader(i).get().getStateRoot()); + } + + // Mark + pruner.mark(markBlock.getStateRoot()); + // Mark an extra state root + Hash markedRoot = Hash.wrap(stateRoots.remove(stateRoots.size() / 2)); + pruner.markNode(markedRoot); + // Sweep + pruner.sweepBefore(markBlock.getNumber()); + + // Check stateRoots are marked first + InOrder inOrder = inOrder(hashValueStore, stateStorage); + for (Bytes32 stateRoot : stateRoots) { + inOrder.verify(hashValueStore).remove(stateRoot); + } + inOrder.verify(stateStorage).removeAllKeysUnless(any()); + + assertThat(stateStorage.containsKey(markedRoot.getArrayUnsafe())).isTrue(); + } + + private void generateBlockchainData(final int numBlocks, final int numAccounts) { + Block parentBlock = blockchain.getChainHeadBlock(); + for (int i = 0; i < numBlocks; i++) { + final MutableWorldState worldState = + worldStateArchive.getMutable(parentBlock.getHeader().getStateRoot()).get(); + gen.createRandomContractAccountsWithNonEmptyStorage(worldState, numAccounts); + final Hash stateRoot = worldState.rootHash(); + + final Block block = + gen.block( + BlockOptions.create() + .setStateRoot(stateRoot) + .setBlockNumber(parentBlock.getHeader().getNumber() + 1L) + .setParentHash(parentBlock.getHash())); + final List receipts = gen.receipts(block); + blockchain.appendBlock(block, receipts); + parentBlock = block; + } + } + + private Set collectWorldStateNodes(final Hash stateRootHash) { + final Set nodeData = new HashSet<>(); + collectWorldStateNodes(stateRootHash, nodeData); + return nodeData; + } + + private Set collectWorldStateNodes( + final Hash stateRootHash, final Set collector) { + final List storageRoots = new ArrayList<>(); + final MerklePatriciaTrie stateTrie = createStateTrie(stateRootHash); + + // Collect storage roots and code + stateTrie + .entriesFrom(Bytes32.ZERO, 1000) + .forEach( + (key, val) -> { + final StateTrieAccountValue accountValue = + StateTrieAccountValue.readFrom(RLP.input(val)); + stateStorage + .get(accountValue.getCodeHash().getArrayUnsafe()) + .ifPresent(v -> collector.add(BytesValue.wrap(v))); + storageRoots.add(accountValue.getStorageRoot()); + }); + + // Collect state nodes + collectTrieNodes(stateTrie, collector); + // Collect storage nodes + for (Hash storageRoot : storageRoots) { + final MerklePatriciaTrie storageTrie = createStorageTrie(storageRoot); + collectTrieNodes(storageTrie, collector); + } + + return collector; + } + + private void collectTrieNodes( + final MerklePatriciaTrie trie, final Set collector) { + final Bytes32 rootHash = trie.getRootHash(); + trie.visitAll( + (node) -> { + if (node.isReferencedByHash() || node.getHash().equals(rootHash)) { + collector.add(node.getRlp()); + } + }); + } + + private MerklePatriciaTrie createStateTrie(final Bytes32 rootHash) { + return new StoredMerklePatriciaTrie<>( + worldStateStorage::getAccountStateTrieNode, + rootHash, + Function.identity(), + Function.identity()); + } + + private MerklePatriciaTrie createStorageTrie(final Bytes32 rootHash) { + return new StoredMerklePatriciaTrie<>( + worldStateStorage::getAccountStorageTrieNode, + rootHash, + Function.identity(), + Function.identity()); + } + + private static class TestInMemoryStorage extends InMemoryKeyValueStorage { + + public TestInMemoryStorage(final Map hashValueStore) { + super(hashValueStore); + } + } +} From 366070e087b0b52f93db0a42ed0d1da6b20afb9a Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Sat, 21 Sep 2019 18:18:28 +0300 Subject: [PATCH 11/23] remove duplicate tests Signed-off-by: Ratan Rai Sur --- .../worldstate/PruningIntegrationTest.java | 122 ------------------ 1 file changed, 122 deletions(-) diff --git a/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PruningIntegrationTest.java b/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PruningIntegrationTest.java index 419cef44992..be387598f65 100644 --- a/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PruningIntegrationTest.java +++ b/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PruningIntegrationTest.java @@ -162,122 +162,6 @@ private void testPruner( pruner.stop(); } - @Test - public void mark_marksAllExpectedNodes() { - final MarkSweepPruner pruner = - new MarkSweepPruner(worldStateStorage, blockchain, markStorage, metricsSystem); - - // Generate accounts and save corresponding state root - final int numBlocks = 15; - final int numAccounts = 10; - generateBlockchainData(numBlocks, numAccounts); - - final int markBlockNumber = 10; - final BlockHeader markBlock = blockchain.getBlockHeader(markBlockNumber).get(); - // Collect the nodes we expect to keep - final Set expectedNodes = collectWorldStateNodes(markBlock.getStateRoot()); - assertThat(hashValueStore.size()).isGreaterThan(expectedNodes.size()); // Sanity check - - // Mark and sweep - pruner.mark(markBlock.getStateRoot()); - pruner.sweepBefore(markBlock.getNumber()); - - // Assert that the block we marked is still present and all accounts are accessible - assertThat(worldStateArchive.get(markBlock.getStateRoot())).isPresent(); - final WorldState markedState = worldStateArchive.get(markBlock.getStateRoot()).get(); - // Traverse accounts and make sure all are accessible - final int expectedAccounts = numAccounts * markBlockNumber; - final long accounts = markedState.streamAccounts(Bytes32.ZERO, expectedAccounts * 2).count(); - assertThat(accounts).isEqualTo(expectedAccounts); - // Traverse storage to ensure that all storage is accessible - markedState - .streamAccounts(Bytes32.ZERO, expectedAccounts * 2) - .forEach(a -> a.storageEntriesFrom(Bytes32.ZERO, 1000)); - - // All other state roots should have been removed - for (int i = 0; i < numBlocks; i++) { - final BlockHeader curHeader = blockchain.getBlockHeader(i + 1L).get(); - if (curHeader.getNumber() == markBlock.getNumber()) { - continue; - } - assertThat(worldStateArchive.get(curHeader.getStateRoot())).isEmpty(); - } - - // Check that storage contains only the values we expect - assertThat(hashValueStore.size()).isEqualTo(expectedNodes.size()); - assertThat(hashValueStore.values()) - .containsExactlyInAnyOrderElementsOf( - expectedNodes.stream().map(BytesValue::getArrayUnsafe).collect(Collectors.toSet())); - } - - @Test - public void sweepBefore_shouldSweepStateRootFirst() { - final MarkSweepPruner pruner = - new MarkSweepPruner(worldStateStorage, blockchain, markStorage, metricsSystem, 1); - - // Generate accounts and save corresponding state root - final int numBlocks = 15; - final int numAccounts = 10; - generateBlockchainData(numBlocks, numAccounts); - - final int markBlockNumber = 10; - final BlockHeader markBlock = blockchain.getBlockHeader(markBlockNumber).get(); - - // Collect state roots we expect to be swept first - final List stateRoots = new ArrayList<>(); - for (int i = markBlockNumber - 1; i >= 0; i--) { - stateRoots.add(blockchain.getBlockHeader(i).get().getStateRoot()); - } - - // Mark and sweep - pruner.mark(markBlock.getStateRoot()); - pruner.sweepBefore(markBlock.getNumber()); - - // Check stateRoots are marked first - InOrder inOrder = inOrder(hashValueStore, stateStorage); - for (Bytes32 stateRoot : stateRoots) { - inOrder.verify(hashValueStore).remove(stateRoot); - } - inOrder.verify(stateStorage).removeAllKeysUnless(any()); - } - - @Test - public void sweepBefore_shouldNotRemoveMarkedStateRoots() { - final MarkSweepPruner pruner = - new MarkSweepPruner(worldStateStorage, blockchain, markStorage, metricsSystem, 1); - - // Generate accounts and save corresponding state root - final int numBlocks = 15; - final int numAccounts = 10; - generateBlockchainData(numBlocks, numAccounts); - - final int markBlockNumber = 10; - final BlockHeader markBlock = blockchain.getBlockHeader(markBlockNumber).get(); - - // Collect state roots we expect to be swept first - final List stateRoots = new ArrayList<>(); - for (int i = markBlockNumber - 1; i >= 0; i--) { - stateRoots.add(blockchain.getBlockHeader(i).get().getStateRoot()); - } - - // Mark - pruner.mark(markBlock.getStateRoot()); - // Mark an extra state root - Hash markedRoot = Hash.wrap(stateRoots.remove(stateRoots.size() / 2)); - pruner.markNode(markedRoot); - // Sweep - pruner.sweepBefore(markBlock.getNumber()); - - // Check stateRoots are marked first - InOrder inOrder = inOrder(hashValueStore, stateStorage); - for (Bytes32 stateRoot : stateRoots) { - inOrder.verify(hashValueStore).remove(stateRoot); - } - inOrder.verify(stateStorage).removeAllKeysUnless(any()); - - assertThat(stateStorage.containsKey(markedRoot.getArrayUnsafe())).isTrue(); - } - private void generateBlockchainData(final int numBlocks, final int numAccounts) { Block parentBlock = blockchain.getChainHeadBlock(); for (int i = 0; i < numBlocks; i++) { @@ -298,12 +182,6 @@ private void generateBlockchainData(final int numBlocks, final int numAccounts) } } - private Set collectWorldStateNodes(final Hash stateRootHash) { - final Set nodeData = new HashSet<>(); - collectWorldStateNodes(stateRootHash, nodeData); - return nodeData; - } - private Set collectWorldStateNodes( final Hash stateRootHash, final Set collector) { final List storageRoots = new ArrayList<>(); From 781e6b3b264e54150d39c5262bd437fe8438aedb Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Sat, 21 Sep 2019 18:20:07 +0300 Subject: [PATCH 12/23] rename PruningIntegrationTest Signed-off-by: Ratan Rai Sur --- .../{PruningIntegrationTest.java => PrunerIntegrationTest.java} | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) rename ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/{PruningIntegrationTest.java => PrunerIntegrationTest.java} (99%) diff --git a/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PruningIntegrationTest.java b/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PrunerIntegrationTest.java similarity index 99% rename from ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PruningIntegrationTest.java rename to ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PrunerIntegrationTest.java index be387598f65..aef56ec0c1c 100644 --- a/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PruningIntegrationTest.java +++ b/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PrunerIntegrationTest.java @@ -52,7 +52,7 @@ import org.junit.Test; import org.mockito.InOrder; -public class PruningIntegrationTest { +public class PrunerIntegrationTest { private final BlockDataGenerator gen = new BlockDataGenerator(); private final NoOpMetricsSystem metricsSystem = new NoOpMetricsSystem(); From 9dfedda017c7cf0992054cd40d4cd7aa0853078d Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Sat, 21 Sep 2019 18:20:47 +0300 Subject: [PATCH 13/23] spotless Signed-off-by: Ratan Rai Sur --- .../besu/ethereum/worldstate/PrunerIntegrationTest.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PrunerIntegrationTest.java b/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PrunerIntegrationTest.java index aef56ec0c1c..12542863a57 100644 --- a/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PrunerIntegrationTest.java +++ b/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PrunerIntegrationTest.java @@ -16,8 +16,6 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.hyperledger.besu.ethereum.core.InMemoryStorageProvider.createInMemoryBlockchain; -import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.spy; import org.hyperledger.besu.ethereum.chain.MutableBlockchain; @@ -50,7 +48,6 @@ import java.util.stream.Collectors; import org.junit.Test; -import org.mockito.InOrder; public class PrunerIntegrationTest { From d9f4a789d18d7996c5d664e4d7c8d299cbe9faad Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Sun, 22 Sep 2019 23:09:29 +0300 Subject: [PATCH 14/23] move clear after node added listener removal Signed-off-by: Ratan Rai Sur --- .../hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java index af090862a2b..701106de8ae 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java @@ -97,8 +97,8 @@ public MarkSweepPruner( } public void prepare() { - markStorage.clear(); worldStateStorage.removeNodeAddedListener(nodeAddedListenerId); // Just in case. + markStorage.clear(); nodeAddedListenerId = worldStateStorage.addNodeAddedListener(this::markNodes); } From af5a78e5ed3611f8fbce6b54d052068132ea046b Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Mon, 23 Sep 2019 15:26:46 +0300 Subject: [PATCH 15/23] remove flush from sweep so that tests can catch pending marks bug Signed-off-by: Ratan Rai Sur --- .../ethereum/worldstate/MarkSweepPruner.java | 16 +++++++++++----- .../kvstore/InMemoryKeyValueStorage.java | 14 +++++++++++--- 2 files changed, 22 insertions(+), 8 deletions(-) diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java index 701106de8ae..b41b33ff8c5 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java @@ -99,6 +99,7 @@ public MarkSweepPruner( public void prepare() { worldStateStorage.removeNodeAddedListener(nodeAddedListenerId); // Just in case. markStorage.clear(); + pendingMarks.clear(); nodeAddedListenerId = worldStateStorage.addNodeAddedListener(this::markNodes); } @@ -123,7 +124,6 @@ public void mark(final Hash rootHash) { } public void sweepBefore(final long markedBlockNumber) { - flushPendingMarks(); sweepOperationCounter.inc(); LOG.info("Sweeping unused nodes"); // Sweep state roots first, walking backwards until we get to a state root that isn't in the @@ -138,7 +138,7 @@ public void sweepBefore(final long markedBlockNumber) { break; } - if (!markStorage.containsKey(candidateStateRootHash.getArrayUnsafe())) { + if (!isMarked(candidateStateRootHash)) { updater.removeAccountStateTrieNode(candidateStateRootHash); prunedNodeCount++; if (prunedNodeCount % operationsPerTransaction == 0) { @@ -149,15 +149,21 @@ public void sweepBefore(final long markedBlockNumber) { } updater.commit(); // Sweep non-state-root nodes - prunedNodeCount += - worldStateStorage.prune( - key -> pendingMarks.contains(Bytes32.wrap(key)) || markStorage.containsKey(key)); + prunedNodeCount += worldStateStorage.prune(this::isMarked); sweptNodesCounter.inc(prunedNodeCount); worldStateStorage.removeNodeAddedListener(nodeAddedListenerId); markStorage.clear(); LOG.info("Completed sweeping unused nodes"); } + private boolean isMarked(final Bytes32 key) { + return pendingMarks.contains(key) || markStorage.containsKey(key.getArrayUnsafe()); + } + + private boolean isMarked(final byte[] key) { + return pendingMarks.contains(Bytes32.wrap(key)) || markStorage.containsKey(key); + } + private MerklePatriciaTrie createStateTrie(final Bytes32 rootHash) { return new StoredMerklePatriciaTrie<>( worldStateStorage::getAccountStateTrieNode, diff --git a/services/kvstore/src/main/java/org/hyperledger/besu/services/kvstore/InMemoryKeyValueStorage.java b/services/kvstore/src/main/java/org/hyperledger/besu/services/kvstore/InMemoryKeyValueStorage.java index 7b8efdc7aba..f2dc02baeea 100644 --- a/services/kvstore/src/main/java/org/hyperledger/besu/services/kvstore/InMemoryKeyValueStorage.java +++ b/services/kvstore/src/main/java/org/hyperledger/besu/services/kvstore/InMemoryKeyValueStorage.java @@ -77,9 +77,17 @@ public Optional get(final byte[] key) throws StorageException { @Override public long removeAllKeysUnless(final Predicate retainCondition) throws StorageException { - long initialSize = hashValueStore.keySet().size(); - hashValueStore.keySet().removeIf(key -> !retainCondition.test(key.getArrayUnsafe())); - return initialSize - hashValueStore.keySet().size(); + long finalSize; + final Lock lock = rwLock.readLock(); + lock.lock(); + try { + long initialSize = hashValueStore.keySet().size(); + hashValueStore.keySet().removeIf(key -> !retainCondition.test(key.getArrayUnsafe())); + finalSize = initialSize - hashValueStore.keySet().size(); + } finally { + lock.unlock(); + } + return finalSize; } @Override From ed61d893ca09418c7c8d7fb88663afb15eb3086a Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Mon, 23 Sep 2019 18:49:38 +0300 Subject: [PATCH 16/23] remove unnecessary spys Signed-off-by: Ratan Rai Sur --- .../besu/ethereum/worldstate/PrunerIntegrationTest.java | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PrunerIntegrationTest.java b/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PrunerIntegrationTest.java index 12542863a57..855508f9402 100644 --- a/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PrunerIntegrationTest.java +++ b/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PrunerIntegrationTest.java @@ -16,7 +16,6 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.hyperledger.besu.ethereum.core.InMemoryStorageProvider.createInMemoryBlockchain; -import static org.mockito.Mockito.spy; import org.hyperledger.besu.ethereum.chain.MutableBlockchain; import org.hyperledger.besu.ethereum.core.Block; @@ -53,8 +52,8 @@ public class PrunerIntegrationTest { private final BlockDataGenerator gen = new BlockDataGenerator(); private final NoOpMetricsSystem metricsSystem = new NoOpMetricsSystem(); - private final Map hashValueStore = spy(new HashMap<>()); - private final InMemoryKeyValueStorage stateStorage = spy(new TestInMemoryStorage(hashValueStore)); + private final Map hashValueStore = new HashMap<>(); + private final InMemoryKeyValueStorage stateStorage = new TestInMemoryStorage(hashValueStore); private final WorldStateStorage worldStateStorage = new WorldStateKeyValueStorage(stateStorage); private final WorldStateArchive worldStateArchive = new WorldStateArchive( From 737387b865fcfcfbc6c5d9c83ae5f8dc2d67aba8 Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Mon, 23 Sep 2019 18:49:55 +0300 Subject: [PATCH 17/23] move some info level logging to debug level Signed-off-by: Ratan Rai Sur --- .../besu/ethereum/worldstate/MarkSweepPruner.java | 6 +++--- .../org/hyperledger/besu/ethereum/worldstate/Pruner.java | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java index b41b33ff8c5..951e265de7d 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPruner.java @@ -120,12 +120,12 @@ public void mark(final Hash rootHash) { markNode(node.getHash()); node.getValue().ifPresent(this::processAccountState); }); - LOG.info("Completed marking used nodes for pruning"); + LOG.debug("Completed marking used nodes for pruning"); } public void sweepBefore(final long markedBlockNumber) { sweepOperationCounter.inc(); - LOG.info("Sweeping unused nodes"); + LOG.debug("Sweeping unused nodes"); // Sweep state roots first, walking backwards until we get to a state root that isn't in the // storage long prunedNodeCount = 0; @@ -153,7 +153,7 @@ public void sweepBefore(final long markedBlockNumber) { sweptNodesCounter.inc(prunedNodeCount); worldStateStorage.removeNodeAddedListener(nodeAddedListenerId); markStorage.clear(); - LOG.info("Completed sweeping unused nodes"); + LOG.debug("Completed sweeping unused nodes"); } private boolean isMarked(final Bytes32 key) { diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/Pruner.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/Pruner.java index 054d065f74c..0b8a0373f2d 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/Pruner.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/Pruner.java @@ -90,7 +90,7 @@ private void handleNewBlock(final BlockAddedEvent event) { private void mark(final BlockHeader header) { markBlockNumber = header.getNumber(); final Hash stateRoot = header.getStateRoot(); - LOG.info( + LOG.debug( "Begin marking used nodes for pruning. Block number: {} State root: {}", markBlockNumber, stateRoot); @@ -102,7 +102,7 @@ private void mark(final BlockHeader header) { } private void sweep() { - LOG.info( + LOG.debug( "Begin sweeping unused nodes for pruning. Keeping full state for blocks {} to {}", markBlockNumber, markBlockNumber + blocksRetained); From 9a2c48fff62051d3695130327e97f9a92a0de985 Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Mon, 23 Sep 2019 18:50:25 +0300 Subject: [PATCH 18/23] streamline logic inside method with lock Signed-off-by: Ratan Rai Sur --- .../besu/services/kvstore/InMemoryKeyValueStorage.java | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/services/kvstore/src/main/java/org/hyperledger/besu/services/kvstore/InMemoryKeyValueStorage.java b/services/kvstore/src/main/java/org/hyperledger/besu/services/kvstore/InMemoryKeyValueStorage.java index f2dc02baeea..4c6614d02e5 100644 --- a/services/kvstore/src/main/java/org/hyperledger/besu/services/kvstore/InMemoryKeyValueStorage.java +++ b/services/kvstore/src/main/java/org/hyperledger/besu/services/kvstore/InMemoryKeyValueStorage.java @@ -77,17 +77,15 @@ public Optional get(final byte[] key) throws StorageException { @Override public long removeAllKeysUnless(final Predicate retainCondition) throws StorageException { - long finalSize; final Lock lock = rwLock.readLock(); lock.lock(); try { long initialSize = hashValueStore.keySet().size(); hashValueStore.keySet().removeIf(key -> !retainCondition.test(key.getArrayUnsafe())); - finalSize = initialSize - hashValueStore.keySet().size(); + return initialSize - hashValueStore.keySet().size(); } finally { lock.unlock(); } - return finalSize; } @Override From 847a59d1a1456e70718a422f3b462741d8331748 Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Tue, 24 Sep 2019 01:12:14 +0300 Subject: [PATCH 19/23] fix lock type Signed-off-by: Ratan Rai Sur --- .../besu/services/kvstore/InMemoryKeyValueStorage.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/services/kvstore/src/main/java/org/hyperledger/besu/services/kvstore/InMemoryKeyValueStorage.java b/services/kvstore/src/main/java/org/hyperledger/besu/services/kvstore/InMemoryKeyValueStorage.java index 4c6614d02e5..59ae98c626c 100644 --- a/services/kvstore/src/main/java/org/hyperledger/besu/services/kvstore/InMemoryKeyValueStorage.java +++ b/services/kvstore/src/main/java/org/hyperledger/besu/services/kvstore/InMemoryKeyValueStorage.java @@ -77,7 +77,7 @@ public Optional get(final byte[] key) throws StorageException { @Override public long removeAllKeysUnless(final Predicate retainCondition) throws StorageException { - final Lock lock = rwLock.readLock(); + final Lock lock = rwLock.writeLock(); lock.lock(); try { long initialSize = hashValueStore.keySet().size(); From e9770a26f8a4534009536d1859284b87bc3fa83d Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Tue, 24 Sep 2019 15:27:26 +0300 Subject: [PATCH 20/23] add comments explaining TestInMemoryKeyValueStorage Signed-off-by: Ratan Rai Sur --- .../besu/ethereum/worldstate/PrunerIntegrationTest.java | 1 + .../besu/ethereum/worldstate/MarkSweepPrunerTest.java | 1 + 2 files changed, 2 insertions(+) diff --git a/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PrunerIntegrationTest.java b/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PrunerIntegrationTest.java index 855508f9402..c30657d08b7 100644 --- a/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PrunerIntegrationTest.java +++ b/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PrunerIntegrationTest.java @@ -234,6 +234,7 @@ private MerklePatriciaTrie createStorageTrie(final Bytes32 Function.identity()); } + // Proxy class so that we have access to the constructor that takes our own map private static class TestInMemoryStorage extends InMemoryKeyValueStorage { public TestInMemoryStorage(final Map hashValueStore) { diff --git a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java index e71e7f4e475..e2fcddab72a 100644 --- a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java +++ b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/worldstate/MarkSweepPrunerTest.java @@ -263,6 +263,7 @@ private MerklePatriciaTrie createStorageTrie(final Bytes32 Function.identity()); } + // Proxy class so that we have access to the constructor that takes our own map private static class TestInMemoryStorage extends InMemoryKeyValueStorage { public TestInMemoryStorage(final Map hashValueStore) { From d94abfd646a849a2d89de75c744fdead6ead1187 Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Tue, 24 Sep 2019 15:52:00 +0300 Subject: [PATCH 21/23] parameterize block confirmations Signed-off-by: Ratan Rai Sur --- .../worldstate/PrunerIntegrationTest.java | 21 ++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PrunerIntegrationTest.java b/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PrunerIntegrationTest.java index c30657d08b7..4008ef9b667 100644 --- a/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PrunerIntegrationTest.java +++ b/ethereum/core/src/integration-test/java/org/hyperledger/besu/ethereum/worldstate/PrunerIntegrationTest.java @@ -64,27 +64,38 @@ public class PrunerIntegrationTest { @Test public void pruner_smallState_manyOpsPerTx() throws InterruptedException { - testPruner(3, 1, 4, 1000); + testPruner(3, 1, 1, 4, 1000); } @Test public void pruner_largeState_fewOpsPerTx() throws InterruptedException { - testPruner(2, 5, 6, 5); + testPruner(2, 5, 5, 6, 5); } @Test public void pruner_emptyBlocks() throws InterruptedException { - testPruner(5, 0, 5, 10); + testPruner(5, 0, 2, 5, 10); } @Test public void pruner_markChainhead() throws InterruptedException { - testPruner(4, 2, 10, 20); + testPruner(4, 2, 1, 10, 20); + } + + @Test + public void pruner_lowRelativeBlockConfirmations() throws InterruptedException { + testPruner(3, 2, 1, 4, 20); + } + + @Test + public void pruner_highRelativeBlockConfirmations() throws InterruptedException { + testPruner(3, 2, 9, 10, 20); } private void testPruner( final int numCycles, final int accountsPerBlock, + final long blockConfirmations, final int numBlocksToKeep, final int opsPerTransaction) throws InterruptedException { @@ -97,7 +108,7 @@ private void testPruner( markSweepPruner, blockchain, new MockExecutorService(), - new PruningConfiguration(1, numBlocksToKeep)); + new PruningConfiguration(blockConfirmations, numBlocksToKeep)); pruner.start(); From 9128b27e55e8919bef282be25b1217c1398d489c Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Tue, 24 Sep 2019 15:52:10 +0300 Subject: [PATCH 22/23] additional argument validation Signed-off-by: Ratan Rai Sur --- .../besu/ethereum/worldstate/Pruner.java | 11 +++++------ .../besu/ethereum/worldstate/PrunerTest.java | 16 ++++++++++++++++ 2 files changed, 21 insertions(+), 6 deletions(-) diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/Pruner.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/Pruner.java index 0b8a0373f2d..e407452d032 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/Pruner.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/Pruner.java @@ -14,6 +14,8 @@ */ package org.hyperledger.besu.ethereum.worldstate; +import static com.google.common.base.Preconditions.checkArgument; + import org.hyperledger.besu.ethereum.chain.BlockAddedEvent; import org.hyperledger.besu.ethereum.chain.Blockchain; import org.hyperledger.besu.ethereum.core.BlockHeader; @@ -49,12 +51,9 @@ public Pruner( this.blockchain = blockchain; this.blocksRetained = pruningConfiguration.getBlocksRetained(); this.blockConfirmations = pruningConfiguration.getBlockConfirmations(); - if (blockConfirmations < 0 || blocksRetained < 0) { - throw new IllegalArgumentException( - String.format( - "blockConfirmations and blocksRetained must be non-negative. blockConfirmations=%d, blocksRetained=%d", - blockConfirmations, blocksRetained)); - } + checkArgument( + blockConfirmations > 0 && blockConfirmations < blocksRetained, + "blockConfirmations and blocksRetained must be non-negative. blockConfirmations must be less than blockRetained."); } public void start() { diff --git a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/worldstate/PrunerTest.java b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/worldstate/PrunerTest.java index 1b13cdbcdb2..c2ef768b3a1 100644 --- a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/worldstate/PrunerTest.java +++ b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/worldstate/PrunerTest.java @@ -159,6 +159,22 @@ public void shouldRejectInvalidArguments() { mockExecutorService, new PruningConfiguration(-1, -2))) .isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy( + () -> + new Pruner( + markSweepPruner, + mockchain, + mockExecutorService, + new PruningConfiguration(10, 8))) + .isInstanceOf(IllegalArgumentException.class); + assertThatThrownBy( + () -> + new Pruner( + markSweepPruner, + mockchain, + mockExecutorService, + new PruningConfiguration(10, 10))) + .isInstanceOf(IllegalArgumentException.class); } @Test From 748ad5074be31418dc53b9b43c8335db37c25bb4 Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Tue, 24 Sep 2019 16:15:00 +0300 Subject: [PATCH 23/23] fix breakages from argument validation Signed-off-by: Ratan Rai Sur --- .../java/org/hyperledger/besu/ethereum/worldstate/Pruner.java | 2 +- .../org/hyperledger/besu/ethereum/worldstate/PrunerTest.java | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/Pruner.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/Pruner.java index e407452d032..fb8c7e5116f 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/Pruner.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/Pruner.java @@ -52,7 +52,7 @@ public Pruner( this.blocksRetained = pruningConfiguration.getBlocksRetained(); this.blockConfirmations = pruningConfiguration.getBlockConfirmations(); checkArgument( - blockConfirmations > 0 && blockConfirmations < blocksRetained, + blockConfirmations >= 0 && blockConfirmations < blocksRetained, "blockConfirmations and blocksRetained must be non-negative. blockConfirmations must be less than blockRetained."); } diff --git a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/worldstate/PrunerTest.java b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/worldstate/PrunerTest.java index c2ef768b3a1..b60fd543481 100644 --- a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/worldstate/PrunerTest.java +++ b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/worldstate/PrunerTest.java @@ -65,7 +65,7 @@ public void shouldMarkCorrectBlockAndSweep() throws InterruptedException { final Pruner pruner = new Pruner( - markSweepPruner, blockchain, mockExecutorService, new PruningConfiguration(0, 0)); + markSweepPruner, blockchain, mockExecutorService, new PruningConfiguration(0, 1)); pruner.start(); final Block block1 = appendBlockWithParent(blockchain, genesisBlock); @@ -187,7 +187,7 @@ public void shouldCleanUpPruningStrategyOnShutdown() throws InterruptedException final Pruner pruner = new Pruner( - markSweepPruner, blockchain, mockExecutorService, new PruningConfiguration(0, 0)); + markSweepPruner, blockchain, mockExecutorService, new PruningConfiguration(0, 1)); pruner.start(); pruner.stop(); verify(markSweepPruner).cleanup();