Refactor protocol circuit array functions #12278
Labels
A-security
Area: Relates to security. Something is insecure.
C-protocol-circuits
Component: Protocol circuits (kernel & rollup)
T-refactor
Type: this code needs refactoring
team-turing
Leila's team
A list of some things that feel unsafe:
Some functions are commented with
Important: Only use it for validated arrays: validate_array(array) should be true.
, because these functions are intentionally underconstrained. This feels like too big a footgun for us humans to track. Imo, we should use the type system to ensure this for us:validate_array
should return aValidatedArray
type (or something... naming to be explored next) which tracks its length.|Naming:
validate_array
isn't a descriptive-enough name. It has two requirements in one: we want it to return the array length, but under an unconventional (but understandable) definition of length, which should be qualified:trimmed_array_length_assert_dense
/trimmed_array_length_assert_left_packed
. (Using the word "trimmed" because it neatly mirrors the next function name suggestion...).Given these name suggestions, it could return a
DensePrefixArray
orDenseLeftArray
orLeftPackedArray
, which holds the length, so that then the functions that should "only be used for validated arrays" can take as a param this exact type.In fact, we can rename the function again in this light:
fn assert_left_packed_and_trimmed() -> LeftPackedTrimmedArray
orfn assert_dense_when_trimmed -> DenseTrimmedArray
where the return type holds the underlying array and its unconventionallength
. I like something like this.validate_trailing_zeroes
also seeks to return the array length under a different definition of length, which should be qualified. The difference betweeen this andvalidate_array
is that it allows empty elements interspersed with nonempty elements. It basically trims the array.fn trimmed_array_length
?If, like above, there are functions that will rely on being passed an array which has been validated as trimmed already, we should return a type.
fn assert_trimmed() -> TrimmedArray
which holds the underlying array and its unconventional length. Aligns nicely with the previous paragraph.array_length
is underconstrained, because it assumes a validated array is being passed into it. Perhaps it should be calledarray_length_unsafe. But even that doesn't convey that this function actually expects an array with a dense lhs and empty rhs. I think we should use the type system and put a method on the newly-suggested
DenseTrimmedArray<-- no confusion, no possibility of underconstrained. Similarly, we can put a method on
TrimmedArray` to get the length.array_to_bounded_vec
also should have a version which takes aDenseTrimmedArray
and aTrimmedArray
, rather than an array with the commentImportant: Only use it for validated arrays: validate_array(array) should be true.
.I guess all of these suggestions cause pain in the rest of the circuits. E.g. all the other functions which expect arrays, and iterate over arrays, and access the Empty trait of array elements would need to change to accommodate these "bulkier" types. But at the moment it's too unsafe (liable to underconstrained bugs).
Thanks for reading! :)
cc @LeilaWang @MirandaWood wdyt?
The text was updated successfully, but these errors were encountered: