Minor recommendations

In this section we collect some minor recommendations.

String function for lists

The String function for lists just places a comma and space consecutively (", ") between each component. Adding, for example, the left square bracket "[" at the start and the right square bracket "]" at the end, similarly to how it is done for tuples with the left and right parentheses "(" and ")", would make it easier to understand the output.

Typo in one of the constants for Poseidon implementation in zk-hash

In the zk-hash repository's poseidon/circuit/constants.go, the very first string, in the function strPOSEIDON_C and for case t == 2, which should consist of a flat list, begins correctly with a single left square bracket "[" introducing the list, but it ends with two right square brackets, "]". Both of the bracket characters are replaced by an empty string in the function parseOneDimensionArray that is used to parse this string, which is why this does not cause an error on runtime.

	s := ""
	if t == 2 {
		s =
			`[0x9c46e9ec68e9bd4fe1faaba294cba38a71aa177534cdd1b6c7dc0dbd0abd7a7,
// ...
0xe3eca007699dd0f852eb22da642e495f67c988dd5bf0137676b16a31eab4667]
]`

We recommend to remove the last right square bracket, ].

Ambiguous genericity in ToUint521

The ToUint521 in the brevis-sdk file sdk/circuit_api.go has the cases for Bytes32 and Uint248 implemented as follows:

case Bytes32:
	// Recompose the Bytes32 into BigField.NbLimbs limbs
	bits := v.toBinaryVars(api.g)
	f := Uint521Field{}
	limbs := make([]variable, f.NbLimbs())
	b := f.BitsPerLimb()
	limbs[0] = api.g.FromBinary(bits[:b]...)
	limbs[1] = api.g.FromBinary(bits[b : 2*b]...)
	limbs[2] = api.g.FromBinary(bits[2*b:]...)
	limbs[3], limbs[4], limbs[5] = 0, 0, 0
	el := api.Uint521.f.NewElement(limbs)
	return newU521(el)
case Uint248:
	el := api.Uint521.f.NewElement([]variable{v.Val, 0, 0, 0, 0, 0})
	return newU521(el)

Note that in the Bytes32 case, the implementation starts in a way that does not hardcode the number of limbs of a Uint521, by instantiating limbs with f.NbLimbs() components instead of a hardcoded 6. However, the code following this assumes that this is what the value was; if f.NbLimbs() were smaller, one of the assignments would panic due to an out-of-bounds write attempt. In the second case shown, Uint248 constructs the result, assuming six limbs as well.

Similarly, the function appears to be independent of the precise value of f.BitsPerLimbs() but makes the implicit assumption that 3*b >= 256 so that the 256 bits from Bytes32 fits into the first three limbs.

We recommend to make the code more consistent by either hardcoding actual values throughout in this function or making the implementation independent of the values.

In the former case, it would be good to assert that the values used are equal to f.NbLimbs() and f.BitsPerLimb() to ensure that, should one or both change later, it will not be forgotten to update the ToUint521 function.

String representation of LogField is empty

In the sdk repository's sdk/circuit_input.go, the type LogField and its method function String is defined. They are given by the following:

// LogField represents a single field of an event.
type LogField struct {
	// The contract from which the event is emitted
	Contract Uint248
	// The event ID of the event to which the field belong (aka topics[0])
	EventID Uint248
	// Whether the field is a topic (aka "indexed" as in solidity events)
	IsTopic Uint248
	// The index of the field. For example, if a field is the second topic of a log, then Index is 1; if a field is the
	// third field in the RLP decoded data, then Index is 2.
	Index Uint248
	// The value of the field in event, aka the actual thing we care about, only 32-byte fixed length values are supported.
	Value Bytes32
}

func (f LogField) String() string { return "" }

The string representation of a LogField is thus just the empty string. In contrast, similar types have nonempty string representations:

func (s StorageSlot) String() string { return "StorageSlot" }

func (r Receipt) String() string { return "Receipt" }

// ...

