1/3 Serialize sparse MAST forests - #3313
Conversation
e2efae9 to
0dc9d42
Compare
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I left some comments inline - some applicable now and some probably best left for the future.
Overall, I think this works but we should be able to simplify the code quite a bit by making some opinionated assumptions and also simplifying the structure of the underlying SparseMastForest (e.g., it is not clear to me why nodes and digests are kept separately).
| /// Digests of nodes that were referenced (but not entered) during execution, keyed by their | ||
| /// original [`MastNodeId`]. Nodes present in [`Self::nodes`] are excluded from this map: a | ||
| /// full-node entry implicitly carries its own digest via [`MastNodeExt::digest`]. | ||
| digests: BTreeMap<MastNodeId, Word>, |
There was a problem hiding this comment.
Not related to this PR, but why not combine these with nodes by using ExternalNode as a substitute for nodes for which we know only digests?
There was a problem hiding this comment.
The sparse forest is a replay view. ExecutionTracer::record_visit records the executed node as FullVisit and its children as DigestOnly, because a parent row may need both child digests even when only one child runs: recording rule. External nodes are different. Execution can reach them, and the VM resolves them into a target forest before the trace row starts: external execution, tracer contract. Mixing the two would make skipped child nodes look like resolved external calls.
There was a problem hiding this comment.
I'm not sure I follow the distinction. Let's say we have node A with children B and C such that, only child B is executed. Currently, we'd record node C as just digest. But we should be able to record it just as easily as an External node. This node will never be executed and so, the VM will not try to fetch the actual underlying MAST for it, but for the purposes of executing A and B it should be sufficient.
There was a problem hiding this comment.
At the end of the day, if we decide to store non-resolved nodes as External nodes, we lose track of the reason those nodes are absent: is it because they are absent of the original forest, or because the replay did not involve them?
This is going to make debugging some issues with SparseMastForests harder: looking at the SparseMastForest as the "cache" of an execution, a cache miss becomes hard to interpret: is it that the value is missing and shouldn't be (the SparseMastForest creator messed up) or that execution has a bug and should not query at that key in the first place (the SparseMastForest user is messing up)?
That's a trade-off though, and for that, we should get something in return, namely the ability to not concern ourselves about SparseMastForests as a separate case of MastForest, since given that External encoding of absent nodes, they can masquerade (from serialization and commitment Pov) as MastForest.
Besides not having to specialize the serialization, we can avoid separate digest-sorted sections in the MastForest serialization overall: today, dense MastForest serialization can recompute digest-sorted commitment inputs from its full node data, but SparseMastForest may omit source nodes that still matter to the source forest’s dependency commitment, so sparse serialization needs an explicit source dependency digest list, or an equivalent field, to prove that the sparse payload commits to the same external dependencies as the full source forest — using External nodes for that guarantees a simpler model from a commitment PoV.
This approach doe not have your expressed preference.
There was a problem hiding this comment.
Kept digest-only entries. Digest-only entries are skipped children in the same source forest. ExecutionTracer::record_visit records the executed node as full and records its immediate children as digest-only, because a parent row may need child digests even when execution enters only one child: recording rule. This also keeps accidental entry into a pruned child as a get_node_by_id miss.
0dc9d42 to
61bb5e8
Compare
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you! I've left some replies inline. Overall, this is fine to merge as is, but I do think we've overcomplicated both the SparseMerkleTree and its serialization quite a bit. This would be good to fix in the future. Let's create an issue for this.
2b6b237 to
a874e21
Compare
cd2feb1 to
d094857
Compare
Part of #3235.
This PR adds trusted sparse MAST forest serialization.
Sparse payloads preserve source node IDs, full visited nodes, digest-only entries, roots, the advice map, and the source forest commitment.
Dense MAST readers reject sparse payloads. Sparse readers reject dense payloads, trailing bytes, invalid node IDs, duplicate sparse entries, missing child digests, and full-node digest overlap.
This PR does not serialize trace proving inputs yet.