From 2e0747e051bfcd87cdceed2898bb9eecc38861ee Mon Sep 17 00:00:00 2001 From: Simon Dudley Date: Tue, 7 Nov 2023 06:55:35 +1000 Subject: [PATCH] Readability improvements - use Hash instead of byte[] Renames and logging Refactor test Signed-off-by: Simon Dudley --- .../BonsaiWorldStateKeyValueStorage.java | 4 +- .../bonsai/trielog/TrieLogManager.java | 2 +- .../bonsai/trielog/TrieLogPruner.java | 37 ++++---- .../bonsai/trielog/TrieLogPrunerTest.java | 94 ++++++++----------- 4 files changed, 62 insertions(+), 75 deletions(-) diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/storage/BonsaiWorldStateKeyValueStorage.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/storage/BonsaiWorldStateKeyValueStorage.java index a24ea54636d..d20c3c0b305 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/storage/BonsaiWorldStateKeyValueStorage.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/storage/BonsaiWorldStateKeyValueStorage.java @@ -340,8 +340,8 @@ public long prune(final Predicate inUseCheck) { throw new RuntimeException("Bonsai Tries do not work with pruning."); } - public boolean pruneTrieLog(final byte[] blockHashBytes) { - return trieLogStorage.tryDelete(blockHashBytes); + public boolean pruneTrieLog(final Hash blockHash) { + return trieLogStorage.tryDelete(blockHash.toArrayUnsafe()); } @Override diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/trielog/TrieLogManager.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/trielog/TrieLogManager.java index 60aff6ec2c3..c642882733e 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/trielog/TrieLogManager.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/trielog/TrieLogManager.java @@ -86,7 +86,7 @@ public synchronized void saveTrieLog( if (success) { stateUpdater.commit(); trieLogPruner.cacheForLaterPruning( - forBlockHeader.getNumber(), forBlockHeader.getBlockHash().toArrayUnsafe()); + forBlockHeader.getNumber(), forBlockHeader.getBlockHash()); trieLogPruner.pruneFromCache(); } else { stateUpdater.rollback(); diff --git a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/trielog/TrieLogPruner.java b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/trielog/TrieLogPruner.java index a3643e77f13..11f43eed6f0 100644 --- a/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/trielog/TrieLogPruner.java +++ b/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/trielog/TrieLogPruner.java @@ -22,7 +22,6 @@ import org.hyperledger.besu.ethereum.core.ProcessableBlockHeader; import java.util.ArrayList; -import java.util.Arrays; import java.util.Comparator; import java.util.List; import java.util.Optional; @@ -32,7 +31,6 @@ import com.google.common.collect.Multimap; import com.google.common.collect.TreeMultimap; -import org.apache.tuweni.bytes.Bytes; import org.apache.tuweni.bytes.Bytes32; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -50,8 +48,9 @@ public class TrieLogPruner { private final BonsaiWorldStateKeyValueStorage rootWorldStateStorage; private final Blockchain blockchain; private final long numBlocksToRetain; - private static final Multimap knownTrieLogKeysByDescendingBlockNumber = - TreeMultimap.create(Comparator.reverseOrder(), Comparator.comparingInt(Arrays::hashCode)); + + private static final Multimap trieLogBlocksAndForksByDescendingBlockNumber = + TreeMultimap.create(Comparator.reverseOrder(), Comparator.naturalOrder()); public TrieLogPruner( final BonsaiWorldStateKeyValueStorage rootWorldStateStorage, @@ -78,27 +77,27 @@ private void preloadCache() { final AtomicLong count = new AtomicLong(); trieLogKeys.forEach( blockHashAsBytes -> { - Hash hash = Hash.wrap(Bytes32.wrap(blockHashAsBytes)); - final Optional header = blockchain.getBlockHeader(hash); + Hash blockHash = Hash.wrap(Bytes32.wrap(blockHashAsBytes)); + final Optional header = blockchain.getBlockHeader(blockHash); if (header.isPresent()) { - knownTrieLogKeysByDescendingBlockNumber.put(header.get().getNumber(), blockHashAsBytes); + trieLogBlocksAndForksByDescendingBlockNumber.put(header.get().getNumber(), blockHash); count.getAndIncrement(); } else { // prune orphaned blocks (sometimes created during block production) - rootWorldStateStorage.pruneTrieLog(blockHashAsBytes); + rootWorldStateStorage.pruneTrieLog(blockHash); } }); LOG.atInfo().log("Loaded {} trie logs from database", count); pruneFromCache(); } - void cacheForLaterPruning(final long blockNumber, final byte[] trieLogKey) { + void cacheForLaterPruning(final long blockNumber, final Hash blockHash) { LOG.atTrace() - .setMessage("caching trie log for later pruning blockNumber {}; trieLogKey (blockHash) {}") + .setMessage("caching trie log for later pruning blockNumber {}; blockHash {}") .addArgument(blockNumber) - .addArgument(Bytes.wrap(trieLogKey).toHexString()) + .addArgument(blockHash) .log(); - knownTrieLogKeysByDescendingBlockNumber.put(blockNumber, trieLogKey); + trieLogBlocksAndForksByDescendingBlockNumber.put(blockNumber, blockHash); } void pruneFromCache() { @@ -114,9 +113,10 @@ void pruneFromCache() { LOG.atTrace() .setMessage( - "min((chainHeadNumber: {} - numBlocksToRetain: {}), finalized: {})) = retainAboveThisBlockOrFinalized: {}") + "min((chainHeadNumber: {} - numBlocksToRetain: {}) = {}, finalized: {})) = retainAboveThisBlockOrFinalized: {}") .addArgument(blockchain::getChainHeadBlockNumber) .addArgument(numBlocksToRetain) + .addArgument(retainAboveThisBlock) .addArgument( () -> blockchain @@ -128,7 +128,7 @@ void pruneFromCache() { .log(); final var pruneWindowEntries = - knownTrieLogKeysByDescendingBlockNumber.asMap().entrySet().stream() + trieLogBlocksAndForksByDescendingBlockNumber.asMap().entrySet().stream() .dropWhile((e) -> e.getKey() > retainAboveThisBlockOrFinalized) .limit(pruningLimit); @@ -137,14 +137,15 @@ void pruneFromCache() { final AtomicInteger count = new AtomicInteger(); pruneWindowEntries.forEach( (e) -> { - for (byte[] trieLogKey : e.getValue()) { - rootWorldStateStorage.pruneTrieLog(trieLogKey); + for (Hash blockHash : e.getValue()) { + rootWorldStateStorage.pruneTrieLog(blockHash); count.getAndIncrement(); } blockNumbersToRemove.add(e.getKey()); }); - blockNumbersToRemove.forEach(knownTrieLogKeysByDescendingBlockNumber::removeAll); + // TODO SLD could just remove each key inline? + blockNumbersToRemove.forEach(trieLogBlocksAndForksByDescendingBlockNumber::removeAll); LOG.atTrace() .setMessage("pruned {} trie logs for blocks {}") .addArgument(count) @@ -176,7 +177,7 @@ public void initialize() { } @Override - void cacheForLaterPruning(final long blockNumber, final byte[] trieLogKey) { + void cacheForLaterPruning(final long blockNumber, final Hash blockHash) { // no-op } diff --git a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/bonsai/trielog/TrieLogPrunerTest.java b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/bonsai/trielog/TrieLogPrunerTest.java index abbef430e2e..9e669e6c7e2 100644 --- a/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/bonsai/trielog/TrieLogPrunerTest.java +++ b/ethereum/core/src/test/java/org/hyperledger/besu/ethereum/bonsai/trielog/TrieLogPrunerTest.java @@ -20,6 +20,7 @@ import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; +import org.hyperledger.besu.datatypes.Hash; import org.hyperledger.besu.ethereum.bonsai.storage.BonsaiWorldStateKeyValueStorage; import org.hyperledger.besu.ethereum.chain.Blockchain; import org.hyperledger.besu.ethereum.core.BlockDataGenerator; @@ -31,6 +32,7 @@ import org.apache.logging.log4j.Level; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.core.config.Configurator; +import org.apache.tuweni.bytes.Bytes; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.mockito.InOrder; @@ -47,6 +49,27 @@ public void setup() { blockchain = Mockito.mock(Blockchain.class); } + @Test + public void initialize_preloads_cache_and_prunes_orphaned_blocks() { + // Given + int loadingLimit = 2; + final BlockDataGenerator generator = new BlockDataGenerator(); + final BlockHeader header1 = generator.header(1); + final BlockHeader header2 = generator.header(2); + when(worldState.streamTrieLogKeys(loadingLimit)) + .thenReturn(Stream.of(header1.getBlockHash().toArray(), header2.getBlockHash().toArray())); + when(blockchain.getBlockHeader(header1.getBlockHash())).thenReturn(Optional.of(header1)); + when(blockchain.getBlockHeader(header2.getBlockHash())).thenReturn(Optional.empty()); + + // When + TrieLogPruner trieLogPruner = new TrieLogPruner(worldState, blockchain, 3, loadingLimit); + trieLogPruner.initialize(); + + // Then + verify(worldState, times(1)).streamTrieLogKeys(2); + verify(worldState, times(1)).pruneTrieLog(header2.getBlockHash()); + } + @SuppressWarnings("BannedMethod") @Test public void trieLogs_pruned_in_reverse_order_within_pruning_window() { @@ -57,51 +80,35 @@ public void trieLogs_pruned_in_reverse_order_within_pruning_window() { // pruning window is below numBlocksToRetain and inside the pruningWindowSize offset. final long blocksToRetain = 3; final int pruningWindowSize = 2; + when(blockchain.getChainHeadBlockNumber()).thenReturn(5L); TrieLogPruner trieLogPruner = new TrieLogPruner(worldState, blockchain, blocksToRetain, pruningWindowSize); - final byte[] key0 = new byte[] {1, 2, 3}; // older block outside the prune window - final byte[] key1 = new byte[] {1, 2, 3}; // block inside the prune window - final byte[] key2 = new byte[] {4, 5, 6}; // same block (fork) - final byte[] key3 = new byte[] {7, 8, 9}; // different block inside the prune window - final byte[] key4 = new byte[] {10, 11, 12}; // retained block - final byte[] key5 = new byte[] {13, 14, 15}; // different retained block - final byte[] key6 = new byte[] {7, 8, 9}; // another retained block - final long block0 = 1000L; - final long block1 = 1001L; - final long block2 = 1002L; - final long block3 = 1003L; - final long block4 = 1004L; - final long block5 = 1005L; - - trieLogPruner.cacheForLaterPruning(block0, key0); // older block outside prune window - trieLogPruner.cacheForLaterPruning(block1, key1); // block inside the prune window - trieLogPruner.cacheForLaterPruning(block1, key2); // same block number (fork) - trieLogPruner.cacheForLaterPruning(block2, key3); // different block inside prune window - trieLogPruner.cacheForLaterPruning(block3, key4); // retained block - trieLogPruner.cacheForLaterPruning(block4, key5); // different retained block - trieLogPruner.cacheForLaterPruning(block5, key6); // another retained block - - when(blockchain.getChainHeadBlockNumber()).thenReturn(block5); + trieLogPruner.cacheForLaterPruning(0, key(0)); // older block outside prune window + trieLogPruner.cacheForLaterPruning(1, key(1)); // block inside the prune window + trieLogPruner.cacheForLaterPruning(1, key(2)); // same block number (fork) + trieLogPruner.cacheForLaterPruning(2, key(3)); // different block inside prune window + trieLogPruner.cacheForLaterPruning(3, key(4)); // retained block + trieLogPruner.cacheForLaterPruning(4, key(5)); // different retained block + trieLogPruner.cacheForLaterPruning(5, key(6)); // another retained block // When trieLogPruner.pruneFromCache(); // Then InOrder inOrder = Mockito.inOrder(worldState); - inOrder.verify(worldState, times(1)).pruneTrieLog(key3); - inOrder.verify(worldState, times(1)).pruneTrieLog(key1); - inOrder.verify(worldState, times(1)).pruneTrieLog(key2); + inOrder.verify(worldState, times(1)).pruneTrieLog(key(3)); + inOrder.verify(worldState, times(1)).pruneTrieLog(key(1)); // forks in order + inOrder.verify(worldState, times(1)).pruneTrieLog(key(2)); // Subsequent run should add one more block, then prune two oldest remaining keys - long block6 = 1006L; - trieLogPruner.cacheForLaterPruning(block6, new byte[] {1, 2, 3}); - when(blockchain.getChainHeadBlockNumber()).thenReturn(block6); + trieLogPruner.cacheForLaterPruning(6, key(6)); + when(blockchain.getChainHeadBlockNumber()).thenReturn(6L); trieLogPruner.pruneFromCache(); - inOrder.verify(worldState, times(1)).pruneTrieLog(key4); - inOrder.verify(worldState, times(1)).pruneTrieLog(key0); + inOrder.verify(worldState, times(1)).pruneTrieLog(key(4)); + inOrder.verify(worldState, times(1)).pruneTrieLog(key(0)); } @SuppressWarnings("BannedMethod") @@ -200,28 +207,7 @@ private TrieLogPruner setupPrunerAndFinalizedBlock( return trieLogPruner; } - @Test - public void initialize_preloads_cache_and_prunes_orphaned_blocks() { - // Given - int loadingLimit = 2; - final BlockDataGenerator generator = new BlockDataGenerator(); - final BlockHeader header1 = generator.header(1); - final BlockHeader header2 = generator.header(2); - when(worldState.streamTrieLogKeys(loadingLimit)) - .thenReturn(Stream.of(header1.getBlockHash().toArray(), header2.getBlockHash().toArray())); - when(blockchain.getBlockHeader(header1.getBlockHash())).thenReturn(Optional.of(header1)); - when(blockchain.getBlockHeader(header2.getBlockHash())).thenReturn(Optional.empty()); - - // When - TrieLogPruner trieLogPruner = new TrieLogPruner(worldState, blockchain, 3, loadingLimit); - trieLogPruner.initialize(); - - // Then - verify(worldState, times(1)).streamTrieLogKeys(2); - verify(worldState, times(1)).pruneTrieLog(header2.getBlockHash().toArray()); - } - - private byte[] key(final int k) { - return new byte[] {(byte) k}; + private Hash key(final int k) { + return Hash.hash(Bytes.of(k)); } }