feat: use a partial SMT proof instead of a list of merkle proofs #1158
Conversation
drahnr
changed the title
drahnr
force-pushed
the
bernhard-617-batch-proof
branch
from
0b3db1c to
34e313f
Compare
| uint32 storage_slot = 1; | ||
|
|
||
| // Merkle proofs of the map value as partial sparse merkle tree for compression. | ||
| bytes partial_smt = 2; |
There was a problem hiding this comment.
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.
Effectively that'd re-implement the Serializable for PartialSmt with the generated types. I am not convinced that adds much value for introspectability of messages.
There was a problem hiding this comment.
added a comment with the relevant info
There was a problem hiding this comment.
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 SmtLeaf message implemented (in the primitives.proto). So, we'll just need InnerNode implemented and then PartialSmt could look like so:
message PartialSmt {
Digest root = 1;
repeated SmtLeaf leaves = 2;
repeated InnerNode nodes = 3;
}And I would put these messages into primitives.proto.
Separately, I've just realized that the way we serialize PartialSmt right 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 like PartialSmt is 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 PartialSmt work 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.
PartialSmt is indeed serialized by adding all inner nodes, but by definition we only serialize those present. To my understanding by constructing the PartialSmt from MerklePaths, this is guaranteed to only contain relevant inner nodes.
We do check validity on the deserialization side after obtaining the PartialSmt by doing a ::open call which would fail at get_path if any element in the path would be missing.
There was a problem hiding this comment.
Let's say that this is a partial SMT where we can open nodes 7 and 13. In the current implementation, all nodes with solid borders will be included because they are all needed to authenticate the two openings. But, we actually don't need nodes 6, 4, 14, 12, and 8 because we can compute them from the remaining nodes.
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.
Due to increased effort updating base to [email protected], the optimization is excluded from this PR / carved out to #1178
There was a problem hiding this comment.
I'd rather not break the API twice - so, probably makes sense to go directly to #1178.
There was a problem hiding this comment.
#1178 requires miden-base to bump to dependency miden-crypto, which is a bit more work, since it's still at 0.15.6 in next, and has a few more breaking changes 0xMiden/miden-vm#2113 and 0xMiden/miden-base#1831.
There was a problem hiding this comment.
Ah yes! I'm hoping that we'll update miden-base to the next version of the VM some time next week.
drahnr
force-pushed
the
bernhard-617-batch-proof
branch
from
9e973ef to
d9ea339
Compare
This reverts commit af22a32.
drahnr
force-pushed
the
bernhard-617-batch-proof
branch
from
1e57e04 to
22ca448
Compare
drahnr
changed the title
|
Not quite sure what is left to address in the scope of this PR, the inefficiencies will be addressed in #1178 (duplicate inner nodes, additional merkle path traversal on decoding for verification, ..) |
SantiagoPittella
left a comment
SantiagoPittella
left a comment
There was a problem hiding this comment.
Considering #1178 I think that this looks good.
bobbinth
left a comment
bobbinth
left a comment
There was a problem hiding this comment.
Looks good! Thank you. I left a few comments inline.
| // 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; |
There was a problem hiding this comment.
I think we are intending to deserialize this as PartialStorageMap (cc @igamigo or @SantiagoPittella to confirm). If so, I would name this message PartialStorageMap as well.
Also, we've recently modified the definition of PartialStorageMap to also include original keys (see 0xMiden/miden-base#1878) - so, serializing PartialSmt may no-longer be sufficient. So, maybe, what we should do here is serialize PartialStorageMap.
There was a problem hiding this comment.
We should be using the serialization of PartialSmt inside the impl of PartialStorageMap iiuc.
| // A sparse merkle tree per storage slot, including all relevant merkle proofs for storage entries. | ||
| repeated StorageSlotMap partial_storage_smts = 5; |
There was a problem hiding this comment.
nit: I would name this field just storage_maps.
|
Marking as blocked, since there is a lot of conflict with #1214 and that takes precedence |
Closes #617 with the carve-out of improving efficiency in #1178