Add boundary constraints to the constraint graph #126

grjte · 2023-01-26T14:16:28Z

This PR addresses part of #106, by moving boundary constraints into the AlgebraicGraph alongside the integrity constraints, and updating the codegen and tests accordingly.

It includes the following changes:

add the boundary statements to the graph
refactor to graph handling to simplify the combined handling of integrity + boundary constraints
update winter codegen to work with the new boundary constraints
update the tests to reflect the changes (primarily constraint re-ordering) and skip the tests which are failing due to the pending changes listed below

There are several things that yet to be completed, but I'd like to tackle these in a subsequent PR since this is already quite large:

restore functionality of public inputs, by adding them to the graph, then restore the related tests
restore boundary constraint validation in the IR, then restore the related tests
add support for boundary variables
verify that all cases are captured and add any additional validation/testing as required

grjte · 2023-01-26T15:02:40Z

air-script/tests/variables/variables.air

-    enf a.first = stack_inputs[0]
+    enf a.first = 0


I changed this to keep the public inputs usage in the public inputs test. Once we add variables for the boundary constraints, we can test them here as well

Al-Kindi-0

Looks great to me, thanks @grjte !
Just some minor nits from my side.

ir/src/constraints/mod.rs

tohrnii

Thank you @grjte, looks good to me. I left a few minor nits inline.

codegen/winterfell/src/air/boundary_constraints.rs

codegen/winterfell/src/air/mod.rs

ir/src/constraints/graph.rs

Fumuran

Looks great to me, thank you @grjte!

grjte · 2023-01-30T17:06:46Z

Thanks for your reviews @Al-Kindi-0 @Overcastan and @tohrnii! I made the suggested changes, except for one point of discussion raised by @tohrnii (which it would be great to get your thoughts on).

Since this is a very significant change, let's wait for the review from @bobbinth before we merge.

bobbinth

Looks great! Thank you! I reviewed everything except for tests.

I think the structure is much cleaner than before. My comments are mostly for the future except fro the split_boundary_constraint comment. But even that one is non-blocking.

bobbinth · 2023-01-30T17:48:11Z

codegen/winterfell/src/air/ir_types.rs

+                    ElemType::Felt => format!("({lhs}).exp(Felt::new({r_idx}))"),
+                    ElemType::E => format!("({lhs}).exp(E::PositiveInteger::from({r_idx}_u64))"),


We should remember that using exp could be quite expensive as it uses constant-time exponentiation. In the future, we might want o "unroll" this into a sequence of multiplications.

Agreed. I was trying to minimize changes in this PR. I've opened #130 to track this.

Are you thinking this unrolling should be done in the IR or in the winterfell codegen? (It seems to make sense as a future optimization that could be done in the IR)

codegen/winterfell/src/air/mod.rs

codegen/winterfell/src/air/boundary_constraints.rs

bobbinth · 2023-01-30T18:11:28Z

ir/src/constraints/mod.rs

+    /// Variable roots for the variables used in integrity constraints. For each element in a
+    /// vector or a matrix, a new root is added with a key equal to the [VariableValue] of the
+    /// element.
+    variable_roots: VariableRoots,


As far as I can tell, this is a helper map which is used only during the construction of the graph and is not really needed after the graph is constructed. If so, it may be good to find a better way to handle it in the future so that once constructed Constraints does not need to retain any extra info.

Yes, this is true. I think there are still a few refactors that could be done, but I wanted to keep this PR as minimal as possible. Once the remaining issues related to this work are addressed this is worth revisiting (or possibly while addressing those)

ir/src/constraints/graph.rs

grjte commented Jan 26, 2023

View reviewed changes

grjte force-pushed the grjte-refactor-graph branch from cd7e777 to 8828c54 Compare January 26, 2023 15:48

tohrnii mentioned this pull request Jan 26, 2023

Tracking PR for v0.2 release #79

Merged

14 tasks

