-
Notifications
You must be signed in to change notification settings - Fork 87
feat: [2/4] integrate smtforest, avoid ser/de of full account/vault data in database #1394
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: bernhard-db-schema-queries
Are you sure you want to change the base?
Conversation
7980bed to
771f6d7
Compare
771f6d7 to
b2aa54b
Compare
b2aa54b to
2964a93
Compare
Mirko-von-Leipzig
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.
Mostly questions; no real issues found as yet
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.
Thank you! I left some comments inline and pushed a few commits to try to organize the code a bit better. Overall, we are still quite far from being able to merge this PR as there are some bugs and logical inconsistencies.
Also, given the amount of remaining issues, I'm a bit concerned about our ability to wrap this PR up in a timely manner. Given the complexity of this PR, deep reviews take quite a bit of time. Maybe there is a way to split this up into smaller incremental PRs that we can review and merge much more quickly.
crates/store/src/state.rs
Outdated
| let public_account_ids: Vec<AccountId> = db | ||
| .select_all_account_commitments() | ||
| .await? | ||
| .iter() | ||
| .filter_map(|(id, _commitment)| if id.has_public_state() { Some(*id) } else { None }) | ||
| .collect(); |
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.
This is pretty inefficient because we just need to get a list of accounts with public state, but instead we are loading all account commitments. This is OK for now because we'll replace the initialization code as we migrate to RocksDB-based backends.
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.
We could filter as part of the query using lt/gt given a certain prefix range if the layout is advantageous, LIKE statements are not supported by diesel after taking a brief look.
EDIT: looking at the layout of the exact bits in AccountId and the prefix, it seems the pub/priv/network types are encoding it at the lower octet of the prefix, range filtering won't work.
| /// Read-write lock used to prevent writing to a structure while it is being used. | ||
| /// | ||
| /// The lock is writer-preferring, meaning the writer won't be starved. | ||
| inner: RwLock<InnerState>, | ||
|
|
||
| /// Forest-related state `(SmtForest, storage_roots, vault_roots)` with its own lock. | ||
| forest: RwLock<InnerForest>, |
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'm not seeing a good reason to separate InnerForest from InnerState - so, let's combine them. This could look simply as:
struct InnerState<S = MemoryStorage>
where
S: SmtStorage,
{
blockchain: Blockchain,
account_tree: AccountTreeWithHistory<S>,
nullifier_tree: NullifierTree<LargeSmt<S>>,
forest: InnerForest,
}In the future (when moving to a RocksDB backend), we should refactor this to move InnerState into a separate file and clean up the module hierarchy.
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.
It takes care of encapsulating the indices required for lookup and hiding the internals, which will come in handy once LargeSmtForest lands, which has a largely different API.
That's not withstanding a separate module cleanup/InnerState move.
3bd5451 to
c5a199a
Compare
Part 2/4 of #1185 and lays the ground for #1354
Intent
AccountInfocore changes
vault/storage_mapBLOB entries from the accounts table.SmtForestand integrate intoapply_blockandState::loadsignificant changes in the following files:
crates/store/src/db/schema.rsintroducesaccount_storage_headersand removesstorage(the full serialized account storage) fromaccountstablecrates/store/src/state.rs/fn State::apply_blocknow updates the database, but also the lookup table of roots for theSmtForestand the entries in the forest itself, indirect lookup tablesout of scope
LargeSmtForestfor partial storage maps - [devops] Deploy without nuking db #670GetAccountProofendpoint #1354(Large|)SmtForest- Add cleanup routine for outdated historical storage entries #1355how to review
Take the existing TODOs into consideration, if they make sense. This will be the follow up PR.