FIX: bugs in visitors and nodes to make testDeleteManyValues pass (hy…

…perledger#10)

testDeleteManyValuesWithDivergentStemsAtDepth2 was added from previous MPT tests.
The test allowed to detect several bugs in visitors and nodes, that were all fixed.
Javadoc was also made to emit no warnings.

Signed-off-by: Thomas Zamojski <[email protected]>
Signed-off-by: Dragan Pilipovic <[email protected]>

src/main/java/org/hyperledger/besu/ethereum/trie/verkle/SimpleVerkleTrie.java

@@ -54,6 +54,15 @@ public SimpleVerkleTrie(Node<V> root) {
     this.root = root;
   }
 
+  /**
+   * Creates a new Verkle Trie with the specified node as the root.
+   *
+   * @param root The root node of the Verkle Trie.
+   */
+  public SimpleVerkleTrie(Optional<Node<V>> root) {
+    this.root = root.orElse(new InternalNode<V>(Bytes.EMPTY));
+  }
+
   /**
    * Retrieves the root node of the Verkle Trie.
    *

src/main/java/org/hyperledger/besu/ethereum/trie/verkle/StoredVerkleTrie.java

@@ -16,11 +16,17 @@
 package org.hyperledger.besu.ethereum.trie.verkle;
 
 import org.hyperledger.besu.ethereum.trie.verkle.factory.NodeFactory;
-import org.hyperledger.besu.ethereum.trie.verkle.node.NullNode;
 
 import org.apache.tuweni.bytes.Bytes;
 
+/**
+ * Implementation of a Verkle Trie with nodes saved in storage.
+ *
+ * @param <K> The type of keys in the Verkle Trie.
+ * @param <V> The type of values in the Verkle Trie.
+ */
 public class StoredVerkleTrie<K extends Bytes, V extends Bytes> extends SimpleVerkleTrie<K, V> {
+  /** NodeFactory that load nodes from storage */
   protected final NodeFactory<V> nodeFactory;
 
   /**
@@ -29,7 +35,7 @@ public class StoredVerkleTrie<K extends Bytes, V extends Bytes> extends SimpleVe
    * @param nodeFactory The {@link NodeFactory} to retrieve node.
    */
   public StoredVerkleTrie(final NodeFactory<V> nodeFactory) {
-    super(nodeFactory.retrieve(Bytes.EMPTY, null).orElse(NullNode.instance()));
+    super(nodeFactory.retrieve(Bytes.EMPTY, null));
     this.nodeFactory = nodeFactory;
   }
 }

src/main/java/org/hyperledger/besu/ethereum/trie/verkle/VerkleTrie.java

@@ -38,6 +38,7 @@ public interface VerkleTrie<K, V> {
    *
    * @param key The key that corresponds to the value to be updated.
    * @param value The value to associate the key with.
+   * @return Optional previous value before replacement if it exists.
    */
   Optional<V> put(K key, V value);
 

src/main/java/org/hyperledger/besu/ethereum/trie/verkle/exceptions/VerkleTrieException.java

@@ -17,22 +17,46 @@
 
 import org.apache.tuweni.bytes.Bytes;
 
+/** This exception is thrown when there is an issue retrieving or decoding values from Storage */
 public class VerkleTrieException extends RuntimeException {
+  /** Location at which the Exception occurs */
   private Bytes location;
 
+  /**
+   * Constructs a VerkleTrieException.
+   *
+   * @param message Exception's messasge.
+   */
   public VerkleTrieException(final String message) {
     super(message);
   }
 
+  /**
+   * Constructs a VerkleTrieException at location.
+   *
+   * @param message Exception's messasge.
+   * @param location Exception occured at location.
+   */
   public VerkleTrieException(final String message, final Bytes location) {
     super(message);
     this.location = location;
   }
 
+  /**
+   * Constructs a VerkleTrieException throwned from another exception.
+   *
+   * @param message Exception's messasge.
+   * @param cause Exception from which this exception was throwned.
+   */
   public VerkleTrieException(final String message, final Exception cause) {
     super(message, cause);
   }
 
+  /**
+   * Location at which the exception occured
+   *
+   * @return the location at which the exception occured
+   */
   public Bytes getLocation() {
     return location;
   }

src/main/java/org/hyperledger/besu/ethereum/trie/verkle/factory/StoredNodeFactory.java

@@ -31,10 +31,12 @@
 import org.apache.tuweni.bytes.Bytes32;
 import org.apache.tuweni.rlp.RLP;
 
+/** Node types that are saved to storage. */
 enum NodeType {
-  LEAF,
+  ROOT,
   INTERNAL,
-  STEM
+  STEM,
+  LEAF
 }
 
 /**
@@ -67,15 +69,20 @@ public StoredNodeFactory(NodeLoader nodeLoader, Function<Bytes, V> valueDeserial
    */
   @Override
   public Optional<Node<V>> retrieve(final Bytes location, final Bytes32 hash) {
+    /* Currently, Root and Leaf are distinguishable by location.
+     * To distinguish internal from stem, we further need values.
+     * Currently, they are distinguished by values length.
+     */
     Optional<Bytes> optionalEncodedValues = nodeLoader.getNode(location, hash);
     if (optionalEncodedValues.isEmpty()) {
       return Optional.empty();
     }
     Bytes encodedValues = optionalEncodedValues.get();
     List<Bytes> values = RLP.decodeToList(encodedValues, reader -> reader.readValue().copy());
+    final int locLength = location.size();
     final int nValues = values.size();
     NodeType type =
-        (nValues == 1 ? NodeType.LEAF : (nValues == 2 ? NodeType.INTERNAL : NodeType.STEM));
+        (locLength == 32 ? NodeType.LEAF : (nValues == 2 ? NodeType.INTERNAL : NodeType.STEM));
     return switch (type) {
       case LEAF -> Optional.of(createLeafNode(location, values));
       case INTERNAL -> Optional.of(createInternalNode(location, values));

src/main/java/org/hyperledger/besu/ethereum/trie/verkle/node/InternalNode.java

@@ -27,6 +27,11 @@
 import org.apache.tuweni.rlp.RLP;
 import org.apache.tuweni.rlp.RLPWriter;
 
+/**
+ * Represents an internal node in the Verkle Trie.
+ *
+ * @param <V> The type of the node's value.
+ */
 public class InternalNode<V> extends BranchNode<V> {
   private Optional<Bytes> encodedValue = Optional.empty(); // Encoded value
 
@@ -109,8 +114,9 @@ public Node<V> accept(final NodeVisitor<V> visitor) {
   /**
    * Replace the vector commitment with a new one.
    *
-   * @param hash The new vector commitment to set.
-   * @return A new BranchNode with the updated vector commitment.
+   * @param hash The new vector commitment's hash to set.
+   * @param commitment The new vector commitment to set.
+   * @return A new InternalNode with the updated vector commitment.
    */
   public Node<V> replaceHash(Bytes32 hash, Bytes32 commitment) {
     return new InternalNode<V>(
@@ -143,10 +149,10 @@ public String print() {
     final StringBuilder builder = new StringBuilder();
     builder.append("Internal:");
     for (int i = 0; i < maxChild(); i++) {
-      final Node<V> child = child((byte) i);
-      if (child == NullNode.instance()) {
-        final String label = "[" + Integer.toHexString(i) + "] ";
-        final String childRep = child.print().replaceAll("\n\t", "\n\t\t");
+      final Node<V> childNode = child((byte) i);
+      if (childNode != NullNode.instance()) {
+        final String label = String.format("[%02x] ", i);
+        final String childRep = childNode.print().replaceAll("\n\t", "\n\t\t");
         builder.append("\n\t").append(label).append(childRep);
       }
     }

src/main/java/org/hyperledger/besu/ethereum/trie/verkle/node/LeafNode.java

@@ -34,7 +34,7 @@
  */
 public class LeafNode<V> implements Node<V> {
   private final Optional<Bytes> location; // Location in the tree, or the key
-  protected final V value; // Value associated with the node
+  private final V value; // Value associated with the node
   private Optional<Bytes> encodedValue = Optional.empty(); // Encoded value
   private final Function<V, Bytes> valueSerializer; // Serializer function for the value
   private boolean dirty = true; // not persisted

src/main/java/org/hyperledger/besu/ethereum/trie/verkle/node/Node.java

@@ -32,9 +32,10 @@
  */
 public interface Node<V> {
 
-  /** A constant representing a commitment to NullNodes */
+  /** A constant representing a commitment's hash to NullNodes */
   Bytes32 EMPTY_HASH = Bytes32.ZERO;
 
+  /** A constant representing a commitment to NullNodes */
   Bytes32 EMPTY_COMMITMENT = Bytes32.ZERO;
 
   /**

src/main/java/org/hyperledger/besu/ethereum/trie/verkle/node/StemNode.java

@@ -27,6 +27,13 @@
 import org.apache.tuweni.rlp.RLP;
 import org.apache.tuweni.rlp.RLPWriter;
 
+/**
+ * Represents a stem node in the Verkle Trie.
+ *
+ * <p>StemNodes are nodes storing the stem of the key, and is the root of the suffix to value trie.
+ *
+ * @param <V> The type of the node's value.
+ */
 public class StemNode<V> extends BranchNode<V> {
   private final Node<V> NULL_LEAF_NODE = NullLeafNode.instance();
 
@@ -149,6 +156,15 @@ public Bytes getStem() {
     return stem;
   }
 
+  /**
+   * Get Node's extension.
+   *
+   * @return the extension path.
+   */
+  public Optional<Bytes> getPathExtension() {
+    return getLocation().map((loc) -> stem.slice(loc.size()));
+  }
+
   /**
    * Get the leftHash.
    *
@@ -189,6 +205,7 @@ public Optional<Bytes32> getRightCommitment() {
    * Creates a new node by replacing its location
    *
    * @param location The location in the tree.
+   * @return StemNode with new location.
    */
   public StemNode<V> replaceLocation(final Bytes location) {
     return new StemNode<V>(
@@ -212,6 +229,7 @@ public StemNode<V> replaceLocation(final Bytes location) {
    * @param leftCommitment Node's left vector commitment
    * @param rightHash Node's right vector commitment hash
    * @param rightCommitment Node's right vector commitment
+   * @return StemNode with new commitments.
    */
   public StemNode<V> replaceHash(
       final Bytes32 hash,
@@ -264,11 +282,11 @@ public Bytes getEncodedValue() {
   @Override
   public String print() {
     final StringBuilder builder = new StringBuilder();
-    builder.append("Stem:");
+    builder.append(String.format("Stem: %s", stem));
     for (int i = 0; i < maxChild(); i++) {
       final Node<V> child = child((byte) i);
-      if (child == NullLeafNode.instance()) {
-        final String label = "[" + Integer.toHexString(i) + "] ";
+      if (child != NullLeafNode.instance()) {
+        final String label = String.format("[%02x] ", i);
         final String childRep = child.print().replaceAll("\n\t", "\n\t\t");
         builder.append("\n\t").append(label).append(childRep);
       }

src/main/java/org/hyperledger/besu/ethereum/trie/verkle/node/StoredNode.java

@@ -25,13 +25,26 @@
 import org.apache.tuweni.bytes.Bytes;
 import org.apache.tuweni.bytes.Bytes32;
 
+/**
+ * Represents a regular node that can possibly be stored in storage.
+ *
+ * <p>StoredNodes wrap regular nodes and loads them lazily from storage as needed.
+ *
+ * @param <V> The type of the node's value.
+ */
 public class StoredNode<V> implements Node<V> {
   private final Bytes location;
   private final NodeFactory<V> nodeFactory;
   private Optional<Node<V>> loadedNode;
 
   private boolean dirty = true; // not persisted
 
+  /**
+   * Constructs a new StoredNode at location.
+   *
+   * @param nodeFactory The node factory for creating nodes from storage.
+   * @param location The location in the tree.
+   */
   public StoredNode(final NodeFactory<V> nodeFactory, final Bytes location) {
     this.location = location;
     this.nodeFactory = nodeFactory;
@@ -152,13 +165,19 @@ public boolean isDirty() {
   @Override
   public String print() {
     final Node<V> node = load();
-    return node.print();
+    return String.format("(stored) %s", node.print());
   }
 
   private Node<V> load() {
-    if (!loadedNode.isPresent()) {
+    if (loadedNode.isEmpty()) {
       loadedNode = nodeFactory.retrieve(location, null);
     }
-    return loadedNode.orElse(NullNode.instance());
+    if (loadedNode.isPresent()) {
+      return loadedNode.get();
+    } else if (location.size() == 32) {
+      return NullLeafNode.instance();
+    } else {
+      return NullNode.instance();
+    }
   }
 }

src/main/java/org/hyperledger/besu/ethereum/trie/verkle/visitor/FlattenVisitor.java

@@ -22,16 +22,25 @@
 
 import org.apache.tuweni.bytes.Bytes;
 
-public class FlattenVisitor<V> implements PathNodeVisitor<V> {
+/**
+ * Class representing a visitor for flattening a node in a Trie tree.
+ *
+ * <p>Flattening a node means that it is merged with its parent, adding one level to the extension
+ * path. Per current specs, only StemNodes can have extensions, so only StemNodes can potentially be
+ * flattened.
+ *
+ * @param <V> The type of node values.
+ */
+public class FlattenVisitor<V> implements NodeVisitor<V> {
   private final Node<V> NULL_NODE = NullNode.instance();
 
   @Override
-  public Node<V> visit(InternalNode<V> internalNode, Bytes path) {
+  public Node<V> visit(InternalNode<V> internalNode) {
     return NULL_NODE;
   }
 
   @Override
-  public Node<V> visit(StemNode<V> stemNode, Bytes path) {
+  public Node<V> visit(StemNode<V> stemNode) {
     final Bytes location = stemNode.getLocation().get();
     final Bytes newLocation = location.slice(0, location.size() - 1);
     // Should not flatten root node

src/main/java/org/hyperledger/besu/ethereum/trie/verkle/visitor/GetVisitor.java

@@ -53,8 +53,13 @@ public Node<V> visit(final InternalNode<V> internalNode, final Bytes path) {
    */
   @Override
   public Node<V> visit(final StemNode<V> stemNode, final Bytes path) {
-    final byte childIndex = path.get(path.size() - 1); // extract suffix
-    return stemNode.child(childIndex).accept(this, path.slice(1));
+    final Bytes extension = stemNode.getPathExtension().get();
+    final int prefix = path.commonPrefixLength(extension);
+    if (prefix < extension.size()) {
+      return NULL_NODE_RESULT;
+    }
+    final byte childIndex = path.get(prefix); // extract suffix
+    return stemNode.child(childIndex).accept(this, path.slice(prefix + 1));
   }
 
   /**

src/main/java/org/hyperledger/besu/ethereum/trie/verkle/visitor/RemoveVisitor.java

@@ -37,8 +37,9 @@
  */
 public class RemoveVisitor<V> implements PathNodeVisitor<V> {
   private final Node<V> NULL_NODE = NullNode.instance();
-  private final Node<V> NULL_LEAF_NODE = NullNode.instance();
+  private final Node<V> NULL_LEAF_NODE = NullLeafNode.instance();
   private final FlattenVisitor<V> flatten = new FlattenVisitor<>();
+  private final GetVisitor<V> getter = new GetVisitor<>();
 
   /**
    * Visits a internal node to remove a node associated with the provided path and maintain the
@@ -51,17 +52,18 @@ public class RemoveVisitor<V> implements PathNodeVisitor<V> {
   @Override
   public Node<V> visit(InternalNode<V> internalNode, Bytes path) {
     final byte index = path.get(0);
-    final Node<V> updatedChild = internalNode.child(index).accept(this, path.slice(1));
+    final Node<V> childNode = internalNode.child(index);
+    final Node<V> updatedChild = childNode.accept(this, path.slice(1));
     internalNode.replaceChild(index, updatedChild);
-    if (updatedChild.isDirty()) {
+    final boolean wasChildNullified = (childNode != NULL_NODE && updatedChild == NULL_NODE);
+    if (updatedChild.isDirty() || wasChildNullified) {
       internalNode.markDirty();
     }
     final Optional<Byte> onlyChildIndex = findOnlyChild(internalNode);
     if (onlyChildIndex.isEmpty()) {
       return internalNode;
     }
-    final Node<V> childNode = internalNode.child(onlyChildIndex.get());
-    final Node<V> newNode = childNode.accept(flatten, Bytes.of(index));
+    final Node<V> newNode = internalNode.child(onlyChildIndex.get()).accept(flatten);
     if (newNode != NULL_NODE) { // Flatten StemNode one-level up
       newNode.markDirty();
       return newNode;
@@ -153,7 +155,10 @@ Optional<Byte> findOnlyChild(final InternalNode<V> branchNode) {
   boolean allLeavesAreNull(final StemNode<V> stemNode) {
     final List<Node<V>> children = stemNode.getChildren();
     for (int i = 0; i < children.size(); ++i) {
-      if (children.get(i) != NullLeafNode.instance()) {
+      Node<V> child =
+          children.get(i).accept(getter, Bytes.EMPTY); // forces to load node if StoredNode;
+      stemNode.replaceChild((byte) i, child);
+      if (child != NULL_LEAF_NODE) {
         return false;
       }
     }