Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Some RandomVirtualMapReconnectTests tests fail with deep VirtualMap key validation #16754

Open
artemananiev opened this issue Nov 22, 2024 · 0 comments · May be fixed by #16824
Open

Some RandomVirtualMapReconnectTests tests fail with deep VirtualMap key validation #16754

artemananiev opened this issue Nov 22, 2024 · 0 comments · May be fixed by #16824
Assignees
Labels
Platform Data Structures Platform Reconnect Platform Virtual Map Platform Tickets pertaining to the platform Stability Tickets pertaining to the stability of the consensus node.

Comments

@artemananiev
Copy link
Member

artemananiev commented Nov 22, 2024

This issue was found while investigating #16657.

To improve reconnect tests in context of #16657 above, I added one more validation to MerkleTestUtils. Here is the patch:

diff --git a/platform-sdk/swirlds-common/src/testFixtures/java/com/swirlds/common/test/fixtures/merkle/util/MerkleTestUtils.java b/platform-sdk/swirlds-common/src/testFixtures/java/com/swirlds/common/test/fixtures/merkle/util/MerkleTestUtils.java
index 2f67a01b36..0aa8c1823e 100644
--- a/platform-sdk/swirlds-common/src/testFixtures/java/com/swirlds/common/test/fixtures/merkle/util/MerkleTestUtils.java
+++ b/platform-sdk/swirlds-common/src/testFixtures/java/com/swirlds/common/test/fixtures/merkle/util/MerkleTestUtils.java
@@ -53,6 +53,9 @@ import com.swirlds.common.threading.pool.StandardWorkGroup;
 import com.swirlds.config.api.Configuration;
 import com.swirlds.config.extensions.test.fixtures.TestConfigBuilder;
 import com.swirlds.metrics.api.Metrics;
+import com.swirlds.virtualmap.VirtualKey;
+import com.swirlds.virtualmap.VirtualMap;
+import com.swirlds.virtualmap.internal.merkle.VirtualLeafNode;
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
 import java.util.ArrayList;
@@ -909,6 +912,37 @@ public final class MerkleTestUtils {
         return !iteratorB.hasNext();
     }
 
+    /**
+     * For every virtual map in the trees and for every virtual key in the given key set, make
+     * sure either the map in both trees contains the key, or the map in both trees doesn't
+     * contain the key.
+     */
+    @SuppressWarnings({"rawtypes", "unchecked"})
+    public static boolean checkVirtualMapKeys(
+            final MerkleNode rootA, final MerkleNode rootB, final Set<VirtualKey> virtualKeys) {
+        final Iterator<MerkleNode> iteratorA = new MerkleIterator<>(rootA);
+        final Iterator<MerkleNode> iteratorB = new MerkleIterator<>(rootB);
+        while (iteratorA.hasNext()) {
+            if (!iteratorB.hasNext()) {
+                return false;
+            }
+            final MerkleNode a = iteratorA.next();
+            final MerkleNode b = iteratorB.next();
+            if (a instanceof VirtualMap vmA) {
+                if (!(b instanceof VirtualMap vmB)) {
+                    return false;
+                }
+                for (final VirtualKey key : virtualKeys) {
+                    if (vmA.containsKey(key) != vmB.containsKey(key)) {
+                        return false;
+                    }
+                }
+
+            }
+        }
+        return true;
+    }
+
     /**
      * Check if a tree has had initialize() called on each internal node.
      */
@@ -1148,6 +1182,18 @@ public final class MerkleTestUtils {
         return node != null && (node.getClassId() == 0xaf2482557cfdb6bfL || node.getClassId() == 0x499677a326fb04caL);
     }
 
+    private static Set<VirtualKey> getVirtualKeys(final MerkleNode node) {
+        final Set<VirtualKey> keys = new HashSet<>();
+        final Iterator<MerkleNode> it = new MerkleIterator<>(node);
+        while (it.hasNext()) {
+            final MerkleNode n = it.next();
+            if (n instanceof VirtualLeafNode<?, ?> leaf) {
+                keys.add(leaf.getKey());
+            }
+        }
+        return keys;
+    }
+
     /**
      * Make sure the reconnect was valid.
      *
@@ -1161,8 +1207,15 @@ public final class MerkleTestUtils {
     private static void assertReconnectValidity(
             final MerkleNode startingTree, final MerkleNode desiredTree, final MerkleNode generatedTree) {
 
+        // Checks that the trees are equal as merkle structures
         assertTrue(areTreesEqual(generatedTree, desiredTree), "reconnect should produce identical tree");
 
+        final Set<VirtualKey> allKeys = new HashSet<>();
+        allKeys.addAll(getVirtualKeys(startingTree));
+        allKeys.addAll(getVirtualKeys(desiredTree));
+        // A deeper check at VirtualMap level
+        assertTrue(checkVirtualMapKeys(generatedTree, desiredTree, allKeys));
+
         if (desiredTree != null) {
             assertNotSame(startingTree, desiredTree, "trees should be distinct objects");

What it does is it collects all virtual keys from both teacher and learner maps. After reconnect, for every virtual map in the tree, and for every collected virtual key, a check is added that the key is either present in both maps, or absent in both maps.

This new validation correctly fails for the corner case from #16657, when learner map is empty. After that fix, the validation passes fine. However, the validation fails for a different test, RandomVirtualMapReconnectTests. At least one virtual key is missing in one map and present in the other map, using virtualMap.containsKey() method. At the same time, the two maps are equal as merkle trees, which means that lookups by paths are fine, but lookups by keys are broken.

It must be a different problem than #16657, this is why it's tracked in a separate ticket.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platform Data Structures Platform Reconnect Platform Virtual Map Platform Tickets pertaining to the platform Stability Tickets pertaining to the stability of the consensus node.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants