Serialize MAST commitment inputs in forest payloads - #3294
Conversation
cf221e4 to
0c4c6f4
Compare
29e03ba to
28fd964
Compare
3452482 to
b775170
Compare
b775170 to
51b783f
Compare
fa34b31 to
cad30a2
Compare
e2efae9 to
0dc9d42
Compare
bobbinth
left a comment
There was a problem hiding this comment.
Thank you! Looks good. Not a full review but I left a few comments/questions inline. The main thing is that I didn't fully understand the approach of this PR. It seems to me that that serialized MAST forests already have all the required info - so, new sections should not be needed (both for full and sparse forests). But maybe I missed something?
| /// Sorted source root digests used as the forest commitment input. | ||
| commitment_root_digests: Vec<Word>, | ||
|
|
||
| /// Sorted full external node digests represented in this sparse forest. | ||
| dependency_digests: Vec<Word>, |
There was a problem hiding this comment.
Why do we need to keep track of this info? Specifically:
- Aren't
dependency_digestsalready covered bydigests(these are all the external nodes in the forests)? - and they are already sorted. - How are
commitment_root_digestsdifferent from the info provided byroots?
There was a problem hiding this comment.
digests, dependency_digests, roots, and commitment_root_digests are different views of the same sparse forest. digests is keyed by source MastNodeId for replay lookups, while dependency_digests is the sorted external digest list used by the dependency commitment: fields. roots stores source root IDs. commitment_root_digests stores sorted root digests. The sparse reader reads the sorted sections separately and then checks them against the roots and full external nodes: read, root check, dependency check.
There was a problem hiding this comment.
I'm still not sure I fully understand why we need these distinctions. Is your underlying assumption that we need a Sparse MAST forest to have exactly the same commitment as the source MAST forest? If so, I'm not sure we need this assumption (and relaxing it should simplify a few things).
| root_digest_offset: usize, | ||
| dependency_digest_offset: usize, |
There was a problem hiding this comment.
How is dependency_digest_offset different from external_digest_offset? Aren't these the same thing?
And same question for roots_offset and root_digest_offset.
There was a problem hiding this comment.
The offsets point to sections with different order rules. external_digest_offset points to external digests ordered by node index, so lookup can find the digest for the Nth external node: format docs. dependency_commitment_digest_offset points to the same dependency digest set sorted by digest: commitment docs. The root sections have the same split. roots_offset points to root IDs, while root_commitment_digest_offset points to sorted root digests: layout offsets. The names now mark the commitment sections explicitly.
| roots_offset: usize, | ||
| basic_block_len: usize, | ||
| external_digests_len: usize, | ||
| root_digests_len: usize, |
There was a problem hiding this comment.
I'm not sure I fully understand why we need to introduce root_digests_len. We already have roots_count and roots_offset and this gives us MAST node IDs for the roots, right? We just need to make sure the roots IDs are sorted in such a way that root digests are sorted in increasing order.
There was a problem hiding this comment.
There is no separate root digest count on the wire. The reader reads roots_count once, then uses the same count to size the root ID section and the root commitment digest section: layout read, length calculation. #3294 keeps root IDs in procedure order and writes a separate digest-sorted commitment section. That avoids adding an in-memory root-order rule here. The writer creates that sorted root digest section with sorted_root_digests: write, sort.
| //! (Commitment input sections) | ||
| //! - root node digests (`Vec<Word>`, sorted by digest) | ||
| //! - external node digests (`Vec<Word>`, sorted by digest) |
There was a problem hiding this comment.
Similar to the above comments, I'm not sure I understand why we need these new sections. How is data described on line 63 here different from the data described on line 54 above?
There was a problem hiding this comment.
The two external digest sections may contain the same values, but they support different access patterns. The structural section is ordered by node index. That follows the earlier goal that a reader can find one node without reading the whole forest. That was a goal in #2623 r2807884038, and #2726 kept the same goal for direct mmap access and size calculation: issue, meeting note. The commitment section is sorted by digest, which gives the commitment code stable input order.
There was a problem hiding this comment.
The two external digest sections may contain the same values, but they support different access patterns. The structural section is ordered by node index. That follows the earlier goal that a reader can find one node without reading the whole forest.
For external nodes, the node index doesn't matter - right? External nodes are always leaf nodes and so, we can sort them in arbitrary order. My assumption was that we'd just sort them by digest, and that this was relatively easy to do at the time when we build MAST forest. If that creates complications, we can always just sort them in memory - there is definitely no need to create two separate sections with the same data but different ordering.
cad30a2 to
e042008
Compare
0dc9d42 to
61bb5e8
Compare
|
Closing this in favor of #3334. |
On top of
#3284#3313. Fixes #3291This PR writes sorted MAST root and dependency digests into dense and sparse forest payloads. The readers now check those digests before accepting the payload.
It also rejects invalid or too large package type data.