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.

Sign up for GitHub

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

Jump to bottom

Block machine witgen: Always run default sequence after the cached sequence #1562

Merged
merged 2 commits into from
Jul 15, 2024
Merged

Block machine witgen: Always run default sequence after the cached sequence #1562

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aa1048d
Block machine witgen: Always run default sequence after the cached se…
georgwiese Jul 11, 2024
2100471
Undo change
georgwiese Jul 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 0 additions & 19 deletions executor/src/witgen/machines/block_machine.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,6 @@ impl<'a, T: FieldElement> ProcessResult<'a, T> {
false => ProcessResult::Incomplete(updates),
}
}

fn is_success(&self) -> bool {
match self {
ProcessResult::Success(_, _) => true,
ProcessResult::Incomplete(_) => false,
}
}
}

fn collect_fixed_cols<T: FieldElement>(
Expand Down Expand Up @@ -517,18 +510,6 @@ impl<'a, T: FieldElement> BlockMachine<'a, T> {
let process_result =
self.process(mutable_state, &mut sequence_iterator, outer_query.clone())?;

let process_result = if sequence_iterator.is_cached() && !process_result.is_success() {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This code tried to solve the same problem.

Disadvantages of the old approach:

  • process_result.is_success() returns true if it was able to complete the outer query. But there might still be cells in the block that could not be solved. They will be set to 0, which might violate some constraints. This causes the issue described in Witgen produces wrong witness when reordering constraints #1559.
  • If this block does get executed, the block processor is invoked again from scratch, forgetting any progress it has made already in the first call.

log::debug!("The cached sequence did not complete the block machine. \
This can happen if the machine's execution steps depend on the input or constant values. \
We'll try again with the default sequence.");
let mut sequence_iterator = self
.processing_sequence_cache
.get_default_sequence_iterator();
self.process(mutable_state, &mut sequence_iterator, outer_query.clone())?
} else {
process_result
};

match process_result {
ProcessResult::Success(new_block, updates) => {
log::trace!(
Expand Down
35 changes: 22 additions & 13 deletions executor/src/witgen/sequence_iterator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,10 @@ pub enum ProcessingSequenceIterator {
/// The default strategy
Default(DefaultSequenceIterator),
/// The machine has been run successfully before and the sequence is cached.
Cached(<Vec<SequenceStep> as IntoIterator>::IntoIter),
Cached(
<Vec<SequenceStep> as IntoIterator>::IntoIter,
DefaultSequenceIterator,
),
/// The machine has been run before, but did not succeed. There is no point in trying again.
Incomplete,
}
Expand All @@ -179,24 +182,17 @@ impl ProcessingSequenceIterator {
pub fn report_progress(&mut self, progress_in_last_step: bool) {
match self {
Self::Default(it) => it.report_progress(progress_in_last_step),
Self::Cached(_) => {} // Progress is ignored
Self::Cached(_, _) => {} // Progress is ignored
Self::Incomplete => unreachable!(),
}
}

pub fn has_steps(&self) -> bool {
match self {
Self::Default(_) | Self::Cached(_) => true,
Self::Default(_) | Self::Cached(_, _) => true,
Self::Incomplete => false,
}
}

pub fn is_cached(&self) -> bool {
match self {
Self::Default(_) => false,
Self::Cached(_) | Self::Incomplete => true,
}
}
}

impl Iterator for ProcessingSequenceIterator {
Expand All @@ -205,7 +201,13 @@ impl Iterator for ProcessingSequenceIterator {
fn next(&mut self) -> Option<Self::Item> {
match self {
Self::Default(it) => it.next(),
Self::Cached(it) => it.next(),
// After the cached iterator is exhausted, run the default iterator again.
// This is because the order in which the identities should be processed *might*
// depend on the concrete input values.
// In the typical scenario, most identities will be completed at this point and
// the block processor will skip them. But if an identity was not completed before,
// it will try again.
Self::Cached(it, default_iterator) => it.next().or_else(|| default_iterator.next()),
Self::Incomplete => unreachable!(),
}
}
Expand Down Expand Up @@ -246,7 +248,14 @@ impl ProcessingSequenceCache {
match self.cache.get(&left.into()) {
Some(CacheEntry::Complete(cached_sequence)) => {
log::trace!("Using cached sequence");
ProcessingSequenceIterator::Cached(cached_sequence.clone().into_iter())
ProcessingSequenceIterator::Cached(
cached_sequence.clone().into_iter(),
DefaultSequenceIterator::new(
self.block_size,
self.identities_count,
Some(self.outer_query_row as i64),
),
)
}
Some(CacheEntry::Incomplete) => ProcessingSequenceIterator::Incomplete,
None => {
Expand Down Expand Up @@ -291,7 +300,7 @@ impl ProcessingSequenceCache {
.is_none());
}
ProcessingSequenceIterator::Incomplete => unreachable!(),
ProcessingSequenceIterator::Cached(_) => {} // Already cached, do nothing
ProcessingSequenceIterator::Cached(_, _) => {} // Already cached, do nothing
}
}
}
Loading