bug in sha256_ethereum.tcc | get_hash

[kaxxa123]

Hello there is a little bug in this code:
https://github.com/clearmatics/zeth/blob/master/libzeth/circuits/sha256/sha256_ethereum.tcc

Function:
template
libff::bit_vector sha256_ethereum::get_hash(
const libff::bit_vector &input)

This function is missing the ZERO parameter
sha256_ethereum eth_hasher(
pb, input_block, output_variable, "eth_hasher_gadget");

This causes a compile time error so its not serious.

I fixed this as follows:

template
libff::bit_vector sha256_ethereum::get_hash(
const libff::bit_vector &input)
{
libsnark::protoboard pb;

libsnark::pb_variable<FieldT> ZERO;
ZERO.allocate(pb, "ZERO");

libsnark::block_variable<FieldT> input_block(
    pb, libsnark::SHA256_block_size, "input_block");
libsnark::digest_variable<FieldT> output_variable(
    pb, libsnark::SHA256_digest_size, "output_variable");
sha256_ethereum<FieldT> eth_hasher(
    pb, ZERO, input_block, output_variable, "eth_hasher_gadget");

pb.val(ZERO) = FieldT::zero();
input_block.generate_r1cs_witness(input);
eth_hasher.generate_r1cs_witness();

return output_variable.get_digest();

}

[dtebbs]

Thanks for raising this @kaxxa123, and for the code snippet. I guess that gadget has not been used for a while and needs some attention (and a test). Feel free to submit a pull request with your fix.

[AntoineRondelet]

Thanks @kaxxa123! Good catch, the function signature is not satisfied indeed justifying the compilation error.

Rather than quick fixing this and add ZERO back, we should probably bring the sha256 hasher interface in sync with the blake one (which doesn't use ZERO), see:

zeth/libzeth/circuits/blake2s/blake2s.hpp

Lines 149 to 153 in 8247fc3

	BLAKE2s_256(
	libsnark::protoboard<FieldT> &pb,
	const libsnark::block_variable<FieldT> &input,
	const libsnark::digest_variable<FieldT> &output,
	const std::string &annotation_prefix = "blake2s_gadget");

Removing ZERO from the arguments of the sha constructor begs for a refactoring of this "legacy" function that uses ONE and ZERO. So we need to get rid of the useless directives at the top of the file and use FieldT::one(), FieldT::zero() apropriately here:

zeth/libzeth/circuits/circuit_utils.tcc

Lines 51 to 64 in 8247fc3

	// Takes a vector of boolean values, and convert this vector of boolean values
	// into a vector of FieldT::zero() and FieldT:one()
	template<typename FieldT>
	libsnark::pb_variable_array<FieldT> variable_array_from_bit_vector(
	const std::vector<bool> &bits, const libsnark::pb_variable<FieldT> &ZERO)
	{
	libsnark::pb_variable_array<FieldT> acc;
	acc.reserve(bits.size());
	for (bool bit : bits) {
	acc.emplace_back(bit ? ONE : ZERO);
	}
	return acc;
	};

In fact, we should probably use the bits class here and the associated method:

zeth/libzeth/core/bits.tcc

Lines 107 to 119 in 8247fc3

	template<size_t numBits>
	template<typename FieldT>
	void bits<numBits>::fill_variable_array(
	libsnark::protoboard<FieldT> &pb,
	libsnark::pb_variable_array<FieldT> &var_array) const
	{
	if (var_array.size() != numBits) {
	throw std::invalid_argument("invalid pb_variable_array size");
	}
	for (size_t i = 0; i < numBits; ++i) {
	pb.val(var_array[i]) = ((*this)[i]) ? FieldT::one() : FieldT::zero();
	}
	}

to fill the pb_variable_array, effectively removing this legacy code that turns out to trigger compilation error.

Let us know if you want to contribute to the project and get a shot at fixing this ticket (if not, no worries at all, we'll close this ticket on our end :) )

[kaxxa123]

In that case I will leave the fix to you, since for the moment I am still learning about your various gadgets as I work on my dissertation :)

[dtebbs]

Yes, at some point the code needs to be updated to avoid the ONE and ZERO variables. Happy to do this as a separate change if you did want to submit a PR @kaxxa123. Otherwise we'll keep this issue as a reminder to do it all at once. Either way, thanks again.

[AntoineRondelet]

In that case I will leave the fix to you, since for the moment I am still learning about your various gadgets as I work on my dissertation :)

No problem! We'll fix this on our end :)

[AntoineRondelet]

This issue was fixed in #391, closing the ticket as a consequence.