It may be considered to return a descriptive string for LogField as well (such as "LogField").

Inconsistent serialization of Transaction struct

In the sdk repository, a Transaction struct is defined in sdk/circuit_input.go as follows:

type Transaction struct {
	ChainId  Uint248
	BlockNum Uint32
	Nonce    Uint248
	// GasTipCapOrGasPrice is GasPrice for legacy tx (type 0) and GasTipCapOap for
	// dynamic-fee tx (type 2)
	GasTipCapOrGasPrice Uint248
	// GasFeeCap is always 0 for legacy tx
	GasFeeCap Uint248
	GasLimit  Uint248
	From      Uint248
	To        Uint248
	Value     Bytes32
}

This struct is serialized with a pack function:

func (t Transaction) pack(api frontend.API) []variable {
	var bits []variable
	bits = append(bits, api.ToBinary(t.BlockNum.Val, 8*4)...)
	bits = append(bits, api.ToBinary(t.ChainId.Val, 8*4)...)
	bits = append(bits, api.ToBinary(t.Nonce.Val, 8*4)...)
	bits = append(bits, api.ToBinary(t.GasTipCapOrGasPrice.Val, 8*8)...)
	bits = append(bits, api.ToBinary(t.GasFeeCap.Val, 8*8)...)
	bits = append(bits, api.ToBinary(t.GasLimit.Val, 8*4)...)
	bits = append(bits, api.ToBinary(t.From.Val, 8*20)...)
	bits = append(bits, api.ToBinary(t.To.Val, 8*20)...)
	bits = append(bits, t.Value.toBinaryVars(api)...)
	return packBitsToFr(api, bits)
}

Note that in pack, the field BlockNum is serialized before ChainId, while in the struct these fields are listed in the other order. This inconsistency is not a problem in itself, but it could cause confusion later in development, so we recommend to keep the order consistent.

Incorrect error message in assignToggleCommitment

In the sdk repository's sdk/app.go, in assignToggleCommitment, there is the following snippet:

result, err := hasher.Sum()
if err != nil {
	panic(fmt.Sprintf("invalid toggles length %d", len(toggles)))
}

The error message was likely copied from another error earlier in the function and does not fit this error. We recommend to update the error message to something like the following:

panic(fmt.Sprintf("failed to hash toggle bit leaf: %s", err.Error()))

Incomplete or misleading error messages in validateInput

The validateInput function in the sdk/host_circuit.go file of the sdk repository carries out some sanity checks regarding the inputs. It is implemented as follows:

func (c *HostCircuit) validateInput() error {
	d := c.Input
	inputLen := len(d.Receipts.Raw) + len(d.StorageSlots.Raw) + len(d.Transactions.Raw)
	if inputLen > NumMaxDataPoints {
		return fmt.Errorf("input len must be less than %d", NumMaxDataPoints)
	}
	maxReceipts, maxSlots, maxTransactions := c.Guest.Allocate()
	if len(d.Receipts.Raw) != len(d.Receipts.Toggles) || len(d.Receipts.Raw) != maxReceipts {
		return fmt.Errorf("receipt input/toggle len mismatch: %d vs %d",
			len(d.Receipts.Raw), len(d.Receipts.Toggles))
	}
	if len(d.StorageSlots.Raw) != len(d.StorageSlots.Toggles) || len(d.StorageSlots.Raw) != maxSlots {
		return fmt.Errorf("storageSlots input/toggle len mismatch: %d vs %d",
			len(d.StorageSlots.Raw), len(d.StorageSlots.Toggles))
	}
	if len(d.Transactions.Raw) != len(d.Transactions.Toggles) || len(d.Transactions.Raw) != maxTransactions {
		return fmt.Errorf("transaction input/toggle len mismatch: %d vs %d",
			len(d.Transactions.Raw), len(d.Transactions.Toggles))
	}
	return nil
}

