refactor(tree): safety check for all node types #422

weiihann · 2023-12-05T14:40:20Z

This PR adds a safety check whenever looping through an InternalNode's children.

There are 2 parts to this PR:

1. Nil check
From my understanding, an InternalNode's child should be either the following: InternalNode, LeafNode, Empty, HashedNode, or UnknownNode. Hence, a child who is nil is not accepted, and should be considered an abnormal behavior. Based on the current codebase, the only place that can change an InternalNode's children to nil is the SetChild function. Hence, we should explicitly check for them and panic as soon as they are encountered.

2. Node type switch
There are plenty of places that do something like if _, ok := c.(Empty); !ok , which implies that any node type other than Empty is true. This isn't necessarily the true intention, because we also wouldn't want to deal with HashedNode and UnknownNode. Adding a node type switch ensures that we only want to deal with the correct node type (i.e. usually InternalNode and LeafNode only).

jsign

Thanks! This goes in a good direction.

I left many comments asking for some changes, and also some tangent comments for @gballet.

jsign · 2023-12-05T14:54:02Z

tree.go

@@ -608,8 +613,14 @@ func (n *InternalNode) Delete(key []byte, resolver NodeResolverFn) (bool, error)
 			// signal that this node should be deleted
 			// as well.
 			for _, c := range n.children {
-				if _, ok := c.(Empty); !ok {
+				switch c.(type) {
+				case *InternalNode, *LeafNode:


Shouldn't HashedNode be part of the first case? If HashedNode it means there's an (unresolved) internal or leaf node. (i.e: so not part of the second case but first).

jsign · 2023-12-05T14:54:49Z

tree.go

+				case UnknownNode:
+					panic(errUnknownNodeType)


I wouldn't panic here but return an error. This situation happening isn't a bug, but someone trying to operate in a state tree rebuilt from a proof.

Yeah that case is a valid tree. If anything, one should try to resolve the node.

jsign · 2023-12-05T14:57:51Z

tree.go

+				default:
+					panic(errNilNodeType)


Two points about this:

I don't think we should panic, but return an error. This might be debatable since this should never happen. But lately we usually prefer to return an error so the node can shutdown gracefully instead of panicking.

I think the error shouldn't be errNilNodeType but (the existing) errUnknownNodeType. We usually add default: like this to catch cases in the future were we add (for some reason) a new node type (e.g: OptimizedLeafNode) and we forget to handle properly in these switches. In that case, we'd be returning errNilNodeType which wouldn't be correct. (The nil type could be interpreted as a special case of errUnknownNodeType)

jsign · 2023-12-05T14:59:12Z

tree.go

+		case UnknownNode:
+			panic(errUnknownNodeType)
+		default:
+			panic(errNilNodeType)


Similar as before, I'd take UknownNode and default as errors, not panics.

nil is also valid, although deprecated. feel free to put a warning in that case, but it's no cause for error or panic.

jsign · 2023-12-05T16:47:40Z

tree.go

@@ -329,9 +329,14 @@ func (n *InternalNode) Children() []VerkleNode {

 // SetChild *replaces* the child at the given index with the given node.
 func (n *InternalNode) SetChild(i int, c VerkleNode) error {


@gballet, we should plan to remove this SetChild(...) since it opens the door for external clients to nuke a lot of invariants on how the tree is constructed.

I've looked where this is used in geth, and is for the Verkle Tree iterator to "resolve" a HashedNode and then call SetChild(...) to put the resulting Internal/Leaf node. I believe we can resolve this in a simpler way than requiring this super powerful SetChild(...) thing.

LMK your thoughts. This isn't urgent in anyway, but if makes sense we can track this by creating an issue.

yeah ok, PRs welcome :)

jsign · 2023-12-05T16:52:08Z

tree.go

+				return nil, nil, nil, err
+			}
+			n.children[i] = c
+		case *InternalNode, *LeafNode, Empty, UnknownNode:


Uhm, this UnknownNode case sounds it should be an error.

@gballet, unfortunately, I think in L920 (below) the c.Commitment() call would be incorrect. See here. An unknown node shouldn't have an empty commitment, that's not correct since we don't really know what the commitment is. (Can't assume is empty). I think we don't have other chance than panicking there. And here in this switch, separate UnkownNode case in returning an error.

yeah, or at least return an error. We don't want all the nodes on the network to panic. The error allows us to quit without destroying the db.

jsign · 2023-12-05T16:53:31Z

tree.go

@@ -972,8 +988,14 @@ func (n *InternalNode) Serialize() ([]byte, error) {
 	// Write the <bitlist>.
 	bitlist := ret[internalBitlistOffset:internalCommitmentOffset]
 	for i, c := range n.children {
-		if _, ok := c.(Empty); !ok {
+		switch c.(type) {
+		case *InternalNode, *LeafNode:


I think there's a mistake here, it should also include HashedNode right? Since that signals a leaf or internal node is present. (Note that the previous code contemplated this case correctly).

that was previously true, but now the serialization of an internal node is simply its commitment, so it doesn't matter.

jsign · 2023-12-05T16:53:38Z

tree.go

+		case UnknownNode:
+			panic(errUnknownNodeType)
+		default:
+			panic(errNilNodeType)


Return errors.

jsign · 2023-12-05T16:55:17Z

tree.go

+		case UnknownNode:
+			panic(errUnknownNodeType)
+		default:
+			panic(errNilNodeType)


Can you switch to return an error by changing this API to also return an error?
I've checked and the caller already returns an error so it can bubble it without breaking APIs for clients.

jsign · 2023-12-05T16:55:40Z

tree.go

+		case UnknownNode:
+			panic(errUnknownNodeType)
+		default:
+			panic(errNilNodeType)


Return errors.

gballet

sorry that it took me forever to review this. I don't know if you're still interested in pursuing this PR, the base branch has drifted a bit, but if you are then I'll be quicker to review it.

gballet · 2024-07-04T13:31:42Z

tree.go

@@ -329,9 +329,14 @@ func (n *InternalNode) Children() []VerkleNode {

 // SetChild *replaces* the child at the given index with the given node.
 func (n *InternalNode) SetChild(i int, c VerkleNode) error {


yeah ok, PRs welcome :)

gballet · 2024-07-04T15:42:28Z

tree.go

 					break
+				case Empty, HashedNode:


you are trying to figure out if a node is empty here, to see if it can be deleted. So Empty should definitely not panic, it should continue looping. Same thing with HashedNode, except that in this case it means that the node is not empty. So it should be part of the break case like Ignacio says.

gballet · 2024-07-04T15:43:53Z

tree.go

+				case UnknownNode:
+					panic(errUnknownNodeType)


Yeah that case is a valid tree. If anything, one should try to resolve the node.

gballet · 2024-07-04T15:46:35Z

tree.go

+				case Empty, HashedNode:
+				case UnknownNode:
+					panic(errUnknownNodeType)
+				default:


a nil is a valid input, it should not panic. You are welcome to print a debug message to point out that this is deprecated, and that the code should be fixed, but panicking is too dangerous.

gballet · 2024-07-04T15:49:46Z

tree.go

 			n.children[i] = HashedNode{}
+		case Empty, HashedNode:
+		case UnknownNode:


don't panic here!

if the node is unknown or hashed, it means that it hasn't been resolved, so we don't need to resolve it and we can safely ignore it.

if the node is empty, there is nothing to flush. This is fine, the tree is completely valid.

gballet · 2024-07-04T15:50:37Z

tree.go

+		case UnknownNode:
+			panic(errUnknownNodeType)
+		default:
+			panic(errNilNodeType)


nil is also valid, although deprecated. feel free to put a warning in that case, but it's no cause for error or panic.

gballet · 2024-07-04T16:23:50Z

tree.go

+				return nil, nil, nil, err
+			}
+			n.children[i] = c
+		case *InternalNode, *LeafNode, Empty, UnknownNode:


yeah, or at least return an error. We don't want all the nodes on the network to panic. The error allows us to quit without destroying the db.

gballet · 2024-07-04T16:24:46Z

tree.go

@@ -972,8 +988,14 @@ func (n *InternalNode) Serialize() ([]byte, error) {
 	// Write the <bitlist>.
 	bitlist := ret[internalBitlistOffset:internalCommitmentOffset]
 	for i, c := range n.children {
-		if _, ok := c.(Empty); !ok {
+		switch c.(type) {
+		case *InternalNode, *LeafNode:


that was previously true, but now the serialization of an internal node is simply its commitment, so it doesn't matter.

gballet · 2024-07-04T16:25:11Z

tree.go

+		if child == nil {
+			panic(errNilNodeType)
+		}


log a warning, don't panic

refactor(tree): safety check for all node types

4020135

jsign reviewed Dec 5, 2023

View reviewed changes

gballet requested changes Jul 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(tree): safety check for all node types #422

refactor(tree): safety check for all node types #422

weiihann commented Dec 5, 2023

jsign left a comment

jsign Dec 5, 2023

jsign Dec 5, 2023

gballet Jul 4, 2024

jsign Dec 5, 2023

jsign Dec 5, 2023

gballet Jul 4, 2024

jsign Dec 5, 2023

gballet Jul 4, 2024

jsign Dec 5, 2023

gballet Jul 4, 2024

jsign Dec 5, 2023

gballet Jul 4, 2024

jsign Dec 5, 2023

jsign Dec 5, 2023

jsign Dec 5, 2023

gballet left a comment

gballet Jul 4, 2024

gballet Jul 4, 2024

gballet Jul 4, 2024

gballet Jul 4, 2024

gballet Jul 4, 2024

gballet Jul 4, 2024

gballet Jul 4, 2024

gballet Jul 4, 2024

gballet Jul 4, 2024

		@@ -329,9 +329,14 @@ func (n *InternalNode) Children() []VerkleNode {

		// SetChild replaces the child at the given index with the given node.
		func (n *InternalNode) SetChild(i int, c VerkleNode) error {

refactor(tree): safety check for all node types #422

Are you sure you want to change the base?

refactor(tree): safety check for all node types #422

Conversation

weiihann commented Dec 5, 2023

jsign left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gballet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment