Added batching using DB. #724
Conversation
src/identity_tree.rs
Outdated
| @@ -263,6 +263,9 @@ impl BasicTreeOps for TreeVersionData<lazy_merkle_tree::Derived> { | |||
| fn update(&mut self, leaf_index: usize, element: Hash) { | |||
| let updated_tree = self.tree.update(leaf_index, &element); | |||
|
|
|||
| if self.tree.root() == updated_tree.root() { | |||
There was a problem hiding this comment.
I added it here because of recreating tree on startup. It is hard to easily select from database proper groups of commitments. There are three options:
- As done change the code to not apply updates that don't change the tree at all. Why? Because then we are peeking changes to create a batch that are not changing anything and we can't put such a batch in database (unique/fk constraints).
- Instead of doing it here we can peek the "no-changing" changes and just not save them in database.
- Add additional internal status just for this "batched but not processed yet" situation.
- Do more queries on startup but may use much more memory as we are loading almost all commitments. :/
src/identity_tree.rs
Outdated
| @@ -280,7 +283,25 @@ impl BasicTreeOps for TreeVersionData<lazy_merkle_tree::Derived> { | |||
| fn apply_diffs(&mut self, mut diffs: Vec<AppliedTreeUpdate>) { | |||
| let last = diffs.last().cloned(); | |||
|
|
|||
| self.metadata.diff.append(&mut diffs); | |||
| if !diffs.is_empty() { | |||
src/identity_tree.rs
Outdated
| latest: TreeVersion<Latest>, | ||
| mined: TreeVersion<Canonical>, | ||
| processed: TreeVersion<Intermediate>, | ||
| processed_batching: TreeVersion<Intermediate>, |
There was a problem hiding this comment.
This additional tree is required to generate proofs. We need a tree in a state when root is same as prev_root in batch. We can use batching tree as it is updated to create new batches (with proper leaf_indexes).
src/database/mod.rs
Outdated
| batches.id as id, | ||
| batches.next_root as next_root, | ||
| batches.prev_root as prev_root, | ||
| batches.created_at as created_at, |
There was a problem hiding this comment.
AFAIK sqlx handles fully qualified names, so you can leave this (and others) as batches.created_at
There was a problem hiding this comment.
I'm sure I have done it due to created_at conflict but will reassure myself. :)
src/database/mod.rs
Outdated
| let batches = tx.get_all_batches_after(batch_entry.id).await?; | ||
| let mut result = vec![]; | ||
| for batch in batches { | ||
| match batch.batch_type { |
There was a problem hiding this comment.
Can't we have the same code for both cases? In case for deletion we replace *batch.commitments.0.get(i).unwrap() with Hash::ZERO but *batch.commitments.0.get(i).unwrap() should be zero anyway, no?
piohei
force-pushed
the
piohei/ha_step_1
branch
2 times, most recently
from
0c61b0c to
b38c59f
Compare
piohei
changed the title
This is a first step to introduce batches and transactions table. This is not HA solution yet - just the first step towards it.
Motivation
Make small changes instead of one big.
Solution
Now processing batches is changed into 2 steps. First one is to create a batch (select identities, leaf indexes, etc.). Then the batch is saved in database. Second step is reading data from database (batch) and executes it. Proper batch execution includes storing transaction id in database.
As an alternative solution we could save in database a batch with a proves (no need to calculate it later). It will allow second step (transaction creation/execution) to not use any tree at all. It will require more memory used in database (it can be cleared from time to time as we don't need all the batches forever) but will make HA solution much easier.
PR Checklist