Witgen: Pass range constraints separately #2451

georgwiese · 2025-02-06T13:02:12Z

this PR changes the signature of Machine::process_plookup from

    fn process_plookup<'b, Q: QueryCallback<T>>(
        &mut self,
        mutable_state: &'b MutableState<'a, T, Q>,
        identity_id: u64,
        caller_rows: &'b RowPair<'b, 'a, T>,
    ) -> EvalResult<'a, T>;

To

    fn process_plookup<'b, Q: QueryCallback<T>>(
        &mut self,
        mutable_state: &'b MutableState<'a, T, Q>,
        identity_id: u64,
        parameters: &[AffineExpression<AlgebraicVariable<'a>, T>],
        range_constraints: &dyn RangeConstraintSet<AlgebraicVariable<'a>, T>,
    ) -> EvalResult<'a, T>;

Previously, each machine evaluated the parameters themselves, using the caller_rows and the calling bus send, stored in the connection. But in the future, the calling bus send will not be static, so I moved this to the caller.

This will enable #2447 and eventually the dynamic bus.

executor/src/witgen/data_structures/mutable_state.rs

executor/src/witgen/machines/block_machine.rs

executor/src/witgen/machines/double_sorted_witness_machine_16.rs

executor/src/witgen/machines/double_sorted_witness_machine_32.rs

executor/src/witgen/machines/dynamic_machine.rs

chriseth · 2025-02-06T14:46:21Z

executor/src/witgen/machines/dynamic_machine.rs

-        for (r, l) in identity.right.expressions.iter().zip(&outer_query.left) {
+        for (r, l) in identity
+            .right
+            .expressions


These are the parameters, I would say.

chriseth · 2025-02-06T14:47:42Z

executor/src/witgen/machines/fixed_lookup_machine.rs

-                    rows,
-                    outer_query.left.first().unwrap(),
+                    outer_query.range_constraints,
+                    outer_query.parameters.first().unwrap(),
                    AlgebraicVariable::Column(column_reference),
                );
            }


Should we respect the range constraints in the rest of the code or is it just a hint? Please document in the trait function.

Not sure what you mean. Document in Machine::process_plookup (when are we going to rename this haha)?

I think it would be nice to respect them and could even lead to a unique result when it's not unique just from the known values. But seems like it's not needed in practice. Seems a out of the scope of this PR to fix this.

ah true, because we get a single result anyway. ok!

executor/src/witgen/machines/mod.rs

executor/src/witgen/processor.rs

chriseth · 2025-02-06T14:49:46Z

executor/src/witgen/processor.rs

        connection: Connection<'a, T>,
    ) -> Result<Self, IncompleteCause<AlgebraicVariable<'a>>> {
-        // Evaluate once, for performance reasons.


What was this comment about?

That we don't want to evaluate to an affine expression many times. Now, this happens even earlier in the caller.

executor/src/witgen/machines/mod.rs

Co-authored-by: chriseth <[email protected]>

georgwiese added 2 commits February 6, 2025 10:01

Witgen: Pass range constraints separately

89a3eed

Clippy

559300b

georgwiese marked this pull request as ready for review February 6, 2025 13:17