fix: 16657: State validation fails for round 191161423 on LSE (#16757)

Fixes: #16657 Reviewed-by: Ivan Malygin <[email protected]>, Oleg Mazurov <[email protected]> Signed-off-by: Artem Ananev <[email protected]>
hashgraph · Nov 23, 2024 · 11f5f17 · 11f5f17
1 parent 6897a54
commit 11f5f17
Show file tree

Hide file tree

Showing 5 changed files with 72 additions and 32 deletions.
diff --git a/...le/src/timingSensitive/java/com/swirlds/virtual/merkle/reconnect/BreakableDataSource.java b/...le/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/.../swirlds-virtualmap/src/main/java/com/swirlds/virtualmap/internal/hash/VirtualHasher.java b/.../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<Hash> hashReader,
             final Iterator<VirtualLeafRecord<K, V>> 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<Long, ChunkHashTask> map = new HashMap<>();
         // The result task is never executed but used as an output dependency for

diff --git a/...virtualmap/src/main/java/com/swirlds/virtualmap/internal/merkle/AbstractHashListener.java b/...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/...tualmap/src/main/java/com/swirlds/virtualmap/internal/reconnect/ReconnectNodeRemover.java b/...tualmap/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<K, ?> oldExtraLeafRecord = oldRecords.findLeafRecord(p, false);
             assert oldExtraLeafRecord != null || p < oldFirstLeafPath;
             if (oldExtraLeafRecord != null) {

diff --git a/...lmap/src/timingSensitive/java/com/swirlds/virtualmap/internal/hash/VirtualHasherTest.java b/...lmap/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<TestKey, TestValue> hasher = new VirtualHasher<>();
-        final List<VirtualLeafRecord<TestKey, TestValue>> leaves = new ArrayList<>();
-        leaves.add(appleLeaf(VirtualTestBase.A_PATH));
+        final List<VirtualLeafRecord<TestKey, TestValue>> 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<VirtualLeafRecord<TestKey, TestValue>> 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");
     }
 
     /**