-
Notifications
You must be signed in to change notification settings - Fork 105
refactor(store): get account vault details from account state forest #1981
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
base: main
Are you sure you want to change the base?
Changes from all commits
14941a8
86b95f7
98d53a7
d8e3fa2
e9091dd
6f3a859
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -744,11 +744,17 @@ impl State { | |||||||||||||||||||||||
| Some(commitment) if commitment == account_header.vault_root() => { | ||||||||||||||||||||||||
| AccountVaultDetails::empty() | ||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||
| Some(_) => { | ||||||||||||||||||||||||
| let vault_assets = | ||||||||||||||||||||||||
| self.db.select_account_vault_at_block(account_id, block_num).await?; | ||||||||||||||||||||||||
|
Comment on lines
-748
to
-749
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we still need
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's still used by tests checking that |
||||||||||||||||||||||||
| AccountVaultDetails::from_assets(vault_assets) | ||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||
| Some(_) => self | ||||||||||||||||||||||||
| .forest | ||||||||||||||||||||||||
| .read() | ||||||||||||||||||||||||
| .instrument(tracing::info_span!("acquire_forest_for_vault")) | ||||||||||||||||||||||||
| .await | ||||||||||||||||||||||||
| .get_vault_details(account_id, block_num) | ||||||||||||||||||||||||
| .map_err(|err| { | ||||||||||||||||||||||||
| DatabaseError::DataCorrupted(format!( | ||||||||||||||||||||||||
| "failed to reconstruct vault for account {account_id} at block {block_num}: {err}" | ||||||||||||||||||||||||
| )) | ||||||||||||||||||||||||
|
Comment on lines
+753
to
+756
|
||||||||||||||||||||||||
| .map_err(|err| { | |
| DatabaseError::DataCorrupted(format!( | |
| "failed to reconstruct vault for account {account_id} at block {block_num}: {err}" | |
| )) | |
| .map_err(|err| match err { | |
| WitnessError::MerkleError(err) => DatabaseError::MerkleError(err), | |
| WitnessError::AssetError(err) => DatabaseError::AssetError(err), | |
| WitnessError::StorageMapError(err) => DatabaseError::StorageMapError(err), | |
| WitnessError::RootNotFound(root) => DatabaseError::DataCorrupted(format!( | |
| "failed to reconstruct vault for account {account_id} at block {block_num}: missing root {root}" | |
| )), |
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.
get_vault_details()collects all vault entries into aVecbefore applyingAccountVaultDetails::MAX_RETURN_ENTRIES. For large vaults this can allocate and parse far more than needed (and holds the forest read-lock longer), even though the response will beLimitExceeded. Consider iterating with an early cutoff (e.g., stop afterMAX_RETURN_ENTRIES + 1entries) and returnLimitExceededwithout collecting the full vault.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.
I think this is a valid concern and the way to deal with this is to first check how many entries a given tree has, and iterate over the entries only if the number of entries is under the threshold. Checking the number of entries used to be expensive, but with 0xMiden/crypto#916 it should now be much cheaper (though, I don't remember if 0xMiden/crypto#916 made it into v0.24 release of
miden-cryptoor not).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.
That optimization is in v0.24, but we're still on miden-crypto v0.23 with
mainAFAIU. Does that mean that theentry_count()call is too expensive to use as a pre-check here?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 v0.23 implementation iterates over all entries to get the count - so, using it would result in two passes over the entries here in most cases. We could try to take this hit now. We could also merge this code into
mainand when porting back intonextupdate it to use theentry_count()which will become efficient once we update to the next version ofmiden-crypto(hopefully, in a couple of weeks).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.
Since we're pulling entries by using an iterator we can make sure we never fetch more than
MAX_RETURN_ENTRIES + 1entries from the tree. I've also added a TODO for the follow-up work.