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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions doc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ var (
errSerializeHashedNode = errors.New("trying to serialize a hashed internal node")
errInsertIntoOtherStem = errors.New("insert splits a stem where it should not happen")
errUnknownNodeType = errors.New("unknown node type detected")
errNilNodeType = errors.New("nil node type detected")
errMissingNodeInStateless = errors.New("trying to access a node that is missing from the stateless view")
errIsPOAStub = errors.New("trying to read/write a proof of absence leaf node")
)
Expand Down
98 changes: 67 additions & 31 deletions tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

yeah ok, PRs welcome :)

if c == nil {
return errNilNodeType
}

if i >= NodeWidth {
return errors.New("child index higher than node width")
}

n.children[i] = c
return nil
}
Expand Down Expand Up @@ -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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

break
case Empty, HashedNode:
Copy link
Member

Choose a reason for hiding this comment

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

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.

case UnknownNode:
panic(errUnknownNodeType)
Comment on lines +620 to +621
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

default:
Copy link
Member

Choose a reason for hiding this comment

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

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.

panic(errNilNodeType)
Comment on lines +622 to +623
Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

}
}

Expand Down Expand Up @@ -639,14 +650,20 @@ func (n *InternalNode) Flush(flush NodeFlushFn) {

n.Commit()
for i, child := range n.children {
if c, ok := child.(*InternalNode); ok {
switch c := child.(type) {
case *InternalNode:
c.Commit()
c.Flush(flushAndCapturePath)
n.children[i] = HashedNode{}
} else if c, ok := child.(*LeafNode); ok {
case *LeafNode:
c.Commit()
flushAndCapturePath(c.stem[:n.depth+1], n.children[i])
flushAndCapturePath(c.stem[:n.depth+1], c)
n.children[i] = HashedNode{}
case Empty, HashedNode:
case UnknownNode:
Copy link
Member

Choose a reason for hiding this comment

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

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.

panic(errUnknownNodeType)
default:
panic(errNilNodeType)
Comment on lines +663 to +666
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

}
}
flush(path, n)
Expand Down Expand Up @@ -875,33 +892,32 @@ func (n *InternalNode) GetProofItems(keys keylist, resolver NodeResolverFn) (*Pr
var fiPtrs [NodeWidth]*Fr
var points [NodeWidth]*Point
for i, child := range n.children {
var c VerkleNode

fiPtrs[i] = &fi[i]
if child != nil {
var c VerkleNode
if _, ok := child.(HashedNode); ok {
childpath := make([]byte, n.depth+1)
copy(childpath[:n.depth+1], keys[0][:n.depth])
childpath[n.depth] = byte(i)
if resolver == nil {
return nil, nil, nil, fmt.Errorf("no resolver for path %x", childpath)
}
serialized, err := resolver(childpath)
if err != nil {
return nil, nil, nil, fmt.Errorf("error resolving for path %x: %w", childpath, err)
}
c, err = ParseNode(serialized, n.depth+1)
if err != nil {
return nil, nil, nil, err
}
n.children[i] = c
} else {
c = child
switch child := child.(type) {
case HashedNode:
childpath := make([]byte, n.depth+1)
copy(childpath[:n.depth+1], keys[0][:n.depth])
childpath[n.depth] = byte(i)
if resolver == nil {
return nil, nil, nil, fmt.Errorf("no resolver for path %x", childpath)
}
points[i] = c.Commitment()
} else {
// TODO: add a test case to cover this scenario.
points[i] = new(Point)
serialized, err := resolver(childpath)
if err != nil {
return nil, nil, nil, fmt.Errorf("error resolving for path %x: %w", childpath, err)
}
c, err = ParseNode(serialized, n.depth+1)
if err != nil {
return nil, nil, nil, err
}
n.children[i] = c
case *InternalNode, *LeafNode, Empty, UnknownNode:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

c = child
default:
panic(errNilNodeType)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you switch to return ErrUnknownNodeType?

}
points[i] = c.Commitment()
}
if err := banderwagon.BatchMapToScalarField(fiPtrs[:], points[:]); err != nil {
return nil, nil, nil, fmt.Errorf("batch mapping to scalar fields: %s", err)
Expand Down Expand Up @@ -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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

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).

Copy link
Member

Choose a reason for hiding this comment

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

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

setBit(bitlist, i)
case Empty, HashedNode:
case UnknownNode:
panic(errUnknownNodeType)
default:
panic(errNilNodeType)
Comment on lines +995 to +998
Copy link
Collaborator

Choose a reason for hiding this comment

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

Return errors.

}
}

Expand All @@ -995,6 +1017,9 @@ func (n *InternalNode) Copy() VerkleNode {
}

for i, child := range n.children {
if child == nil {
panic(errNilNodeType)
}
Comment on lines +1020 to +1022
Copy link
Member

Choose a reason for hiding this comment

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

log a warning, don't panic

ret.children[i] = child.Copy()
}

Expand Down Expand Up @@ -1024,7 +1049,7 @@ func (n *InternalNode) toDot(parent, path string) string {

for i, child := range n.children {
if child == nil {
continue
panic(errNilNodeType)
}
ret = fmt.Sprintf("%s%s", ret, child.toDot(me, fmt.Sprintf("%s%02x", path, i)))
}
Expand Down Expand Up @@ -1719,6 +1744,11 @@ func (n *InternalNode) collectNonHashedNodes(list []VerkleNode, paths [][]byte,
copy(childpath, path)
childpath[len(path)] = byte(i)
list, paths = childNode.collectNonHashedNodes(list, paths, childpath)
case Empty, HashedNode:
case UnknownNode:
panic(errUnknownNodeType)
default:
panic(errNilNodeType)
Comment on lines +1748 to +1751
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

}
}
return list, paths
Expand All @@ -1729,8 +1759,14 @@ func (n *InternalNode) serializeInternalWithUncompressedCommitment(pointsIdx map
serialized := make([]byte, nodeTypeSize+bitlistSize+banderwagon.UncompressedSize)
bitlist := serialized[internalBitlistOffset:internalCommitmentOffset]
for i, c := range n.children {
if _, ok := c.(Empty); !ok {
switch c.(type) {
case *InternalNode, *LeafNode:
setBit(bitlist, i)
case Empty, HashedNode:
case UnknownNode:
panic(errUnknownNodeType)
default:
panic(errNilNodeType)
Comment on lines +1766 to +1769
Copy link
Collaborator

Choose a reason for hiding this comment

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

Return errors.

}
}
serialized[nodeTypeOffset] = internalRLPType
Expand Down
Loading