Remove synced tips and use last valid hash #10439
Conversation
use last valid hash in removing invalid nodes.
potuz
added
Ready For Review
Merge
| if firstInvalid.parent == nil { | ||
| firstInvalid = node | ||
| } | ||
| s.nodesLock.Unlock() |
There was a problem hiding this comment.
Any reason why we can't defer s.nodesLock.Unlock() at the top?
There was a problem hiding this comment.
because removeNode needs the lock.
| lastValidIndex, ok := f.store.payloadIndices[payload] | ||
| if !ok || lastValidIndex == NonExistentNode { | ||
| f.store.nodesLock.Unlock() | ||
| return invalidRoots, errInvalidFinalizedNode |
There was a problem hiding this comment.
errInvalidFinalizedNode doesn't really imply !ok, but I don't really mind it
There was a problem hiding this comment.
yeah, here I'm assuming that the EL is not buggy, so if we don't have the LVH in forkchoice it means that it's past finalization.
| } else { | ||
| firstInvalidIndex = f.store.nodesIndices[node.root] | ||
| } |
There was a problem hiding this comment.
I really don't think this else condition is necessary. The for loop should account for it
There was a problem hiding this comment.
It is firstInvalidIndex is updated in the loop above. If LHV was found it will have the right first invalid node, but if it wasn't found in the ancestry of the invalid node, then it will have a bogus value, so we need to set it to the only invalid node we want to remove.
| invalidNode.status = invalid | ||
| invalidIndices[index] = true | ||
| invalidNode.weight = 0 |
There was a problem hiding this comment.
Why do we need to set these for invalidNode if we were just going to delete it anyway down below?
delete(f.store.nodesIndices, invalidNode.root)
delete(f.store.canonicalNodes, invalidNode.root)
delete(f.store.payloadIndices, invalidNode.payloadHash)There was a problem hiding this comment.
Because we delete the nodes from the maps, but we do not delete the node from the protoarray, so anyone accessing the nodes directly from store.nodes will have access to them, we need to mark them as invalid.
This PR removes the use of synced tips in the protoarray fork choice implementation. It also uses the last valid hash returned from the EL when removing invalid blocks from forkchoice.
Rationale
The biggest advantage of synced tips is that it provides us with an effective way of tracking optimistic status of block that may be outside of forkchoice (eg. after pruned by finalization from ForkChoice).
The disadvantage is the increased complexity added and the number of edge cases we have to deal with.
Prysm is the unique client (currently to my knowledge) that does not prune and serves non-canonical historical blocks. By removing the synced tips structure we lose the hability of knowing their optimistic status. We would have to resort to serve them as optimistic.
Fixes #10445