Aztec3 Specific Work in Barretenberg

[suyash67]

Description

This branch contains all aztec3 specific changes that we want to incorporate in barretenberg. The aztec3-circuits repository references to the aztec3 branch in barretenberg.

Description of the changes: https://hackmd.io/R1VTaVC6QC6eBXEcATIeCA?view

Checklist:

• I have reviewed my diff in github, line by line.
• Every change is related to the PR description.
• I have linked this pull request to the issue(s) that it resolves.
• There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
• There are no circuit changes, OR specifications in /markdown/specs have been updated.
• There are no circuit changes, OR a cryptographer has been assigned for review.
• I've updated any terraform that needs updating (e.g. environment variables) for deployment.
• The branch has been rebased against the head of its merge target.
• I'm happy for the PR to be merged at the reviewer's next convenience.
• New functions, classes, etc. have been documented according to the doxygen comment format. Classes and structs must have @brief describing the intended functionality.
• If existing code has been modified, such documentation has been added or updated.

[ledwards2225]

Overall this seems fine, particularly the bulk of the changes related to the Pedersen splitting. Some of the other work going into this seems somewhat incomplete, i.e. lots of functions that appear to be untested/unused in the stdlib. If that is indeed the case, my preference would be to either add tests (now or in an immediate follow-on), or split that work out and get it in separately once it is tested.

[ledwards2225]

As far as I can tell, the functions implemented in this file are not used/tested anywhere. If that's the case, I'd suggest they either be tested or removed until they're needed.

[suyash67]

Added tests here: #262

[ledwards2225]

New function does not appear to be tested/used anywhere.

[suyash67]

Added a test: 952bbf2

[ledwards2225]

Untested/unused?

[suyash67]

Added tests:

barretenberg/cpp/src/barretenberg/stdlib/primitives/bool/bool.test.cpp

Lines 394 to 470 in 0414410

	TEST(stdlib_bool, must_imply_multiple)
	{
	honk::StandardHonkComposer composer = honk::StandardHonkComposer();
	/**
	* Define g(x) = 2x + 12
	* if x is divisible by both 4 and 6:
	* => g(x) > 0
	* => g(x) is even
	* => g(x) >= 12
	* => g(x) is a multiple of 6
	*/
	auto g = [](size_t x) { return 2 * x + 12; };
	for (size_t j = 0; j < 3; ++j) { // ignore when both lhs and rhs are constants
	bool lhs_constant = (bool)(j % 2);
	bool rhs_constant = (bool)(j > 1 ? true : false);
	for (size_t x = 10; x < 18; x += 2) {
	std::vector<std::pair<bool_t, std::string>> conditions;
	bool four = (bool)(x % 4 == 0);
	bool six = (bool)(x % 6 == 0);
	bool_t a = lhs_constant ? bool_t(four) : (witness_t(&composer, four));
	bool_t b = rhs_constant ? bool_t(six) : (witness_t(&composer, six));
	auto g_x = g(x);
	conditions.push_back(std::make_pair(g_x > 0, "g(x) > 0"));
	conditions.push_back(std::make_pair(g_x % 2 == 0, "g(x) is even"));
	conditions.push_back(std::make_pair(g_x >= 12, "g(x) >= 12"));
	conditions.push_back(std::make_pair(g_x % 6 == 0, "g(x) is a multiple of 6"));
	(a && b).must_imply(conditions);
	if (composer.failed()) {
	EXPECT_EQ(composer.err(), "multi implication fail: g(x) is a multiple of 6");
	} else {
	auto prover = composer.create_prover();
	auto verifier = composer.create_verifier();
	plonk::proof proof = prover.construct_proof();
	bool result = verifier.verify_proof(proof);
	EXPECT_EQ(result, true);
	}
	}
	}
	}
	TEST(stdlib_bool, must_imply_multiple_fails)
	{
	honk::StandardHonkComposer composer = honk::StandardHonkComposer();
	/**
	* Given x = 15:
	* (x > 10)
	* => (x > 8)
	* => (x > 5)
	* ≠> (x > 18)
	*/
	for (size_t j = 0; j < 2; ++j) { // ignore when both lhs and rhs are constants
	bool is_constant = (bool)(j % 2);
	size_t x = 15;
	bool main = (bool)(x > 10);
	bool_t main_ct = is_constant ? bool_t(main) : (witness_t(&composer, main));
	std::vector<std::pair<bool_t, std::string>> conditions;
	conditions.push_back(std::make_pair(witness_t(&composer, x > 8), "x > 8"));
	conditions.push_back(std::make_pair(witness_t(&composer, x > 5), "x > 5"));
	conditions.push_back(std::make_pair(witness_t(&composer, x > 18), "x > 18"));
	main_ct.must_imply(conditions);
	EXPECT_EQ(composer.failed(), true);
	EXPECT_EQ(composer.err(), "multi implication fail: x > 18");
	}
	}

