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

Implement TSMT delete procedures #1046

Merged
merged 2 commits into from
Aug 29, 2023
Merged

Implement TSMT delete procedures #1046

merged 2 commits into from
Aug 29, 2023

Conversation

bobbinth
Copy link
Contributor

@bobbinth bobbinth commented Aug 17, 2023

This PR implements procedures to support insertion of empty values into TSMT. This also introduces smt::set procedure which can be used to insert either empty or non-empty values into the TSMT.

As described in #1050, there is a soundness issue in the delete procedure. Since everything works as long as the advice provider behaves correctly, I've left this issue for a future PR.

@bobbinth bobbinth marked this pull request as ready for review August 19, 2023 10:06
Copy link
Collaborator

@Al-Kindi-0 Al-Kindi-0 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!
Left minor nits, the discussion regarding the soundness issue can be continued in the relevant issue.

processor/src/advice/mod.rs Outdated Show resolved Hide resolved
processor/src/decorators/adv_stack_injectors.rs 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/tests/collections/smt.rs Outdated Show resolved Hide resolved
stdlib/tests/collections/smt.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@frisitano frisitano left a comment

Choose a reason for hiding this comment

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

Partial review complete (rust advice injectors). I believe there may be a bug regarding identification of lone siblings. Comment and supporting test case left inline. Will review the masm tomorrow.

Comment on lines 582 to 583
/// - KEY and VALUE is they key-value pair of a leaf node occupying the location of the node
/// for the specified key, if it was set in the TSMT.
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand correctly looking at:
- Leaf for another key: [d0, d1, ONE (is_leaf), ONE (key_not_set), KEY, VALUE]
then KEY, VALUE are they key and value of the leaf associated with a different key currently occupying the node. Should we document the fact it's a different KEY / VALUE pair?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I've kept the names the same but updated the description for KEY / VALUE pair.

processor/src/decorators/adv_stack_injectors.rs Outdated Show resolved Hide resolved
processor/src/decorators/adv_stack_injectors.rs Outdated Show resolved Hide resolved
processor/src/decorators/adv_stack_injectors.rs Outdated Show resolved Hide resolved
processor/src/decorators/adv_stack_injectors.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@frisitano frisitano left a comment

Choose a reason for hiding this comment

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

The masm code looks good. Left a minor comment inline.

stdlib/asm/collections/smt.masm Show resolved Hide resolved
@frisitano
Copy link
Contributor

Working as expected downstream for the asset vault in miden-base: frisitano-vault-masm-delete

@bobbinth bobbinth force-pushed the bobbin-tsmt-delete branch 2 times, most recently from 755f58c to 767a961 Compare August 29, 2023 00:11
Copy link
Contributor

@frisitano frisitano 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, thanks!

Copy link
Collaborator

@Al-Kindi-0 Al-Kindi-0 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!

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.

3 participants