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.

Sign up for GitHub

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

Jump to bottom

Refactor NullNode and remove NullLeafNode class #73

Merged
merged 2 commits into from
Oct 2, 2024
Merged

Refactor NullNode and remove NullLeafNode class #73

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c9666c8
Refactor NullNode and remove NullLeafNode class
lu-pinto Oct 1, 2024
88a9586
fixup! Refactor NullNode and remove NullLeafNode class
lu-pinto Oct 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/main/java/org/hyperledger/besu/ethereum/trie/verkle/VerkleTrieBatchHasher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@
import org.hyperledger.besu.ethereum.trie.verkle.node.InternalNode;
import org.hyperledger.besu.ethereum.trie.verkle.node.LeafNode;
import org.hyperledger.besu.ethereum.trie.verkle.node.Node;
import org.hyperledger.besu.ethereum.trie.verkle.node.NullLeafNode;
import org.hyperledger.besu.ethereum.trie.verkle.node.NullNode;
import org.hyperledger.besu.ethereum.trie.verkle.node.StemNode;
import org.hyperledger.besu.ethereum.trie.verkle.node.StoredNode;
Expand Down Expand Up @@ -65,8 +64,7 @@ public class VerkleTrieBatchHasher {
public void addNodeToBatch(final Optional<Bytes> maybeLocation, final Node<?> node) {
maybeLocation.ifPresent(
location -> {
if ((node instanceof NullNode<?> || node instanceof NullLeafNode<?>)
&& !location.isEmpty()) {
if (node instanceof NullNode<?> && !location.isEmpty()) {
updatedNodes.remove(location);
} else {
updatedNodes.put(location, node);
Expand Down
8 changes: 2 additions & 6 deletions src/main/java/org/hyperledger/besu/ethereum/trie/verkle/factory/StoredNodeFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import org.hyperledger.besu.ethereum.trie.verkle.node.InternalNode;
import org.hyperledger.besu.ethereum.trie.verkle.node.LeafNode;
import org.hyperledger.besu.ethereum.trie.verkle.node.Node;
import org.hyperledger.besu.ethereum.trie.verkle.node.NullLeafNode;
import org.hyperledger.besu.ethereum.trie.verkle.node.NullNode;
import org.hyperledger.besu.ethereum.trie.verkle.node.StemNode;
import org.hyperledger.besu.ethereum.trie.verkle.node.StoredInternalNode;
Expand Down Expand Up @@ -52,9 +51,6 @@ enum NodeType {
* @param <V> The type of values stored in Verkle Trie nodes.
*/
public class StoredNodeFactory<V> implements NodeFactory<V> {

private final NullLeafNode<V> nullLeafNode = new NullLeafNode<V>();

private final NodeLoader nodeLoader;
private final Function<Bytes, V> valueDeserializer;
private final Boolean areCommitmentsCompressed;
Expand Down Expand Up @@ -182,7 +178,7 @@ private InternalNode<V> createInternalNode(
List<Node<V>> children = new ArrayList<>(nChild);
for (int i = 0; i < nChild; i++) {
if (scalars.get(i).compareTo(Bytes32.ZERO) == 0) {
children.add(new NullNode<V>());
children.add(NullNode.nullNode());
} else {
if (indices.containsKey((byte) i)) {
children.add(
Expand Down Expand Up @@ -227,7 +223,7 @@ StemNode<V> decodeStemNode(Bytes stem, Bytes encodedValues, Bytes32 hash) {
List<Node<V>> children = new ArrayList<>(nChild);
for (int i = 0; i < nChild; i++) {
if (values.get(i) == Bytes.EMPTY) {
children.add(nullLeafNode);
children.add(NullNode.nullLeafNode());
} else {
children.add(
createLeafNode(
Expand Down
8 changes: 4 additions & 4 deletions src/main/java/org/hyperledger/besu/ethereum/trie/verkle/node/BranchNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ public BranchNode(final Bytes location) {
new ArrayList<>() {
{
for (int i = 0; i < maxChild(); i++) {
add(new NullNode<>());
add(NullNode.nullNode());
}
}
});
Expand Down Expand Up @@ -236,7 +236,7 @@ public String print() {
public String toDot(Boolean showNullNodes) {
StringBuilder result =
new StringBuilder()
.append(getClass().getSimpleName())
.append(getName())
.append(getLocation().orElse(Bytes.EMPTY))
.append(" [label=\"B: ")
.append(getLocation().orElse(Bytes.EMPTY))
Expand All @@ -247,10 +247,10 @@ public String toDot(Boolean showNullNodes) {

for (Node<V> child : getChildren()) {
String edgeString =
getClass().getSimpleName()
getName()
+ getLocation().orElse(Bytes.EMPTY)
+ " -> "
+ child.getClass().getSimpleName()
+ child.getName()
+ child.getLocation().orElse(Bytes.EMPTY)
+ "\n";

Expand Down
6 changes: 3 additions & 3 deletions src/main/java/org/hyperledger/besu/ethereum/trie/verkle/node/InternalNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ public String print() {
public String toDot(Boolean showNullNodes) {
StringBuilder result =
new StringBuilder()
.append(getClass().getSimpleName())
.append(getName())
.append(getLocation().orElse(Bytes.EMPTY))
.append(" [label=\"I: ")
.append(getLocation().orElse(Bytes.EMPTY))
Expand All @@ -206,10 +206,10 @@ public String toDot(Boolean showNullNodes) {

for (Node<V> child : getChildren()) {
String edgeString =
getClass().getSimpleName()
getName()
+ getLocation().orElse(Bytes.EMPTY)
+ " -> "
+ child.getClass().getSimpleName()
+ child.getName()
+ child.getLocation().orElse(Bytes.EMPTY)
+ "\n";

Expand Down
4 changes: 2 additions & 2 deletions src/main/java/org/hyperledger/besu/ethereum/trie/verkle/node/LeafNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,14 +176,14 @@ public String print() {
public String toDot(Boolean showNullNodes) {
Bytes locationBytes = getLocation().orElse(Bytes.EMPTY);

return getClass().getSimpleName()
return getName()
+ locationBytes
+ " [label=\"L: "
+ locationBytes
+ "\nSuffix: "
+ Bytes.of(locationBytes.get(locationBytes.size() - 1))
+ "\"]\n"
+ getClass().getSimpleName()
+ getName()
+ locationBytes
+ " -> "
+ "Value"
Expand Down
9 changes: 9 additions & 0 deletions src/main/java/org/hyperledger/besu/ethereum/trie/verkle/node/Node.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,15 @@ public boolean isPersisted() {
*/
public abstract String toDot(Boolean showNullNodes);

/**
* Gets the name for the DOT representation for the Node.
*
* @return name for the DOT representation for the Node.
*/
public String getName() {
return getClass().getSimpleName();
}

/**
* Generates DOT representation for the Node.
*
Expand Down
131 changes: 0 additions & 131 deletions src/main/java/org/hyperledger/besu/ethereum/trie/verkle/node/NullLeafNode.java
View file
Open in desktop

This file was deleted.

61 changes: 52 additions & 9 deletions src/main/java/org/hyperledger/besu/ethereum/trie/verkle/node/NullNode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,10 +32,17 @@
*/
public class NullNode<V> extends Node<V> {

public NullNode() {
private boolean isLeaf;

private NullNode() {
super(false, true);
}

private NullNode(Optional<V> previousValue) {
super(false, true);
this.previous = previousValue;
}

/**
* Accepts a visitor for path-based operations on the node.
*
Expand Down Expand Up @@ -95,14 +102,18 @@ public void markDirty() {
dirty = true;
}

public boolean isLeaf() {
return isLeaf;
}

/**
* Get a string representation of the `NullNode`.
*
* @return A string representation indicating that it is a "NULL" node.
*/
@Override
public String print() {
return "[NULL]";
return isLeaf() ? "[NULL-LEAF]" : "[NULL]";
}

/**
Expand All @@ -115,12 +126,44 @@ public String toDot(Boolean showRepeatingEdges) {
if (!showRepeatingEdges) {
return "";
}
String result =
getClass().getSimpleName()
+ getLocation().orElse(Bytes.EMPTY)
+ " [label=\"NL: "
+ getLocation().orElse(Bytes.EMPTY)
+ "\"]\n";
return result;
return getName()
+ getLocation().orElse(Bytes.EMPTY)
+ " [label=\"NL: "
+ getLocation().orElse(Bytes.EMPTY)
+ "\"]\n";
}

@Override
public String getName() {
return isLeaf() ? "NullLeafNode" : "NullNode";
}

@SuppressWarnings("rawtypes")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for NullNode class maybe better to have the @SuppressWarnings directly for the class ? and not for each method

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we make it class wide then other accept methods might not report relevant warnings and they could be useful. I prefer to keep it locally to where we actually need to disable them

private static NullNode nullLeafNode;

@SuppressWarnings("rawtypes")
private static NullNode nullNode;

@SuppressWarnings("unchecked")
public static <T> NullNode<T> nullNode() {
if (nullNode == null) {
nullNode = new NullNode<>();
}
return nullNode;
}

@SuppressWarnings("unchecked")
public static <T> NullNode<T> nullLeafNode() {
if (nullLeafNode == null) {
nullLeafNode = new NullNode<>();
nullLeafNode.isLeaf = true;
}
return nullLeafNode;
}

public static <T> NullNode<T> nullLeafNode(Optional<T> previousValue) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe better to rename the method to createNullLeafNode or newNullLeafNode

NullNode<T> nullLeafNode = new NullNode<>(previousValue);
nullLeafNode.isLeaf = true;
return nullLeafNode;
}
}
Loading
Loading