Filter later-stage witnesses and identities #2129
Conversation
| refs_in_parsed_expression(pf).unique().all(|n| { | ||
| let def = self.fixed.analyzed.definitions.get(n); | ||
| def.and_then(|(s, _)| s.stage).unwrap_or_default() <= stage as u32 |
There was a problem hiding this comment.
I think this was actually wrong, it wouldn't resolve array elements.
georgwiese
changed the title
| @@ -225,7 +233,7 @@ impl<'a, 'b, T: FieldElement> WitnessGenerator<'a, 'b, T> { | |||
| mut machines, | |||
| base_parts, | |||
| } = if self.stage == 0 { | |||
| MachineExtractor::new(&fixed).split_out_machines(retained_identities, self.stage) | |||
| MachineExtractor::new(&fixed).split_out_machines(retained_identities) | |||
There was a problem hiding this comment.
So we don't pass the stage because we already only pass the identities up to this stage
There was a problem hiding this comment.
Yes, and also the machine extractor can use functions like FixedData::current_stage_witnesses.
| fn current_stage_witnesses(&self) -> impl Iterator<Item = PolyID> + '_ { | ||
| self.witness_cols | ||
| .iter() | ||
| .filter(|(_, col)| col.stage <= self.stage as u32) |
There was a problem hiding this comment.
Doesn't this filtering already happen in the constructor?
There was a problem hiding this comment.
And shouldn't it be == self.stage?
There was a problem hiding this comment.
Doesn't this filtering already happen in the constructor?
No
And shouldn't it be
== self.stage?
I mean it's only currently used by the MachineExtractor, which is only run in stage 0, so it doesn't matter in practice. But I think it makes more sense to have a function that returns all witnesses relevant in the current stage, which would include previous-stage witnesses. Maybe I should rename it to witnesses_for_current_stage()?
| fn polynomial_references( | ||
| &self, | ||
| expr: &impl AllChildren<AlgebraicExpression<T>>, | ||
| ) -> HashSet<PolyID> { |
There was a problem hiding this comment.
Is it fine for this to be non-deterministic? Maybe BTreeSet is safer?
There was a problem hiding this comment.
It shouldn't matter, but I can change it!
There was a problem hiding this comment.
Hmm, it is used by the machine extractor, which uses hash sets everywhere. It would propagate quite far, so unless you feel strongly about it, I'd leave it as is. As long as its only used as a set, it should be fine.
| @@ -375,6 +383,7 @@ pub struct FixedData<'a, T: FieldElement> { | |||
| challenges: BTreeMap<u64, T>, | |||
| global_range_constraints: GlobalConstraints<T>, | |||
| intermediate_definitions: BTreeMap<PolyID, &'a AlgebraicExpression<T>>, | |||
| stage: u8, | |||
There was a problem hiding this comment.
Now it's only fixed for one stage of witness generation.
There was a problem hiding this comment.
Although we already had challenges right?
There was a problem hiding this comment.
Yes, it's fixed for the duration of the call to WitnessGenerator::generate!
There was a problem hiding this comment.
Can you mention that in the docstring? ("Fixed for witness generation for a certain proof stage")
Co-authored-by: Thibaut Schaeffer <[email protected]>
georgwiese
force-pushed
the
filter-second-stage-stuff
branch
from
581cc49
to
9aec364
Compare
Prepares #2129
With this PR, later-stage witness columns & identities referencing them (or later-stage challenges) are completely ignored. The columns are not assigned to any machine. Previously, they would end up in the main machine and never receive any updates. That doesn't work if machines have different sized though.