[ledwards2225]

Untested/unused?

[suyash67]

Its unused but I've added a test since we might use it:

barretenberg/cpp/src/barretenberg/stdlib/primitives/bool/bool.test.cpp

Lines 472 to 500 in 0414410

	TEST(stdlib_bool, conditional_assign)
	{
	honk::StandardHonkComposer composer = honk::StandardHonkComposer();
	for (size_t j = 0; j < 4; ++j) {
	bool lhs_constant = (bool)(j % 2);
	bool rhs_constant = (bool)(j > 1 ? true : false);
	const uint256_t x = (uint256_t(1) << 128) - 1;
	const uint256_t val = engine.get_random_uint256();
	bool condition = (val % 2 == 0);
	bool right = x < val;
	bool left = x > val;
	bool_t l_ct = lhs_constant ? bool_t(left) : (witness_t(&composer, left));
	bool_t r_ct = rhs_constant ? bool_t(right) : (witness_t(&composer, right));
	bool_t cond = (witness_t(&composer, condition));
	auto result = bool_t::conditional_assign(cond, l_ct, r_ct);
	EXPECT_EQ(result.get_value(), condition ? left : right);
	}
	auto prover = composer.create_prover();
	auto verifier = composer.create_verifier();
	plonk::proof proof = prover.construct_proof();
	info("composer gates = ", composer.get_num_gates());
	bool result = verifier.verify_proof(proof);
	EXPECT_EQ(result, true);
	}

[ledwards2225]

Do we have fixed standalone tests for pedersen_hash::hash_multiple and pedersen_hash::lookup::hash_multiple that do not depend on which Composer the system Composer is set to? I.e. are we always only testing one or the other? If so maybe we need some individual tests.

[suyash67]

So we do not (and have not had in the past too) tests that check the native Pedersen hash. We always check if the native and circuit (i.e. stdlib) Pedersen hashes match. We do have tests that check the hash ladder (in generators module) but we don't need to test native implementation of Pedersen hash. The reason for that is there is no standard for Pedersen hash like we have for SHA and other hashes. In the other native hash implementations, we test against NIST-based test vectors. We don't have that for Pedersen hash. As long as native and stdlib implementations match, we are fine.

[suyash67]

BTW we still do test lookup based Pedersen hash because its design is much simpler. To write tests for fixed-base Pedersen hash is possible but not necessary right now in my opinion.

[ledwards2225]

This is far from the only place this is done in our code base but just want to start to flag this: I think it's universally agreed that using directives like this are a bad idea in header files because then they apply anywhere this header is included. Would be nice to start to get rid of them but this might not be the time/place.

[suyash67]

Agreed. Lets keep this comment for future reference.

[ledwards2225]

Can this magic number 15 be tied to some fundamental constant somewhere? More generally, looks like the tests in this file have never had any comments but it would be really nice to at least have a couple comments on the new tests being added while someone has a really complete view of them in their head.

[suyash67]

Done: d98286d

[suyash67]

Added a comment too.

[ledwards2225]

I'm assuming this is just some leftover debugging code or something? Print statement does not seem to reflect the condition being checked

[suyash67]

Removed the condition, wasn't necessary.

[suyash67]

$\color{red}{\text{TODO:}}$ test this.

[codygunton]

^ Rebase over the last two commits to master, from PRs #323 and #327.

[ledwards2225]

There's a lot going on here but I'm satisfied. Thanks for all the test updates etc @suyash67!