-
Notifications
You must be signed in to change notification settings - Fork 37
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(node): avoid cloning in-memory data structures on apply_block
#532
Conversation
Co-authored-by: Mirko <[email protected]>
Co-authored-by: Mirko <[email protected]>
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 comments and a question inline.
crates/store/src/state.rs
Outdated
// We need to check that the account tree root hasn't changed while we were waiting for | ||
// the DB update task to complete. If it has changed, we mustn't proceed with in-memory | ||
// updates, since it might lead to inconsistent state. | ||
if inner.account_tree.root() != account_tree_old_root { | ||
return Err(ApplyBlockError::ConcurrentWrite); | ||
} |
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.
Why do we need this check here? If the account tree has changed somehow, wouldn't we get an error on line 344 below? Or more specifically: which scenario are we trying to prevent with this?
If we do need this check, why are we checking only the account tree and not nullifier tree as well?
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 motivation for adding this check was in possible situation, when nullifier tree was successfully updated, but update of accounts tree raised an error, this could lead to inconsistent in-memory state. Additional check before updating of nullifier tree can prevent this.
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.
Makes sense. Could we move this check closer to the update site (e.g., to line 336 below) - or is there a reason it needs to be here?
Also, I'd expand the comment to explain the motivation more (like you've done in the comment). For example, there seems to be a specific order in which we need to do updates: first nullifier tree, then account tree, and lastly MMR - so, we should add a comment that this order is meaningful and shouldn't be changed.
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.
Could we move this check closer to the update site (e.g., to line 336 below) - or is there a reason it needs to be here?
In this case we would allow database to update, but fail to update in-memory state. This is also not a good choice. By the way, we should add checking for nullifier tree root here as well in order to prevent this inconsistent between in-memory and db state. I will add it now.
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.
Done!
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 couple of minor comments inline. Once these are addressed, we can merge.
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.
All looks good! Thank you! Once CI is green, we are good to merge.
Resolves #149
In this PR we utilize new
compute_mutations
andapply_mutations
methods inSmt
in order to get rid of cloning in-memory account and nullifier trees. We also don't clone chain MMR anymore, because it wasn't necessary for adding block hash. Also we put additional checks before applying changes and reorder applying sequence due to possible inconsistent state on applying error.