Readability improvements - use Hash instead of byte[]

Renames and logging Refactor test Signed-off-by: Simon Dudley <[email protected]>
siladu · Nov 6, 2023 · 2e0747e · 2e0747e
1 parent 3a779ea
commit 2e0747e
Show file tree

Hide file tree

Showing 4 changed files with 62 additions and 75 deletions.
diff --git a/...in/java/org/hyperledger/besu/ethereum/bonsai/storage/BonsaiWorldStateKeyValueStorage.java b/...in/java/org/hyperledger/besu/ethereum/bonsai/storage/BonsaiWorldStateKeyValueStorage.java
@@ -340,8 +340,8 @@ public long prune(final Predicate<byte[]> 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
@@ -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
@@ -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<Long, byte[]> knownTrieLogKeysByDescendingBlockNumber =
-      TreeMultimap.create(Comparator.reverseOrder(), Comparator.comparingInt(Arrays::hashCode));
+
+  private static final Multimap<Long, Hash> 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<BlockHeader> header = blockchain.getBlockHeader(hash);
+          Hash blockHash = Hash.wrap(Bytes32.wrap(blockHashAsBytes));
+          final Optional<BlockHeader> 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/...um/core/src/test/java/org/hyperledger/besu/ethereum/bonsai/trielog/TrieLogPrunerTest.java b/...um/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));
   }
 }