grjte force-pushed the grjte-refactor-graph branch 2 times, most recently from d2f0406 to 49f2fd8 Compare January 26, 2023 19:36

grjte marked this pull request as ready for review January 26, 2023 19:42

grjte requested review from tohrnii, bobbinth, Fumuran and Al-Kindi-0 and removed request for tohrnii and bobbinth January 26, 2023 19:42

grjte mentioned this pull request Jan 27, 2023

Allow expressions as exponents in exp expressions #120

Merged

grjte force-pushed the grjte-refactor-graph branch 2 times, most recently from c915ba6 to cd08bd7 Compare January 27, 2023 13:11

Al-Kindi-0 reviewed Jan 30, 2023

View reviewed changes

ir/src/constraints/mod.rs Outdated Show resolved Hide resolved

ir/src/constraints/mod.rs Outdated Show resolved Hide resolved

tohrnii approved these changes Jan 30, 2023

View reviewed changes

Fumuran approved these changes Jan 30, 2023

View reviewed changes

grjte force-pushed the grjte-refactor-graph branch 3 times, most recently from d48dce5 to f2c43ea Compare January 30, 2023 15:38

grjte marked this pull request as draft January 30, 2023 15:47

grjte force-pushed the grjte-refactor-graph branch from f2c43ea to cb45670 Compare January 30, 2023 15:54

grjte marked this pull request as ready for review January 30, 2023 15:55

bobbinth approved these changes Jan 30, 2023

View reviewed changes

grjte mentioned this pull request Jan 31, 2023

unroll exponentiation into multiplications in winter codegen #130

Closed

feat: move boundary constraints into the graph

9abb772

grjte added 3 commits January 31, 2023 14:14

refactor: simplify insertions into the constraint graph

6447ee9

chore: update winterfell code generation

c31968a

test: update tests for boundary constraint changes

3f44c8c

grjte force-pushed the grjte-refactor-graph branch from cb45670 to 3f44c8c Compare January 31, 2023 14:15

grjte merged commit 2141470 into next Jan 31, 2023

grjte deleted the grjte-refactor-graph branch January 31, 2023 14:18

This was referenced Feb 13, 2023

Restore boundary constraint validation #144

Merged

fix: restore public input support #145

Merged

grjte mentioned this pull request Feb 22, 2023

merge boundary constraints into AlgebraicGraph and support variables #106

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add boundary constraints to the constraint graph #126

Add boundary constraints to the constraint graph #126

grjte commented Jan 26, 2023 •

edited

Loading

grjte Jan 26, 2023

Al-Kindi-0 left a comment

tohrnii left a comment

Fumuran left a comment

grjte commented Jan 30, 2023

bobbinth left a comment

bobbinth Jan 30, 2023

grjte Jan 31, 2023

bobbinth Jan 30, 2023

grjte Jan 31, 2023

		ElemType::Felt => format!("({lhs}).exp(Felt::new({r_idx}))"),
		ElemType::E => format!("({lhs}).exp(E::PositiveInteger::from({r_idx}_u64))"),

Add boundary constraints to the constraint graph #126

Add boundary constraints to the constraint graph #126

Conversation

grjte commented Jan 26, 2023 • edited Loading

grjte Jan 26, 2023

Choose a reason for hiding this comment

Al-Kindi-0 left a comment

Choose a reason for hiding this comment

tohrnii left a comment

Choose a reason for hiding this comment

Fumuran left a comment

Choose a reason for hiding this comment

grjte commented Jan 30, 2023

bobbinth left a comment

Choose a reason for hiding this comment

bobbinth Jan 30, 2023

Choose a reason for hiding this comment

grjte Jan 31, 2023

Choose a reason for hiding this comment

bobbinth Jan 30, 2023

Choose a reason for hiding this comment

grjte Jan 31, 2023

Choose a reason for hiding this comment

grjte commented Jan 26, 2023 •

edited

Loading