From 3f4aa4bcf63f669b0603cfef2032dce22cbcbafe Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Tue, 24 Sep 2019 16:42:53 +0300 Subject: [PATCH 1/9] separate concerns more cleanly Signed-off-by: Ratan Rai Sur --- .../besu/ethereum/worldstate/MarkSweepPruner.java | 13 +++++-------- .../besu/ethereum/worldstate/Pruner.java | 2 ++ 2 files changed, 7 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 951e265de7d..6fe87297f9b 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,16 +97,9 @@ public MarkSweepPruner( } public void prepare() { - worldStateStorage.removeNodeAddedListener(nodeAddedListenerId); // Just in case. - markStorage.clear(); - pendingMarks.clear(); nodeAddedListenerId = worldStateStorage.addNodeAddedListener(this::markNodes); } - public void cleanup() { - worldStateStorage.removeNodeAddedListener(nodeAddedListenerId); - } - public void mark(final Hash rootHash) { markOperationCounter.inc(); createStateTrie(rootHash) @@ -151,9 +144,13 @@ public void sweepBefore(final long markedBlockNumber) { // Sweep non-state-root nodes prunedNodeCount += worldStateStorage.prune(this::isMarked); sweptNodesCounter.inc(prunedNodeCount); + LOG.debug("Completed sweeping unused nodes"); + } + + public void cleanup() { worldStateStorage.removeNodeAddedListener(nodeAddedListenerId); markStorage.clear(); - LOG.debug("Completed sweeping unused nodes"); + pendingMarks.clear(); } 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 fb8c7e5116f..75e35003974 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 @@ -108,6 +108,7 @@ private void sweep() { execute( () -> { pruningStrategy.sweepBefore(markBlockNumber); + pruningStrategy.cleanup(); state.compareAndSet(State.SWEEPING, State.IDLE); }); } @@ -117,6 +118,7 @@ private void execute(final Runnable action) { executorService.execute(action); } catch (final Throwable t) { LOG.error("Pruning failed", t); + pruningStrategy.cleanup(); state.set(State.IDLE); } } From 78c502d32627b2aee6c36ee853040dc04a5ef3fe Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Tue, 24 Sep 2019 17:20:39 +0300 Subject: [PATCH 2/9] restrict visibility of flush method 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 6fe87297f9b..442d6033c9b 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 @@ -207,7 +207,7 @@ private void maybeFlushPendingMarks() { } } - void flushPendingMarks() { + private void flushPendingMarks() { markLock.lock(); try { final KeyValueStorageTransaction transaction = markStorage.startTransaction(); From 7ea61ac95f28a168516e9220106c3e58d3d9ccca Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Wed, 25 Sep 2019 16:52:55 +0300 Subject: [PATCH 3/9] un-DRY because it's in a hot loop Signed-off-by: Ratan Rai Sur --- .../besu/ethereum/worldstate/MarkSweepPruner.java | 9 ++++++++- 1 file changed, 8 insertions(+), 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 442d6033c9b..29871cf522e 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 @@ -187,7 +187,14 @@ private void processAccountState(final BytesValue value) { @VisibleForTesting void markNode(final Bytes32 hash) { - markNodes(Collections.singleton(hash)); + markedNodesCounter.inc(); + markLock.lock(); + try { + pendingMarks.add(hash); + maybeFlushPendingMarks(); + } finally { + markLock.unlock(); + } } private void markNodes(final Collection nodeHashes) { From a8e373cd3e1eea86f5befdcf38473265bb888bfd Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Wed, 25 Sep 2019 16:53:13 +0300 Subject: [PATCH 4/9] remove unnecessary assignment to markBlockNumber Signed-off-by: Ratan Rai Sur --- .../java/org/hyperledger/besu/ethereum/worldstate/Pruner.java | 1 - 1 file changed, 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 75e35003974..c5d88a01279 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 @@ -87,7 +87,6 @@ private void handleNewBlock(final BlockAddedEvent event) { } private void mark(final BlockHeader header) { - markBlockNumber = header.getNumber(); final Hash stateRoot = header.getStateRoot(); LOG.debug( "Begin marking used nodes for pruning. Block number: {} State root: {}", From ee2ba7a8f1b3f5b78b868d3a9014866a1b8fea4b Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Mon, 30 Sep 2019 19:12:20 +0300 Subject: [PATCH 5/9] start pruner before full sync downloader Signed-off-by: Ratan Rai Sur --- .../hyperledger/besu/ethereum/eth/sync/DefaultSynchronizer.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/DefaultSynchronizer.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/DefaultSynchronizer.java index ad9b013a734..51004654d8d 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/DefaultSynchronizer.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/DefaultSynchronizer.java @@ -170,8 +170,8 @@ private void handleFastSyncResult(final FastSyncState result, final Throwable er } private void startFullSync() { - fullSyncDownloader.start(); maybePruner.ifPresent(Pruner::start); + fullSyncDownloader.start(); } @Override From b99f2ed44b107b5a4d486ce80a94e47df5ce57e8 Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Mon, 30 Sep 2019 19:17:13 +0300 Subject: [PATCH 6/9] move prepare to fix race condition When importing blocks quickly, there's a chance that some state from the block after the mark block makes it into the database before the node added listener is fully attached. This means that state after the mark block could be removed. This creates the node added listener once and for all before the block added event observer so the race never happens. Signed-off-by: Ratan Rai Sur --- .../java/org/hyperledger/besu/ethereum/worldstate/Pruner.java | 3 +-- 1 file changed, 1 insertion(+), 2 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 c5d88a01279..5ba38135c72 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 @@ -58,6 +58,7 @@ public Pruner( public void start() { LOG.info("Starting Pruner."); + pruningStrategy.prepare(); blockchain.observeBlockAdded((event, blockchain) -> handleNewBlock(event)); } @@ -73,7 +74,6 @@ private void handleNewBlock(final BlockAddedEvent event) { final long blockNumber = event.getBlock().getHeader().getNumber(); if (state.compareAndSet(State.IDLE, State.MARK_BLOCK_CONFIRMATIONS_AWAITING)) { - pruningStrategy.prepare(); markBlockNumber = blockNumber; } else if (blockNumber >= markBlockNumber + blockConfirmations && state.compareAndSet(State.MARK_BLOCK_CONFIRMATIONS_AWAITING, State.MARKING)) { @@ -107,7 +107,6 @@ private void sweep() { execute( () -> { pruningStrategy.sweepBefore(markBlockNumber); - pruningStrategy.cleanup(); state.compareAndSet(State.SWEEPING, State.IDLE); }); } From a0d636ff642ce57bbe3a6fda5afb007b8a5394c6 Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Mon, 30 Sep 2019 23:05:55 +0300 Subject: [PATCH 7/9] add back mark clearing Signed-off-by: Ratan Rai Sur --- .../besu/ethereum/worldstate/MarkSweepPruner.java | 5 +++++ 1 file changed, 5 insertions(+) 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 29871cf522e..d40d13d53e4 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 @@ -144,11 +144,16 @@ public void sweepBefore(final long markedBlockNumber) { // Sweep non-state-root nodes prunedNodeCount += worldStateStorage.prune(this::isMarked); sweptNodesCounter.inc(prunedNodeCount); + clearMarks(); LOG.debug("Completed sweeping unused nodes"); } public void cleanup() { worldStateStorage.removeNodeAddedListener(nodeAddedListenerId); + clearMarks(); + } + + public void clearMarks() { markStorage.clear(); pendingMarks.clear(); } From f6e1277e6b854df9bb3372238470b9137c5dd3f9 Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Wed, 2 Oct 2019 00:05:41 +0300 Subject: [PATCH 8/9] catch start/stop related bug Signed-off-by: Ratan Rai Sur --- .../besu/ethereum/worldstate/PrunerIntegrationTest.java | 4 ++++ .../org/hyperledger/besu/ethereum/worldstate/Pruner.java | 5 ++++- 2 files changed, 8 insertions(+), 1 deletion(-) 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 4008ef9b667..053a4df3a38 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 @@ -123,6 +123,10 @@ private void testPruner( generateBlockchainData(numBlockInCycle, accountsPerBlock); assertThat(pruner.getState()).isEqualByComparingTo(Pruner.State.IDLE); + // Restarting the Pruner shouldn't matter since we're idle + pruner.stop(); + pruner.start(); + // Collect the nodes we expect to keep final Set expectedNodes = new HashSet<>(); for (int i = fullyMarkedBlockNum; i <= blockchain.getChainHeadBlockNumber(); i++) { 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 5ba38135c72..aebd1988304 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 @@ -35,6 +35,7 @@ public class Pruner { private final MarkSweepPruner pruningStrategy; private final Blockchain blockchain; private final ExecutorService executorService; + private Long blockAddedObserverId; private final long blocksRetained; private final AtomicReference state = new AtomicReference<>(State.IDLE); private volatile long markBlockNumber = 0; @@ -59,11 +60,13 @@ public Pruner( public void start() { LOG.info("Starting Pruner."); pruningStrategy.prepare(); - blockchain.observeBlockAdded((event, blockchain) -> handleNewBlock(event)); + blockAddedObserverId = + blockchain.observeBlockAdded((event, blockchain) -> handleNewBlock(event)); } public void stop() throws InterruptedException { pruningStrategy.cleanup(); + blockchain.removeObserver(blockAddedObserverId); executorService.awaitTermination(10, TimeUnit.SECONDS); } From 152dd77f04f79946686521059f62e557de37ad4f Mon Sep 17 00:00:00 2001 From: Ratan Rai Sur Date: Wed, 2 Oct 2019 00:31:31 +0300 Subject: [PATCH 9/9] make first sweep when a node starts up again more efficient Signed-off-by: Ratan Rai Sur --- .../besu/ethereum/worldstate/MarkSweepPruner.java | 5 +++++ 1 file changed, 5 insertions(+) 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 d40d13d53e4..b240af77664 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,11 @@ public MarkSweepPruner( } public void prepare() { + // Optimization for the case where the previous cycle was interrupted (like the node was shut + // down). If the previous cycle was interrupted, there will be marks in the mark storage from + // last time, causing the first sweep to be smaller than it needs to be. + clearMarks(); + nodeAddedListenerId = worldStateStorage.addNodeAddedListener(this::markNodes); }