Incorrect hex string and parsing errors canceling each other out

In the sdk repository's sdk/app.go, the function assignInputCommitment has a comment saying that 0 is assigned instead of an actual hash for dummy data:

// assign 0 to input commit for dummy and actual data hash for non-dummies

However, the hash is assigned in the following line

leafs[j] = new(big.Int).SetBytes(common.Hex2Bytes("0x01"))

which appears as though the byte value 1 is assigned, not 0. The comment is correct here that 0 should be assigned instead of 1, as can be seen by comparison to the commitInput function in host_circuit.go.

Even though it does not appear as so, the above line does assign the value 0. The reason is that common.Hex2Bytes ultimately passes the argument through to Go's Decode function in the standard library's encoding/hex/hex.go. This function only accepts hexadecimal digits as characters, so 0 through 9, a through f, and A through F. There is no support for 0x prefixes, so an error will be returned when attempting to parse the character x:

// Decode decodes src into [DecodedLen](len(src)) bytes,
// returning the actual number of bytes written to dst.
//
// Decode expects that src contains only hexadecimal
// characters and that src has even length.
// If the input is malformed, Decode returns the number
// of bytes decoded before the error.
func Decode(dst, src []byte) (int, error) {
	i, j := 0, 1
	for ; j < len(src); j += 2 {
		p := src[j-1]
		q := src[j]

		a := reverseHexTable[p]
		b := reverseHexTable[q]
		if a > 0x0f {
			return i, InvalidByteError(p)
		}
		if b > 0x0f {
!			return i, InvalidByteError(q)
		}
		dst[i] = (a << 4) | b
		i++
	}
    // ...
}

This Decode function is called through DecodeString, which is a thin wrapper passing the error upwards. Instead of returning the byte index at which the parsing error occurred, like Decode, this wrapper returns the bytes decoded so far:

// DecodeString returns the bytes represented by the hexadecimal string s.
//
// DecodeString expects that src contains only hexadecimal
// characters and that src has even length.
// If the input is malformed, DecodeString returns
// the bytes decoded before the error.
func DecodeString(s string) ([]byte, error) {
	src := []byte(s)
	// We can use the source slice itself as the destination
	// because the decode loop increments by one and then the 'seen' byte is not used anymore.
	n, err := Decode(src, src)
	return src[:n], err
}

Finally, the common.Hex2Bytes that is called with 0x01 calls DecodeString, but the error is ignored:

// Hex2Bytes returns the bytes represented by the hexadecimal string str.
func Hex2Bytes(str string) []byte {
	h, _ := hex.DecodeString(str)
	return h
}

Thus, with 0x01, as there will be a parsing error at the x, where no byte has been fully parsed yet, the value of h will be the empty slice. When calling new(big.Int).SetBytes with an empty slice, the integer value obtained from it will be zero.

This explains why

leafs[j] = new(big.Int).SetBytes(common.Hex2Bytes("0x01"))

will set leafs[i] to zero, as it should, even though the code looks like it does not.

Note that in the sdk repository's common/utils/hex.go, there is also a function called Hex2Bytes, which could be confused with the function common.Hex2Bytes discussed so far, which comes from the go-ethereum package. Brevis's Hex2Bytes is implemented as follows:

// Hex2Bytes supports hex string with or without 0x prefix
// Calls hex.DecodeString directly and ignore err
// similar to ec.FromHex but better
func Hex2Bytes(s string) (b []byte) {
	if len(s) >= 2 && s[0] == '0' && (s[1] == 'x' || s[1] == 'X') {
		s = s[2:]
	}
	// hex.DecodeString expects an even-length string
	if len(s)%2 == 1 {
		s = "0" + s
	}
	b, _ = hex.DecodeString(s)
	return b
}

As seen, this function does support the 0x prefix. Should common.Hex2Bytes in

leafs[j] = new(big.Int).SetBytes(common.Hex2Bytes("0x01"))

be replaced by a call to this function Hex2Bytes, then leafs[i] would be assigned 1, which would be incorrect.

Currently, usage of go-ethereum's Hex2Bytes — without ensuring there are no parsing errors — and the incorrect 01 instead of 00 essentially amount to two mistakes that happen to cancel each other out and lead to the correct result. This code can be very confusing to future developers though, which may inadvertently introduce mistakes later. Presence of the local function with the same name that behaves differently makes this particularly risky.

We thus recommend to replace common.Hex2Bytes("0x01") by either common.Hex2Bytes("00") or a call to the common/utils/hex.go version of Hex2Bytes with argument "0x00".