Skip to content
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

Merged
merged 1 commit into from
Apr 14, 2023
Merged

feat: add stdlib smt collection #808

merged 1 commit into from
Apr 14, 2023

Conversation

vlopes11
Copy link
Contributor

This commit introduces a collections module for the stdlib. 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.

@vlopes11 vlopes11 force-pushed the vlopes11-tsmt-masm branch 3 times, most recently from 40f10c5 to 30dc8c6 Compare March 30, 2023 00:52
@vlopes11 vlopes11 force-pushed the vlopes11-tsmt-masm branch 2 times, most recently from 7e564e2 to 7eee648 Compare March 30, 2023 00:56
Copy link
Contributor

@bobbinth bobbinth left a 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:

  1. 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 decorator adv.smtget for now.
  2. We could then use adv_push (and/or adv_load) instruction to read the data from the advice stack as needed for by the procedure.
  3. 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:

  1. execute adv.smtget to determine if the depth of the node and whether the node is a root of an empty subtree.
  2. Execute mtree_get to read the node at the specified depth.
  3. 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.
  4. 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

@vlopes11 vlopes11 force-pushed the vlopes11-tsmt-masm branch 2 times, most recently from c4bac50 to a1ae8f0 Compare March 30, 2023 20:37
@vlopes11 vlopes11 requested a review from bobbinth March 30, 2023 20:37
Copy link
Contributor

@bobbinth bobbinth left a 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.

stdlib/asm/collections/smt.masm Outdated Show resolved Hide resolved
stdlib/asm/collections/smt.masm Outdated Show resolved Hide resolved
stdlib/asm/collections/smt.masm Outdated Show resolved Hide resolved
stdlib/asm/collections/smt.masm Outdated Show resolved Hide resolved
stdlib/asm/collections/smt.masm Outdated Show resolved Hide resolved
processor/src/decorators/mod.rs Show resolved Hide resolved
miden/tests/integration/stdlib/collections/smt.rs Outdated Show resolved Hide resolved
miden/tests/integration/stdlib/collections/smt.rs Outdated Show resolved Hide resolved
miden/tests/integration/stdlib/collections/smt.rs Outdated Show resolved Hide resolved
miden/tests/integration/stdlib/collections/smt.rs Outdated Show resolved Hide resolved
@vlopes11 vlopes11 force-pushed the vlopes11-tsmt-masm branch 3 times, most recently from 67d475c to 61d9885 Compare March 31, 2023 20:51
@vlopes11 vlopes11 requested a review from bobbinth March 31, 2023 20:59
@vlopes11 vlopes11 force-pushed the vlopes11-tsmt-masm branch 2 times, most recently from 6f8986b to ff10457 Compare April 3, 2023 15:09
@vlopes11 vlopes11 requested a review from bobbinth April 3, 2023 15:10
Copy link
Contributor

@bobbinth bobbinth left a 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.

stdlib/asm/collections/smt.masm Outdated Show resolved Hide resolved
stdlib/asm/collections/smt.masm Outdated Show resolved Hide resolved
stdlib/asm/collections/smt.masm Outdated Show resolved Hide resolved
stdlib/asm/collections/smt.masm Outdated Show resolved Hide resolved
core/src/operations/decorators/advice.rs Outdated Show resolved Hide resolved
@vlopes11 vlopes11 force-pushed the vlopes11-tsmt-masm branch 3 times, most recently from a5330b0 to ccc5a9f Compare April 4, 2023 21:13
@vlopes11 vlopes11 force-pushed the vlopes11-tsmt-masm branch 2 times, most recently from f075adc to d08a308 Compare April 10, 2023 12:40
@vlopes11 vlopes11 requested a review from bobbinth April 10, 2023 12:40
Copy link
Contributor

@bobbinth bobbinth left a 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.

stdlib/tests/collections/smt.rs Outdated Show resolved Hide resolved
stdlib/tests/collections/smt.rs Outdated Show resolved Hide resolved
stdlib/tests/collections/smt.rs Outdated Show resolved Hide resolved
@vlopes11 vlopes11 force-pushed the vlopes11-tsmt-masm branch 3 times, most recently from 195e3d2 to 57bae52 Compare April 11, 2023 12:21
@vlopes11 vlopes11 requested a review from bobbinth April 11, 2023 14:19
@vlopes11 vlopes11 force-pushed the vlopes11-tsmt-masm branch 3 times, most recently from 601fe1a to d9bdfce Compare April 12, 2023 09:45
@vlopes11 vlopes11 force-pushed the vlopes11-tsmt-masm branch 5 times, most recently from 600f15f to fbdf7dc Compare April 12, 2023 13:09
Copy link
Contributor

@bobbinth bobbinth left a 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.

test-utils/src/lib.rs Outdated Show resolved Hide resolved
stdlib/tests/collections/smt.rs Outdated Show resolved Hide resolved
stdlib/tests/collections/smt.rs Outdated Show resolved Hide resolved
stdlib/tests/collections/smt.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bobbinth bobbinth left a 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.

Comment on lines 281 to 283
#! Depth 16: 86 cycles
#! Depth 32: 79 cycles
#! Depth 48: 87 cycles
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Comment on lines 64 to 65
/// 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`.
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

Comment on lines 311 to 312
/// 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`.
Copy link
Contributor

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.
@vlopes11 vlopes11 merged commit 5a5db0a into next Apr 14, 2023
@vlopes11 vlopes11 deleted the vlopes11-tsmt-masm branch April 14, 2023 10:52
hackaugusto pushed a commit that referenced this pull request Apr 14, 2023
@bobbinth bobbinth mentioned this pull request Apr 14, 2023
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants