-
Notifications
You must be signed in to change notification settings - Fork 160
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
feat: add stdlib smt collection #808
Conversation
40f10c5
to
30dc8c6
Compare
7e564e2
to
7eee648
Compare
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.
Thank you! This is a review of the general approach rather than a detailed review.
Overall, I think we can simplify this quite a bit and also make this much more efficient (we should be able to do executed smt::get
in less than 100 VM cycles). A few concrete thoughts:
- Instead of
MerkleSmtIndex
instruction we should introduce a decorator which would determine all the info we need for the procedure and put this info onto the advice stack. This info could include such things as: depth of the leaf, whether the leaf is a root of an empty subtree, flag values needed for non-deterministic branching, maybe something else. We can name this decoratoradv.smtget
for now. - We could then use
adv_push
(and/oradv_load
) instruction to read the data from the advice stack as needed for by the procedure. - We need to make sure that we are handling the case of missing values as I've described a while back. Specifically, if the value is not in the SMT, then we should get back
[ZERO; 4]
rather than a root of an empty subtree.
There are many ways to structure the procedure itself. Here is one example:
- execute
adv.smtget
to determine if the depth of the node and whether the node is a root of an empty subtree. - Execute
mtree_get
to read the node at the specified depth. - If we node is a root of an empty subtree, prove that (there could be a couple of ways to do that) and return
[ZERO; 4]
via the stack. - If the node is not a root of an empty subtree, "unhash" it to extract the value portion and to verify that the remaining key is correct. Then return the value via the stack.
In MASM this could look like this (though, I'm probably missing some details):
adv.smtget
adv_push # put the depth onto the operand stack
mtree_get
adv_push # get a flag indicating whether the node is a root of empty subtree
if.true
# the node is not a root of an empty subtree; verify this and extract the value
else
# the node is a root of an empty subtree; verify this and return [ZERO; 4]
end
c4bac50
to
a1ae8f0
Compare
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.
Thank you! This is much closer to what we want - but assuming I understood things correctly, it won't quite work just yet. I left some more specific comments/questions inline.
67d475c
to
61d9885
Compare
61d9885
to
e55031d
Compare
6f8986b
to
ff10457
Compare
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.
Looks good! Thank you! I think the overall structure is good now. I left a few comments inline. I commented only get_16
variant for MASM, but I'm guessing some of these comments could apply to the other variants too.
Finally, we should implement 0xPolygonMiden/crypto#119 first, and then update how advice injector works here.
a5330b0
to
ccc5a9f
Compare
f075adc
to
d08a308
Compare
d08a308
to
0aebfa0
Compare
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.
Thank you! I left a couple more test-related comments.
195e3d2
to
57bae52
Compare
601fe1a
to
d9bdfce
Compare
600f15f
to
fbdf7dc
Compare
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.
Thank you! I left a few more test-related comments inline.
fbdf7dc
to
4fdb17e
Compare
4fdb17e
to
4e9007b
Compare
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.
Looks great! Thank you! I left a couple of non-blocking comments inline.
stdlib/asm/collections/smt.masm
Outdated
#! Depth 16: 86 cycles | ||
#! Depth 32: 79 cycles | ||
#! Depth 48: 87 cycles |
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.
nit: I think we should add 6 cycles to each value: 2 for adv_push.2
online 286 and 4 for the if/else blocks.
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.
Fixed
/// After a successful operation, the advice stack will look as follows: | ||
/// - boolean flag set to `1` if a remainin key is not zero. | ||
/// - value word; will be zeroed if the tree don't contain a mapped value for the key. | ||
/// - remaining key word; will be zeroed if the tree don't contain a mapped value for the key. | ||
/// - boolean flag set to `1` if the depth is `16` or `32`. | ||
/// - boolean flag set to `1` if the depth is `16` or `48`. |
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.
nit: I'd reverse the order of bullet points so that we are describing starting at the top of the stack.
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.
Fixed
processor/src/decorators/mod.rs
Outdated
/// After a successful operation, the advice stack will look as follows: | ||
/// - boolean flag set to `1` if a remainin key is not zero. | ||
/// - value word; will be zeroed if the tree don't contain a mapped value for the key. | ||
/// - remaining key word; will be zeroed if the tree don't contain a mapped value for the key. | ||
/// - boolean flag set to `1` if the depth is `16` or `32`. | ||
/// - boolean flag set to `1` if the depth is `16` or `48`. |
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.
Similar nit as before: I'd reverse the order of bullet points.
This commit introduces a `collections` module for the `stdlib`. It contains, initially, functions to support Sparse Merkle Tree functionality. Initially, the `smt::get` is available; it will fetch the value of a key from a Sparse Merkle tree. It adds a `adv.smtget` that will push to the advice stack information about a Sparse Merkle tree keyed leaf.
3392955
to
21b64be
Compare
feat: add stdlib smt collection
This commit introduces a
collections
module for thestdlib
. It contains, initially, functions to support Sparse Merkle Tree functionality.It also adds a
mtree_smt_index
operation that will fetch a depth/index pair from the advice provider and push onto the operand stack.