-
Notifications
You must be signed in to change notification settings - Fork 13
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
Missing Spillage for Lookups / Range Constraints #151
Comments
I'm not sure this is wrong, because you could remove the shift in the lookup, it'll be exactly the same lookup. We would get a spillage of 2 if you have let's say a lookup |
I'm not really following you here though. In my example, its the combination of a normalisation |
What I mean is that a lookup
is imho the same whatever the value of n.
What does it change that there is a normalisation for the spilling ? |
Well, spillage is necessary to handle cases where padding can break automatically generated constraints. Normlisation is one example where this can arise, but not the only one. Let's take an example:
Some valid traces for this constraint are:
Now, the last trace is a problem. If we pad with
This is now an invalid trace and will be rejected. However, in fact, we would consider this to be a mistake by the author of the constraint. They have failed to account for potential padding by using e.g. a guard such as Now, the real problem with spillage arises for constraints that are added by the Normalisation is one example where
Corset expands the above automatically to something like this:
Here,
Here, Now, finally, we come to the issue. Suppose we pad the last trace with one row of zeros, to give the following (where
This trace will be rejected by the constraint Oh boy, this example is long (sorry). The challenge for
And, suppose we add another row of padding. This is what we need:
So, the point here is that the padding values needed for Phew. Sorry. Hopefully that helps. @delehef Be great if you could check my explanation here actually makes sense. |
@delehef Also, my argument above implies my PR to optimise spillage is wrong. I've done it the wrong way around!!! Agree? |
@letypequividelespoubelles Apologies but my explanation above doesn't explain why this matters for lookups. The reason is that looking up based on an expression introduces a new column in a similar way as for
Is translated into this:
That's still not enough to be relevant for spillage. To break things, we need e.g. a lookup involving a normalisation expression to get us into a similar situation as we had above where |
You nailed it.
My brain is not working 100% now, but I think it's still the past padding; the ungabunga solution is to write your example and see which one makes the constraint pass ^^ |
@delehef Thanks!
Yeah, I think I'm gonna have to do that. Otherwise, I'm not going to be confident. |
So, I've realised that writing a test case is not so easy.
It doesn't seem like the CLI supports either of these options. |
I think the solution is to allow the |
ok, understood, thanks for the explanation, I better understand now. The case I have in mind where we would like a min nb of padding because of a lookup, without new column computed :
here we would like to add n padding rows to column C of module2 (with value of 0). Which is different than the definition of spillage you gave "nb of non-zeros needed in the padding before constant 0". But in terms of line counting, it's exactly the same. The only use case I see currently is for the lookup HUB into SHAKIRA (and n=1 in this case). We dealt with it by trivially add one 0 in the trace as a first row, which gives 2 padding rows with the general padding row added for all modules. |
As far as I can tell, this is a bug: spillage is not being correctly computed for lookups involving expressions. Specifically:
With
corset debug -s
we get this:This feels wrong to me, since it is not consistent with the spillage computed for e.g. this:
UPDATE: This may also extend to range constraints.
The text was updated successfully, but these errors were encountered: