Lack of documentation or comments

The code in scope generally lacks comments or documentation. We would recommend for best practice to document what individual functions, structs, and so forth are intended to do using documenting comments. This can help avoid mistakes while also improving developer experience.

In some cases, intended behavior of a function may be guessed based on its name; however, this is often not necessarily the case. As a concrete example, the function byte2Nibs in zk-bridge's circuits/fabric/headers/circuit.go can be guessed from its name to convert a byte into two nibbles. Actually, the function is passed not a byte that gets decomposed but bits, and "byte" refers to the requirement that the input be exactly eight bits long. Also, the function name alone leaves open with what endianess the input will be interpreted and in which order the nibbles will be returned. As it turns out, the input bits are interpreted as little endian, and the return nibbles are returned in big-endian order. This should be documented with a documentation comment above the function.

Below we collected some concrete instances where we recommend additional or changed comments or other documentation.

Validity of data not checked by some constructors of sdk types

Types offered by the sdk provide a Values method function that will provide the data underlying the object as a raw list of circuit variables (or circuit constants). Analogously, such lists of raw values can be used to set the type with the method function FromValues. These functions could thus be described as providing serialization and deserialization to these types.

On FromValues, the raw inputs are used as is and are not constrained to be well-formed in accordance to their use in the respective type. For example, for the Uint64 type, the Val field should contain a value of bit width at most 64. However, FromValues does not enforce this:

func (v Uint64) FromValues(vs ...frontend.Variable) CircuitVariable {
	if len(vs) != 1 {
		panic("Uint64.FromValues only takes 1 param")
	}
	v.Val = vs[0]
	return v
}

In most use cases of FromValues, the data passed to it will be enforced to be well-formed already, so not checking this again in this deserialization function is in those cases unnecessary; avoiding these checks saves circuit variables and constraints. In other cases, the caller may need to ensure well-formedness of the input. We recommend to document this.

Similarly, most types offer internal functions with names beginning with new, such as newU64, to construct the type. These also tend to not validate the input. For example, newU64 is implemented as follows:

func newU64(v frontend.Variable) Uint64 {
	return Uint64{Val: v}
}

We recommend to document this in a documentation comment to ensure developers using these functions are aware of this. In the current codebase, various functions such as Uint64API.Mul do use newU64 in a way that could result in values that are larger than a bit width of at most 64; however, these functions do have documentation comments warning that an overflow may happen.

Consistency check for sign bit when using newI248

The newI248 in sdk/api_int248.go is implemented as follows:

func newI248(v ...frontend.Variable) Int248 {
	ret := Int248{Val: v[0]}
	if len(v) > 1 {
		ret.SignBit = v[1]
		ret.signBitSet = true
	}
	return ret
}

The first argument is stored as Val in the returned Int248 and the second as the cached sign SignBit. However, no consistency check is done to ensure that the sign bit in Val matches the one cached in SignBit. Thus, the caller must ensure this. We recommend to document this clearly.

Dealing with native types where witnesses may be used as well

Witnesses in the circuit are assigned values in a finite field of prime order r. When values, for example of type big.Int, are used for assignments, they will thus be reduced modulo r. In many places, native types can be used as well instead of witnesses, essentially acting as constants from the perspective of the circuit. In those cases, native types are used as is and not reduced modulo r. In some situations, such constants can result in a different behavior than the corresponding witnesses; see for example Finding ref↗. Beyond the concrete issue of that finding, it may be considered to generally deal with potential differences in behavior in one of the following three ways:

1. Restrict the values accepted when instantiating constants by only allowing values v satisfying 0 <= v < r.

2. Reduce the value modulo r.

3. Document any differing behavior.

Ambiguous documentation regarding overflows in the sdk's types

The documentation for various arithmetic operations for types provided by the sdk warn about overflows as in the following example:

