Skip to content
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

chore: cleanup & fix zero out bloom filter #1487

Merged
merged 6 commits into from
Jan 17, 2025

Conversation

ananas-block
Copy link
Contributor

@ananas-block ananas-block commented Jan 16, 2025

Changes:

  1. rename BatchState::CanBeFilled -> BatchState::Fill
  2. rename *_wiped -> *_zeroed
  3. initializing root history with full length
    -> needed to adapt expected len assertion in batched forester tests

@ananas-block ananas-block force-pushed the jorrit/chore-batched-cleanup4 branch from 2eeee1d to 259288c Compare January 16, 2025 02:15
@ananas-block ananas-block marked this pull request as ready for review January 16, 2025 02:16
@ananas-block ananas-block marked this pull request as draft January 16, 2025 02:17
@ananas-block ananas-block force-pushed the jorrit/chore-batched-cleanup4 branch from 259288c to 6c60c3a Compare January 16, 2025 02:34
@ananas-block ananas-block marked this pull request as ready for review January 16, 2025 02:35
@ananas-block ananas-block marked this pull request as draft January 16, 2025 02:35
@ananas-block ananas-block marked this pull request as ready for review January 16, 2025 02:41
@ananas-block ananas-block marked this pull request as draft January 16, 2025 02:42
@ananas-block ananas-block force-pushed the jorrit/chore-batched-cleanup4 branch from d71ba2b to 31f6675 Compare January 16, 2025 14:09
@ananas-block ananas-block marked this pull request as ready for review January 16, 2025 14:09
@ananas-block ananas-block changed the title chore: cleanup & fix wipe bloom filter chore: cleanup & fix zero out bloom filter Jan 16, 2025
pub next_full_batch_index: u64,
pub bloom_filter_capacity: u64,
}

impl BatchMetadata {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
impl BatchMetadata {
impl BatchMetadata {
/// Returns the number of ZKP batches contained within a single regular batch.
pub fn get_num_zkp_batches(&self) -> u64 {
self.batch_size / self.zkp_batch_size
}
/// Validates that the batch size is properly divisible by the ZKP batch size.
fn validate_batch_sizes(batch_size: u64, zkp_batch_size: u64) -> Result<(), BatchedMerkleTreeError> {
if batch_size % zkp_batch_size != 0 {
return Err(BatchedMerkleTreeError::BatchSizeNotDivisibleByZkpBatchSize);
}
Ok(())
}

Comment on lines 46 to 49
if batch_size % zkp_batch_size != 0 {
return Err(BatchedMerkleTreeError::BatchSizeNotDivisibleByZkpBatchSize);
}
Ok(BatchMetadata {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if batch_size % zkp_batch_size != 0 {
return Err(BatchedMerkleTreeError::BatchSizeNotDivisibleByZkpBatchSize);
}
Ok(BatchMetadata {
Self::validate_batch_sizes(batch_size, zkp_batch_size)?;
Ok(BatchMetadata {

Comment on lines 65 to 68
if batch_size % zkp_batch_size != 0 {
return Err(BatchedMerkleTreeError::BatchSizeNotDivisibleByZkpBatchSize);
}
Ok(BatchMetadata {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if batch_size % zkp_batch_size != 0 {
return Err(BatchedMerkleTreeError::BatchSizeNotDivisibleByZkpBatchSize);
}
Ok(BatchMetadata {
Self::validate_batch_sizes(batch_size, zkp_batch_size)?;
Ok(BatchMetadata {

Comment on lines 81 to 82
self.next_full_batch_index += 1;
self.next_full_batch_index %= self.num_batches;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.next_full_batch_index += 1;
self.next_full_batch_index %= self.num_batches;
self.next_full_batch_index = (self.next_full_batch_index + 1) % self.num_batches;

Comment on lines 89 to 90
self.currently_processing_batch_index += 1;
self.currently_processing_batch_index %= self.num_batches;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.currently_processing_batch_index += 1;
self.currently_processing_batch_index %= self.num_batches;
self.currently_processing_batch_index = (self.currently_processing_batch_index + 1) % self.num_batches;

Comment on lines 100 to 105
self.num_batches = num_batches;
self.batch_size = batch_size;
// Check that batch size is divisible by zkp_batch_size.
if batch_size % zkp_batch_size != 0 {
return Err(BatchedMerkleTreeError::BatchSizeNotDivisibleByZkpBatchSize);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
self.num_batches = num_batches;
self.batch_size = batch_size;
// Check that batch size is divisible by zkp_batch_size.
if batch_size % zkp_batch_size != 0 {
return Err(BatchedMerkleTreeError::BatchSizeNotDivisibleByZkpBatchSize);
}
Self::validate_batch_sizes(batch_size, zkp_batch_size)?;
self.num_batches = num_batches;
self.batch_size = batch_size;

Ok((num_value_stores, num_stores, num_batches))
}
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
#[test]
fn test_batch_size_validation() {
// Test invalid batch size
assert!(BatchMetadata::new_input_queue(10, 10, 3, 2).is_err());
assert!(BatchMetadata::new_output_queue(10, 3, 2).is_err());
// Test valid batch size
assert!(BatchMetadata::new_input_queue(9, 10, 3, 2).is_ok());
assert!(BatchMetadata::new_output_queue(9, 3, 2).is_ok());
}

@ananas-block ananas-block merged commit 79321b3 into main Jan 17, 2025
24 checks passed
@ananas-block ananas-block deleted the jorrit/chore-batched-cleanup4 branch January 17, 2025 00:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants