spec: ARE_BYTES#532
Conversation
Codex Code Review
No direct security issues stood out in the diff beyond that compatibility/correctness concern. |
| @@ -101,6 +101,11 @@ name = "μ_IS_BYTE" | |||
| type = "BaseField" | |||
| desc = "" | |||
There was a problem hiding this comment.
Medium – Dead multiplicity variable
μ_IS_BYTE is no longer referenced in any constraint in this file. Since IS_BYTE is now a template (not an interaction), BITWISE no longer contributes to an IS_BYTE interaction, so this multiplicity column serves no purpose. It should be removed.
| desc = "" |
| [[constraints.all]] | ||
| kind = "interaction" | ||
| tag = "ARE_BYTES" | ||
| input = [0, "X"] |
There was a problem hiding this comment.
Low – Constant 0 consumes a BITWISE row slot unnecessarily
IS_BYTE<X, μ> expands to ARE_BYTES[0, X], always using the first byte slot as the constant 0. This means every IS_BYTE invocation occupies a full BITWISE row while only constraining one variable.
This is by design (the template exists for chips with an odd number of byte range-checks), but it's worth noting in the spec that the "waste" is intentional and that implementors should prefer pairing directly into ARE_BYTES[X, Y] whenever possible, as is_byte.typ already encourages.
No code change required — just flagging for visibility.
There was a problem hiding this comment.
that implementors should prefer pairing directly into ARE_BYTES[X, Y] whenever possible ...
This is exactly what is written in the IS_BYTE template description.
Review:
|
RobinJadoul
left a comment
There was a problem hiding this comment.
Let's make an issue to manually add the batching to the spec once we're done with the rework of constraint numbering?
| [[variables.multiplicity]] | ||
| name = "μ" | ||
| type = "BaseField" | ||
| desc = "" |
There was a problem hiding this comment.
This should probably be a cond instead?
There was a problem hiding this comment.
During development, I named it cond initially, but then switched to mu to highlight that the multiplicity should be the second parameter: the IS_BYTE<BaseField, BaseField> interface will be OK with swapping X and cond
There was a problem hiding this comment.
Alternatively, we could stick with it being a multiplicity, and have this be the first template which has a multiplicity (?)
There was a problem hiding this comment.
Why do we want to pass it as a parameter and not a template cond though?
It leading to an actual multiplicity seems no different from how NEG's cond leads to a multiplicity for ZERO to me. And just having IS_BYTE<x> vs IS_BYTE<x, 1> looks a lot more readable to me.
There was a problem hiding this comment.
And just having IS_BYTE vs IS_BYTE<x, 1> looks a lot more readable to me.
I completely agree with you.
It leading to an actual multiplicity seems no different from how NEG's cond leads to a multiplicity for ZERO to me.
There would be a difference: NEG's cond is assumed to be a Bit, where that would not be the case here. This is what was stopping me previously, but I agree that cond => IS_BIT<X> is much easier on the eyes
I don't think we should batch them in the spec:
|
|
I don't disagree with those arguments, my main reason to do it despite those would be to keep the clear correspondence between spec and implementation stricter. |
2be5ce0 to
75216c8
Compare
Let us note that it would be to keep the possibility of having a stricter correspondence between spec and implementation: the fact that we increase the level of detail to the spec does NOT necessarily mean that the devs will (eternally) ensure the implementation matches the spec. I would argue that sticking with But I'll bite: how do you propose we resolve the dilemma I spelled out in the second point? |
|
I still agree with those points, and if indeed we'd end up saying the spec should reflect the batching, we'd have to make sacrifices. I think we can probably table it for now, and keep it implicit. If at any point in the future we have reason to reconsider (e.g. we notice issues, or start extracting code from the spec that we want to keep simple) we can deal with the readability then. |
|
It is now listed as issue #571 |
i) introduces the
ARE_BYTESlookup,ii) introduces the
IS_BYTE<X, μ>template that maps toARE_BYTES[0, X]/μiii) removes the
IS_BYTElookup