-
Notifications
You must be signed in to change notification settings - Fork 78
Implement the history overlay mechanism #679
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
Conversation
ba26460 to
4a48680
Compare
bobbinth
left a comment
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! Not a full review, but I left some comments inline.
Overall, the structure looks good - but we may want to introduce the notion of a version where each historical delta is associated with a numerical version. This should simplify code a bit and would also align better with the use case we have in miden-node.
Also, it would be great if we could use History for AccountTreeWithHistory. This way, we'd reduce the amount of code duplication. I think the interfaces are pretty close, but we should try to make sure that they are indeed aligned.
| pub fn add_version( | ||
| &mut self, | ||
| root: Word, | ||
| nodes: NodeChanges, | ||
| leaves: LeafChanges, | ||
| ) -> Option<Delta> { |
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.
A couple of potential changes to this API:
- We could take a
MutationSetinstead ofroot,nodes, andleavesas for our specific use case, updates will be generated via mutation sets. - I would also pass in a
versionso that we could associate a given mutation set with a version. This should make pruning of history a bit easier in the future.
Combining the above, I think the signature could look something like this:
pub fn add_version(
&mut self,
version: u32,
updates: MutationSet<SMT_DEPTH, Word, Word>,
) -> Option<Delta> {
...
}aaba70f to
0df5dc9
Compare
|
Okay, I have pushed a new commit that addresses all of the feedback except the one comment that I have left unresolved. What follows is a summary of this discussion on Slack. It seems to me—and I may well be missing something—that we cannot construct the reverse delta information passed to Note that the If we want to make use of
If all you meant was to provide a |
On |
|
Not at all. The tree or forest—in other words the user-facing construct—will be able to perform this. I just don't think it's something that should live in the history manager itself. Do you still want the API to talk in terms of |
bobbinth
left a comment
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! Than you! Not a full review but I left some comments inline.
ca66629 to
00250a3
Compare
00250a3 to
20ee012
Compare
bobbinth
left a comment
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 left a few small comments inline.
This mechanism is intended to be used for the forest implementation with pluggable storage backends. This forest will eventually replace the existing in-memory forest implementation, but for now is being developed in a separate portion of the tree to avoid breakage. We are aiming to merge portions of it incrementally to avoid mammoth PRs and increase visibility, and this mechanism being relatively self-contained makes it a good candidate for this. This history mechanism is based on the following simplifying assumptions: - A forest is generally used to store multiple independent trees and versions of each tree, but does not need to share between those independent trees. - Historical versions are not important to persist, and can hence be handled entirely in memory. The commit contains the basic functionality of this history storage and overlay mechanism, and tests that it functions correctly. It is almost entirely internal code, with little change to the public interface, and as a result temporarily uses `#[allow(dead_code)]` to keep the build warnings clean. These will be removed in future commits. The interface for the `History` in this commit is a best effort at the design, and will likely need to undergo some minor changes to support concurrency as the shape of said concurrency in the forest itself is established.
This commit contains a number of changes to the `History` to address PR feedback. The major changes are: - Factoring it out into its own portion of the tree as it is now more generally applicable than just the forest. - Adding support for versions in the history items. - Simplification to the API to eliminate an unneeded feature and thereby increase maintainability.
7eaa60d to
d49799c
Compare
drahnr
left a comment
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.
Two questions, otherwise LGTM
Thank you!
d49799c to
0939968
Compare
This mechanism is intended to be used for the forest implementation with pluggable storage backends. This forest will eventually replace the existing in-memory forest implementation, but for now is being developed in a separate portion of the tree to avoid breakage. We are aiming to merge portions of it incrementally to avoid mammoth PRs and increase visibility, and this mechanism being relatively self-contained makes it a good candidate for this.
This history mechanism is based on the following simplifying assumptions:
The commit contains the basic functionality of this history storage and overlay mechanism, and tests that it functions correctly. It is almost entirely internal code, with little change to the public interface, and as a result temporarily uses
#[allow(dead_code)]to keep the build warnings clean. These will be removed in future commits.The interface for the
Historyin this commit is a best effort at the design, and will likely need to undergo some minor changes to support concurrency as the shape of said concurrency in the forest itself is established.This is the first bit of work toward #670. I would argue that this does not require a changelog entry as it is not user-facing.
Checklist before requesting a review
nextaccording to naming convention.