refactor: forester: batch ops #1453
Conversation
sergeytimoshin
changed the title
| TreeType::BatchedState => { | ||
| info!("Processing state batch"); | ||
| state::process_batch(&self.context).await | ||
| } |
There was a problem hiding this comment.
this needs to distinguish between an input(nullifier) queue batch being ready and an output queue batch being ready. The current implementation assumes that both are always ready at the same time which will fail in many cases.
| } | ||
| } | ||
|
|
||
| async fn verify_batch_ready(&self) -> bool { |
There was a problem hiding this comment.
doesn't cover the output queue whichs batches are independent of input queue batches.
The input queue is part of the Merkle tree account since we usually need access to Merkle tree roots to prove inclusion (except if the account hash wasn't inserted into the tree yet then we need the output queue to proof inclusion by index in the value array).
The output queue is a separate account with separate batches. You can check whether an output queue batch is ready with the same logic used for trees the data structure is similar.
There was a problem hiding this comment.
The address Merkle tree doesn't have an output queue hence the current implementation conceptually works for address trees as is.
There was a problem hiding this comment.
Fixed, thanks!
sergeytimoshin
force-pushed
the
sergey/foresters-batch-ops
branch
from
a3c5f20 to
61346cb
Compare
| if self.verify_input_queue_batch_ready(&mut rpc).await { | ||
| BatchReadyState::ReadyForNullify | ||
| } else if self.verify_output_queue_batch_ready(&mut rpc).await { | ||
| BatchReadyState::ReadyForAppend | ||
| } else { |
There was a problem hiding this comment.
Two thoughts:
- what about switching this checking
verify_output_queue_batch_readywith priority since I expect more output state creation than input state creation? - There could be a case where the forester isn't fast enough and one queue is filled so fast that the forester neglects the other queue. Probably not the right time to focus on this right now but makes sense to keep this in mind.
…ize output batch readiness
forester/tests/batched_state_test.rs
Outdated
| assert!( | ||
| merkle_tree | ||
| .get_metadata() | ||
| .queue_metadata | ||
| .next_full_batch_index | ||
| > 0, | ||
| "No batches were processed" | ||
| ); |
There was a problem hiding this comment.
| assert!( | |
| merkle_tree | |
| .get_metadata() | |
| .queue_metadata | |
| .next_full_batch_index | |
| > 0, | |
| "No batches were processed" | |
| ); | |
| assert_eq!( | |
| merkle_tree | |
| .get_metadata() | |
| .queue_metadata | |
| .next_full_batch_index | |
| ,1, | |
| "No batches were processed" | |
| ); |
forester/tests/batched_state_test.rs
Outdated
| assert!( | ||
| final_metadata.sequence_number > initial_sequence_number, | ||
| "Sequence number should have increased" | ||
| ); | ||
|
|
||
| let post_root = merkle_tree.get_root().unwrap(); | ||
| assert_ne!(pre_root, post_root, "Roots are the same"); | ||
|
|
||
| assert_ne!( | ||
| pre_root, | ||
| merkle_tree.get_root().unwrap(), | ||
| "Root should have changed" | ||
| ); | ||
| assert!( | ||
| merkle_tree.root_history.len() > 1, | ||
| "Root history should contain multiple roots" | ||
| ); |
There was a problem hiding this comment.
would be better to assert exact numbers so that we can detect if the forester starts to behave differently.
There was a problem hiding this comment.
Also we don't assert the root of the test indexer just that the root is not equal.
(The same for address tree asserts.)
No description provided.