-
Notifications
You must be signed in to change notification settings - Fork 158
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
Conversation
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!
Left minor nits, the discussion regarding the soundness issue can be continued in the relevant issue.
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.
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.
/// - 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. |
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.
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?
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.
Agreed. I've kept the names the same but updated the description for KEY / VALUE
pair.
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.
The masm code looks good. Left a minor comment inline.
Working as expected downstream for the asset vault in |
755f58c
to
767a961
Compare
767a961
to
bdc7109
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, thanks!
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!
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.