diff --git a/platform-sdk/swirlds-merkle/src/timingSensitive/java/com/swirlds/virtual/merkle/reconnect/BreakableDataSource.java b/platform-sdk/swirlds-merkle/src/timingSensitive/java/com/swirlds/virtual/merkle/reconnect/BreakableDataSource.java index b6b04851852c..165e6ca71d7e 100644 --- a/platform-sdk/swirlds-merkle/src/timingSensitive/java/com/swirlds/virtual/merkle/reconnect/BreakableDataSource.java +++ b/platform-sdk/swirlds-merkle/src/timingSensitive/java/com/swirlds/virtual/merkle/reconnect/BreakableDataSource.java @@ -66,7 +66,12 @@ public void saveRecords( } delegate.saveRecords( - firstLeafPath, lastLeafPath, pathHashRecordsToUpdate, leaves.stream(), leafRecordsToDelete); + firstLeafPath, + lastLeafPath, + pathHashRecordsToUpdate, + leaves.stream(), + leafRecordsToDelete, + isReconnectContext); } @Override diff --git a/platform-sdk/swirlds-virtualmap/src/main/java/com/swirlds/virtualmap/internal/hash/VirtualHasher.java b/platform-sdk/swirlds-virtualmap/src/main/java/com/swirlds/virtualmap/internal/hash/VirtualHasher.java index f7e081f98797..a1ecf0aa3ae3 100644 --- a/platform-sdk/swirlds-virtualmap/src/main/java/com/swirlds/virtualmap/internal/hash/VirtualHasher.java +++ b/platform-sdk/swirlds-virtualmap/src/main/java/com/swirlds/virtualmap/internal/hash/VirtualHasher.java @@ -287,6 +287,18 @@ private int getIndexInOut() { } } + /** + * If a dirty leaves stream is empty, returns {@code null}. If leaf path is empty, that + * is when {@code firstLeafPath} and/or {@code lastLeafPath} are zero or less, and + * dirty leaves stream is not empty, throws an {@link IllegalArgumentException}. + * + * @param hashReader A function to read hashes for clean paths + * @param sortedDirtyLeaves A stream of leaf records, sorted by path + * @param firstLeafPath First leaf path + * @param lastLeafPath Last leaf path + * @param listener Hash listener. May be null + * @param virtualMapConfig VirtualMap config + */ public Hash hash( final LongFunction hashReader, final Iterator> sortedDirtyLeaves, @@ -296,15 +308,6 @@ public Hash hash( final @NonNull VirtualMapConfig virtualMapConfig) { requireNonNull(virtualMapConfig); - // If the first or last leaf path are invalid, then there is nothing to hash. - if (firstLeafPath < 1 || lastLeafPath < 1) { - return null; - } - - if (!sortedDirtyLeaves.hasNext()) { - return null; - } - // We don't want to include null checks everywhere, so let the listener be NoopListener if null if (listener == null) { listener = @@ -313,6 +316,19 @@ public Hash hash( }; } + // Let the listener know we have started hashing. + listener.onHashingStarted(); + + if (!sortedDirtyLeaves.hasNext()) { + // Nothing to hash. + listener.onHashingCompleted(); + return null; + } else { + if ((firstLeafPath < 1) || (lastLeafPath < 1)) { + throw new IllegalArgumentException("Dirty leaves stream is not empty, but leaf path range is empty"); + } + } + this.hashReader = hashReader; this.listener = listener; this.cryptography = CryptographyHolder.get(); @@ -346,9 +362,6 @@ public Hash hash( int firstLeafRank = Path.getRank(firstLeafPath); int lastLeafRank = Path.getRank(lastLeafPath); - // Let the listener know we have started hashing. - listener.onHashingStarted(); - // This map contains all tasks created, but not scheduled for execution yet final HashMap map = new HashMap<>(); // The result task is never executed but used as an output dependency for diff --git a/platform-sdk/swirlds-virtualmap/src/main/java/com/swirlds/virtualmap/internal/merkle/AbstractHashListener.java b/platform-sdk/swirlds-virtualmap/src/main/java/com/swirlds/virtualmap/internal/merkle/AbstractHashListener.java index a8e1a8b4ae74..018192814c81 100644 --- a/platform-sdk/swirlds-virtualmap/src/main/java/com/swirlds/virtualmap/internal/merkle/AbstractHashListener.java +++ b/platform-sdk/swirlds-virtualmap/src/main/java/com/swirlds/virtualmap/internal/merkle/AbstractHashListener.java @@ -182,11 +182,11 @@ public void onHashingCompleted() { finalLeavesToFlush = leaves; leaves = null; } - if (!finalNodesToFlush.isEmpty() || !finalLeavesToFlush.isEmpty()) { - assert !flushInProgress.get() : "Flush must not be in progress when hashing is complete"; - flushInProgress.set(true); - flush(finalNodesToFlush, finalLeavesToFlush); - } + assert !flushInProgress.get() : "Flush must not be in progress when hashing is complete"; + flushInProgress.set(true); + // Nodes / leaves lists may be empty, but a flush is still needed to make sure + // all stale leaves are removed from the data source + flush(finalNodesToFlush, finalLeavesToFlush); } // Since flushes may take quite some time, this method is called outside synchronized blocks, diff --git a/platform-sdk/swirlds-virtualmap/src/main/java/com/swirlds/virtualmap/internal/reconnect/ReconnectNodeRemover.java b/platform-sdk/swirlds-virtualmap/src/main/java/com/swirlds/virtualmap/internal/reconnect/ReconnectNodeRemover.java index 813bf8a7a59b..66ff2300b7d4 100644 --- a/platform-sdk/swirlds-virtualmap/src/main/java/com/swirlds/virtualmap/internal/reconnect/ReconnectNodeRemover.java +++ b/platform-sdk/swirlds-virtualmap/src/main/java/com/swirlds/virtualmap/internal/reconnect/ReconnectNodeRemover.java @@ -21,6 +21,7 @@ import com.swirlds.virtualmap.VirtualKey; import com.swirlds.virtualmap.VirtualValue; import com.swirlds.virtualmap.datasource.VirtualLeafRecord; +import com.swirlds.virtualmap.internal.Path; import com.swirlds.virtualmap.internal.RecordAccessor; import java.util.HashSet; import java.util.Set; @@ -146,15 +147,12 @@ public synchronized void newLeafNode(final long path, final K newKey) { } public synchronized void allNodesReceived() { - if (newLastLeafPath < 0) { - // Empty teacher - return; - } - // no-op if newLastLeafPath is greater or equal to oldLastLeafPath logger.info( RECONNECT.getMarker(), "allNodesReceived(): newLastLeafPath = " + newLastLeafPath + ", oldLastLeafPath = " + oldLastLeafPath); - for (long p = newLastLeafPath + 1; p <= oldLastLeafPath; p++) { + final long firstOldStalePath = (newLastLeafPath == Path.INVALID_PATH) ? 1 : newLastLeafPath + 1; + // No-op if newLastLeafPath is greater or equal to oldLastLeafPath + for (long p = firstOldStalePath; p <= oldLastLeafPath; p++) { final VirtualLeafRecord oldExtraLeafRecord = oldRecords.findLeafRecord(p, false); assert oldExtraLeafRecord != null || p < oldFirstLeafPath; if (oldExtraLeafRecord != null) { diff --git a/platform-sdk/swirlds-virtualmap/src/timingSensitive/java/com/swirlds/virtualmap/internal/hash/VirtualHasherTest.java b/platform-sdk/swirlds-virtualmap/src/timingSensitive/java/com/swirlds/virtualmap/internal/hash/VirtualHasherTest.java index c93a41188a1d..8004eb3cb4c8 100644 --- a/platform-sdk/swirlds-virtualmap/src/timingSensitive/java/com/swirlds/virtualmap/internal/hash/VirtualHasherTest.java +++ b/platform-sdk/swirlds-virtualmap/src/timingSensitive/java/com/swirlds/virtualmap/internal/hash/VirtualHasherTest.java @@ -91,24 +91,48 @@ void emptyStreamProducesNull() { */ @Test @Tag(TestComponentTags.VMAP) - @DisplayName("Invalid leaf paths returns null for hash") - void invalidFirstOrLastLeafPathProducesNull() { + @DisplayName("Invalid leaf paths") + void invalidLeafPaths() { final TestDataSource ds = new TestDataSource(Path.INVALID_PATH, Path.INVALID_PATH); final VirtualHasher hasher = new VirtualHasher<>(); - final List> leaves = new ArrayList<>(); - leaves.add(appleLeaf(VirtualTestBase.A_PATH)); + final List> emptyLeaves = new ArrayList<>(); + // Empty dirty leaves stream -> null hash + assertNull( + hasher.hash( + ds::loadHash, emptyLeaves.iterator(), Path.INVALID_PATH, Path.INVALID_PATH, VIRTUAL_MAP_CONFIG), + "Call should have produced null"); assertNull( - hasher.hash(ds::loadHash, leaves.iterator(), Path.INVALID_PATH, 2, VIRTUAL_MAP_CONFIG), + hasher.hash(ds::loadHash, emptyLeaves.iterator(), Path.INVALID_PATH, 2, VIRTUAL_MAP_CONFIG), "Call should have produced null"); assertNull( - hasher.hash(ds::loadHash, leaves.iterator(), 1, Path.INVALID_PATH, VIRTUAL_MAP_CONFIG), + hasher.hash(ds::loadHash, emptyLeaves.iterator(), 1, Path.INVALID_PATH, VIRTUAL_MAP_CONFIG), "Call should have produced null"); assertNull( - hasher.hash(ds::loadHash, leaves.iterator(), 0, 2, VIRTUAL_MAP_CONFIG), + hasher.hash(ds::loadHash, emptyLeaves.iterator(), 0, 2, VIRTUAL_MAP_CONFIG), "Call should have produced null"); assertNull( - hasher.hash(ds::loadHash, leaves.iterator(), 1, 0, VIRTUAL_MAP_CONFIG), + hasher.hash(ds::loadHash, emptyLeaves.iterator(), 1, 0, VIRTUAL_MAP_CONFIG), "Call should have produced null"); + // Non-empty dirty leaves stream + empty leaf path range -> IllegalStateException + final List> nonEmptyLeaves = new ArrayList<>(); + nonEmptyLeaves.add(appleLeaf(VirtualTestBase.A_PATH)); + assertThrows( + IllegalArgumentException.class, + () -> hasher.hash( + ds::loadHash, + nonEmptyLeaves.iterator(), + Path.INVALID_PATH, + Path.INVALID_PATH, + VIRTUAL_MAP_CONFIG), + "Non-null leaves iterator + invalid paths should throw an exception"); + assertThrows( + IllegalArgumentException.class, + () -> hasher.hash(ds::loadHash, nonEmptyLeaves.iterator(), 0, 2, VIRTUAL_MAP_CONFIG), + "Non-null leaves iterator + invalid paths should throw an exception"); + assertThrows( + IllegalArgumentException.class, + () -> hasher.hash(ds::loadHash, nonEmptyLeaves.iterator(), 1, 0, VIRTUAL_MAP_CONFIG), + "Non-null leaves iterator + invalid paths should throw an exception"); } /**