-
Notifications
You must be signed in to change notification settings - Fork 81
feat: use a partial SMT proof instead of a list of merkle proofs #1158
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.
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
base: next
Are you sure you want to change the base?
Changes from all commits
0ee5721
a4137f4
87c5963
dc867f4
9e09ad4
3052e21
ff6c474
52ae0fa
b0f5e83
22ca448
cb63a8e
d4ace00
4e213ef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -138,6 +138,17 @@ message AccountProofs { | |
| message AccountProof { | ||
| // State header for public accounts. | ||
| message AccountStateHeader { | ||
| // Represents a single storage slot with the requested keys and their respective values. | ||
| message StorageSlotMap { | ||
| // The storage slot index ([0..255]). | ||
| uint32 storage_slot = 1; | ||
|
|
||
| // Merkle proofs of the map value as partial sparse merkle tree for compression. | ||
| // The respective rust types is `SparseMerkleTree` and the transformation to and from | ||
| // bytes is done via the traits `Serializable::to_bytes` and `Deserializable::from_bytes`. | ||
| bytes partial_smt = 2; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we are intending to deserialize this as Also, we've recently modified the definition of
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should be using the serialization of |
||
| } | ||
|
|
||
| // Represents a single storage slot with the requested keys and their respective values. | ||
| message StorageSlotMapProof { | ||
| // The storage slot index ([0..255]). | ||
|
|
@@ -157,8 +168,8 @@ message AccountProofs { | |
| // the current one. | ||
| optional bytes account_code = 3; | ||
|
|
||
| // Storage slots information for this account | ||
| repeated StorageSlotMapProof storage_maps = 4; | ||
| // A sparse merkle tree per storage slot, including all relevant merkle proofs for storage entries. | ||
| repeated StorageSlotMap partial_storage_smts = 5; | ||
|
Comment on lines
+171
to
+172
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: I would name this field just |
||
| } | ||
|
|
||
| // The account witness for the current state commitment of one account ID. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit out-of-the-loop here; how complex a data structure is this? Would it be easy enough to represent this within protobuf schema, similar to
types::primitives::SparseMerklePath?If kept as raw bytes I'd extend the comment to explain what actual rust type this is encoded by and how e.g. using winterfel's serde.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Effectively that'd re-implement the
Serializable for PartialSmtwith the generated types. I am not convinced that adds much value for introspectability of messages.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added a comment with the relevant info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think in this case, we can probably describe most of the structure using protobuf messages. One of the reasons is that we already have
SmtLeafmessage implemented (in theprimitives.proto). So, we'll just needInnerNodeimplemented and thenPartialSmtcould look like so:And I would put these messages into
primitives.proto.Separately, I've just realized that the way we serialize
PartialSmtright now is not entirely right. It is technically correct, but serializing all inner nodes is not necessary, and moreover, we don't check on deserialization if the data is actually correct (so, we assume that serialization was done by a trusted party). We have the same issue with many other structs/messages - so, it is not likePartialSmtis unique here - but it is something that we should start thinking about fixing.So, if there is a simple way to make the protobuf-based serialization of
PartialSmtwork correct - that would be awesome. But also, if this requires too much work - we can come back to it later.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PartialSmtis indeed serialized by adding all inner nodes, but by definition we only serialize those present. To my understanding by constructing thePartialSmtfromMerklePaths, this is guaranteed to only contain relevant inner nodes.We do check validity on the deserialization side after obtaining the
PartialSmtby doing a::opencall which would fail atget_pathif any element in the path would be missing.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll reduce the included inner-nodes to child-less (in the sense of there is no such node present in the partial smt) nodes only.
The encoding of the minimal set of nodes is done, the reconstruction is WIP.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to increased effort updating
baseto[email protected], the optimization is excluded from this PR / carved out to #1178There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather not break the API twice - so, probably makes sense to go directly to #1178.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#1178 requires
miden-baseto bump to dependencymiden-crypto, which is a bit more work, since it's still at0.15.6innext, and has a few more breaking changes 0xMiden/miden-vm#2113 and 0xMiden/miden-base#1831.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes! I'm hoping that we'll update
miden-baseto the next version of the VM some time next week.