// Add returns a + b. Overflow can happen if a + b > 2^248
func (api *Uint248API) Add(a, b Uint248, other ...Uint248) Uint248 {

The sdk is otherwise designed in such a way as to make its usage accessible to users not very familiar with how ZK circuits work. Such users might expect that the overflow that can happen for a Uint248 type will in practice be reduction modulo $2^{248}$. For example, they might expect that if they multiply by 2, this then corresponds to shifting bits to the left, with the most significant bit not being retained.

But this is not the case. Circuit variables store values that are elements of a finite field of prime order r, and arithmetic operations are performed modulo r. Functions such as the above contain no additional logic to change this. Thus, if the new value will be wider than (in the case of Uint248) 248 bits, then functions such as ToBinary might now not be satisfiable anymore on the result. Additionally, if the resulting value is large enough as an integer, a wraparound modulo a prime r will happen. The documentation should clarify this behavior.

Documentation comments for OutputUint32 and OutputUint64

The documentation comments for OutputUint32 and OutputUint64 in sdk/circuit_api.go of the sdk repository are the same as for OutputUint and have not been adjusted.

Arguments of SlotOfArrayElement

The SlotOfArrayElement function in the sdk repository's sdk/circuit_api.go is implemented and documented as follows:

// SlotOfArrayElement computes the storage slot for an element in a solidity
// array state variable. arrSlot is the plain slot of the array variable.
// index determines the array index. offset determines the
// offset (in terms of bytes32) within each array element.
func (api *CircuitAPI) SlotOfArrayElement(arrSlot Bytes32, elementSize int, index, offset Uint248) Bytes32 {
	//api.Uint248.AssertIsLessOrEqual(offset, ConstUint248(elementSize))
	o := api.g.Mul(index.Val, elementSize)
	return Bytes32{Val: [2]variable{
		api.g.Add(arrSlot.Val[0], o, offset.Val),
		arrSlot.Val[1],
	}}
}

In Solidity's storage layout for dynamic arrays, the storage slot of a dynamic array itself contains the length of the array. The actual data of the array then begins at the address given by the Keccak hash of the address of the slot of the array itself. See the Solidity docs↗ for more details.

In the function SlotOfArrayElement above, arrSlot should be the address of the slot at which the data of the array starts, not the address of the slot in which the length of the array is stored, so the caller must have already taken the Keccak hash. This is not made clear in the documenting comment and should be clarified, in particular as the related SlotOfStructFieldInMapping function does expect the slot of the state variable itself and takes Keccak itself (which cannot be avoided in that case, as the hash also includes the key).

Additionally, it should be clarified that elementSize is intended as the number of storage slots each element uses, not, for example, the size in bytes. Similarly, offset is also counted in slots, but this is already clarified by the current documentation comment.

Should a single element of the dynamic array require 16 or fewer bytes, then more than one element will be packed into the same slot. As elementSize must be a nonnegative integer, it is not possible to use SlotOfArrayElement to compute addresses in the array in such cases. It would be good to document this limitation as well.

Finally, the way the two limbs of the result are calculated does not account for potential overflows of the lower limb. Thus, the result will not be correct should the sum of the lower 248 bits of the array's initial address and o + offset.Val be bigger than or equal to $2^{248}$. However, o and offset.Val will usually be much smaller than 248 bits wide, so arrSlot.Val[0] would need to be very close to $2^{248}$ for this to happen. As normally this would be the lower 248 bits of a Keccak hash, this is extremely improbable to happen, so this should not be an issue in practice. It could be useful to document the reasoning as comments in or above the function to clarify that this edge case has been considered and the current implementation is a deliberate compromise.

Argument of SlotOfStructFieldInMapping

Similarly, the offset argument for the SlotOfStructFieldInMapping function in the sdk repository's sdk/circuit_api.go is described as index of the struct value. It would be useful to clarify that this is an offset counted in slots.

Sum instead of product symbol in assertInputUniqueness comment

In the function assertInputUniqueness in sdk/host_circuit.go of the sdk repository, there is the following comment:

// Grand product check. Asserts the following equation holds:
// ∑_{a \in in} a+ɣ = ∑_{b \in sorted} b+ɣ

Instead of ∑, it should be ∏.

Endianess in common/util/utils.go

The sdk repository's file common/util/utils.go contains functions Recompose6BytesToNibbles, Recompose32ByteToNibbles, and RecomposeSDKByte32ToNibble, for which we recommend to document what endianess arguments and return values have.

Success not constrained in decode

In the zk-hash repository, in mux/multiplexer.go, the decode function is used to convert a selector into a bitmask. It does not check that the selector is within the range of the bitmask width, so if the selector is out of range, the zero bitmask will be returned. However, the function returns a Boolean outputSuccess indicating whether the selector was in range or not. Thus, a caller can ensure that at least one output is selected by constraining outputSuccess to be 1, as is done by the Multiplex function. We recommend to explain this in the documentation comment of decode, as based on the current comment one may think that the function already constrains input this way.

Incomplete comment regarding Poseidon constants

The zk-hash file poseidon/circuit/constants.go constrains the following comment at the top regarding the parameters for the Poseidon hash function defined in that file:

// Parameters are generated by a reference script https://extgit.iaik.tugraz.at/krypto/hadeshash/-/blob/master/code/generate_parameters_grain.sage
// Used like so: sage generate_parameters_grain.sage 1 0 254 2 8 56 0x30644e72e131a029b85045b68181585d2833e84879b9709143e1f593f0000001

This comment is not fully complete, however. While the parameters ultimately come from that script (though generate_parameters_grain.sage is deprecated, and generate_params_poseidon.sage should be used instead according to the authors), they are modified to work with the optimized algorithm used by the circuit. For example, the round constants apart from those for the first round are different than the ones generated by the script. See Discussion section ref↗ for a little more on this and an explanation for how we verified that the constants in constants.go are correct.

Assumption that 8 divides inBits is not documented for Pad101Bits

In the zk-hash implementation of the pad10*1 padding for Keccak in keccak/pad.go, the Pad101Bits function assumes that inBits divides 8, but this is not documented.

Keccak functions require padded input

The zk-hash repository offers multiple functions to calculate the Keccak hash (Keccak256BitsOptimized, Keccak256Bits, and Keccak256). These all require input that has already been padded to a multiple of the block size of 1,088 bits with the pad10*1 padding function, for which an implementation Pad101Bits is available in keccak/pad.go.

We recommend to document for these Keccak functions that they require padded input.