The first check is to ensure that the total number of inputs is not higher than the maximum that is allowed, NumMaxDataPoints. The error message fmt.Errorf("input len must be less than %d", NumMaxDataPoints) is inaccurate as it suggests that an input length that is equal to the maximum is not allowed, while it is. Thus, fmt.Errorf("input len must be less than or equal to %d", NumMaxDataPoints) would be more accurate.

The other three checks are all analogous and check that for each of the three input types, the amount of raw data entries matches the configured allocation and that this is also equal to the number of toggles. The error messages, however, only mention the input and toggle lengths and suggest that they mismatch, when the error could also have arisen due to a mismatch with the configured maximum amount. Updating the first error message to

fmt.Errorf("mismatch between receipt input length, toggle length, or allocated receipt length: %d vs %d vs %d",
			len(d.Receipts.Raw), len(d.Receipts.Toggles), maxReceipts)

would make it more accurate and useful — analogously for the other two types.

Typo in buildSendRequestCalldata error message

The buildSendRequestCalldata function in sdk/app.go of the sdk repository has a typo in an error message, where fonud is used instead of found.

Typo in calMerkelRoot function name

The function calMerkelRoot in the sdk repository's sdk/host_circuit.go is used to calculate the root hash of a Merkle tree. The function name has a typo however, using "Merkel" instead of "Merkle".

Ethereum header Nonce not zero

In the zk-bridge repository's circuits/fabric/headers/util.go, the function genHeader generates an Ethereum header with dummy values. The field Nonce is filled with the byte 1:

Nonce:           [8]byte{1, 1, 1, 1, 1, 1, 1, 1},

The Ethereum yellow paper describes the Nonce field as follows: "A 64-bit value that is now deprecated due to the replacement of proof of work consensus. It is set to 0x0000000000000000; formally Hn."

For this reason, usage of a dummy value of all zero might be better.

Environment variables ignored in NewService

In the sdk repository, the following function in sdk/prover/server.go

func NewService(app sdk.AppCircuit, config ServiceConfig) (*Service, error) {
	pk, vk, ccs, err := readOrSetup(app, config.SetupDir, config.GetSrsDir())
	if err != nil {
		return nil, err
	}
	return &Service{
		svr: newServer(app, pk, vk, ccs),
	}, nil
}

should likely use config.GetSetupDir() instead of config.SetupDir to handle usage of environmental variables in the path correctly, similarly to how it is done for the SRS dir with config.GetSrsDir().

Dead code in Poseidon implementation

The function MixS in the zk-hash repository's poseidon/circuit/circuit.go is never used anywhere and could thus be removed.

Likely unnecessary check in Bytes2Bits

In the zk-hash repository, the Bytes2Bits function in keccak/periphery.go is implemented as follows:

func Bytes2Bits(bytes []byte) (bits []uint8) {
	if len(bytes)%8 != 0 {
		panic("invalid length")
	}
	for i := 0; i < len(bytes); i++ {
		bits = append(bits, byte2Bits(bytes[i])...)
	}
	return
}

It is unclear why the check that 8 divides len(bytes) is performed in this function. If this check is not needed, it should be removed so that the function also becomes usable in other cases. Should this check have meaning in the context of a particular use (for example, when serializing data into a sequence of 64-bit words), consider creating a wrapper with a name reflecting the use case and moving the check into the wrapper.

Keccak functions require round indexes while padding requires input length

The zk-hash repository offers multiple functions to calculate the Keccak hash (Keccak256BitsOptimized, Keccak256Bits, and Keccak256). These require a round index as an argument. This argument roundIndex can be a circuit variable and not just a native constant, so the Keccak functions support variable-length inputs in circuit. The in-circuit padding function Pad101Bits instead gets the length of the input as an argument. This inLen argument can also be a circuit variable, so Pad101Bits supports variable length inputs as well. However, there is no in-circuit implementation of the calculation of the round index from the input length that a user could use. There is a GetRoundIndex function in keccak/periphery.go, but it only carries out this calculation for the native int type. It might make sense to add a function that constrains the calculation of the round index from the input length in circuit.