From 4d9af821a00cc07b550c98f7560399594a28236c Mon Sep 17 00:00:00 2001 From: maramihali Date: Wed, 12 Apr 2023 16:35:06 +0000 Subject: [PATCH 1/3] add constraints to ensure that the result of the logic constraint is indeed produced from the left and right operands of the functions and improve testings. --- .../plonk/composer/ultra_composer.test.cpp | 30 +++ .../stdlib/primitives/logic/logic.cpp | 73 ++++-- .../stdlib/primitives/logic/logic.hpp | 18 +- .../stdlib/primitives/logic/logic.test.cpp | 210 ++++++++++++------ 4 files changed, 235 insertions(+), 96 deletions(-) diff --git a/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp b/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp index 75d30ce519..6207ec399b 100644 --- a/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp +++ b/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp @@ -793,4 +793,34 @@ TYPED_TEST(ultra_composer, range_checks_on_duplicates) TestFixture::prove_and_verify(composer, /*expected_result=*/true); } +// Issue appearing in under-constrained circuit i.e. in which variables: +// - do not appear in any gate of the circuit +// - are smaller than 2^14 (hence not sliced and reconstructed through addition gates) +// - BUT have range constraints applied on them (which, by default, do not add gates to the circuit +// but check sets are correctly sorted) +// The test further shows that the witnesses have different indices hence the problem is not caused by +// the range constraint being applied to the same witness twice. +// TODO: look further into the problem and exemplify it better +TEST(ultra_composer, range_constraint_fails) +{ + auto composer = UltraComposer(); + uint16_t mask = (1 << 8) - 1; + int a = engine.get_random_uint16() & mask; + uint32_t a_idx = composer.add_variable(fr(a)); + uint32_t b_idx = composer.add_variable(fr(a)); + ASSERT_NE(a_idx, b_idx); + uint32_t c_idx = composer.add_variable(fr(a)); + ASSERT_NE(c_idx, b_idx); + composer.create_range_constraint(b_idx, 8, "bad range"); + composer.assert_equal(a_idx, b_idx); + composer.create_range_constraint(c_idx, 8, "bad range"); + composer.assert_equal(a_idx, c_idx); + + auto prover = composer.create_prover(); + auto proof = prover.construct_proof(); + auto verifier = composer.create_verifier(); + bool result = verifier.verify_proof(proof); + EXPECT_EQ(result, false); +} + } // namespace proof_system::plonk::test_ultra_composer diff --git a/cpp/src/barretenberg/stdlib/primitives/logic/logic.cpp b/cpp/src/barretenberg/stdlib/primitives/logic/logic.cpp index 47d1d848f0..c1822638d7 100644 --- a/cpp/src/barretenberg/stdlib/primitives/logic/logic.cpp +++ b/cpp/src/barretenberg/stdlib/primitives/logic/logic.cpp @@ -2,13 +2,18 @@ #include "../composers/composers.hpp" #include "../plookup/plookup.hpp" +#include "barretenberg/common/assert.hpp" +#include "barretenberg/numeric/uint256/uint256.hpp" +#include "barretenberg/stdlib/primitives/field/field.hpp" +#include namespace proof_system::plonk::stdlib { /** * @brief A logical AND or XOR over a variable number of bits. * - * @details Defaults to basic Composer method if not using plookup-compatible composer + * @details Defaults to basic Composer method if not using plookup-compatible composer. If the left and right operands + * are larger than num_bit, the result will be truncated to num_bits. * * @tparam Composer * @param a @@ -18,10 +23,17 @@ namespace proof_system::plonk::stdlib { * @return field_t */ template -field_t logic::create_logic_constraint(field_pt& a, field_pt& b, size_t num_bits, bool is_xor_gate) +field_t logic::create_logic_constraint( + field_pt& a, + field_pt& b, + size_t num_bits, + bool is_xor_gate, + const std::function(uint256_t, uint256_t, size_t)>& get_chunk) { - // can't extend past field size! + // ensure the number of bits doesn't exceed field size and is not negatove ASSERT(num_bits < 254); + ASSERT(num_bits > 0); + if (a.is_constant() && b.is_constant()) { uint256_t a_native(a.get_value()); uint256_t b_native(b.get_value()); @@ -31,59 +43,76 @@ field_t logic::create_logic_constraint(field_pt& a, field_pt if (a.is_constant() && !b.is_constant()) { Composer* ctx = b.get_context(); uint256_t a_native(a.get_value()); - field_t a_witness = field_pt::from_witness_index(ctx, ctx->put_constant_variable(a_native)); - return create_logic_constraint(a_witness, b, num_bits, is_xor_gate); + field_pt a_witness = field_pt::from_witness_index(ctx, ctx->put_constant_variable(a_native)); + return create_logic_constraint(a_witness, b, num_bits, is_xor_gate, get_chunk); } if (!a.is_constant() && b.is_constant()) { Composer* ctx = a.get_context(); uint256_t b_native(b.get_value()); field_pt b_witness = field_pt::from_witness_index(ctx, ctx->put_constant_variable(b_native)); - return create_logic_constraint(a, b_witness, num_bits, is_xor_gate); + return create_logic_constraint(a, b_witness, num_bits, is_xor_gate, get_chunk); } if constexpr (Composer::type == ComposerType::PLOOKUP) { Composer* ctx = a.get_context(); const size_t num_chunks = (num_bits / 32) + ((num_bits % 32 == 0) ? 0 : 1); - uint256_t left(a.get_value()); - uint256_t right(b.get_value()); + auto left((uint256_t)a.get_value()); + auto right((uint256_t)b.get_value()); + + field_pt a_accumulator(barretenberg::fr::zero()); + field_pt b_accumulator(barretenberg::fr::zero()); field_pt res(ctx, 0); for (size_t i = 0; i < num_chunks; ++i) { - uint256_t left_chunk = left & ((uint256_t(1) << 32) - 1); - uint256_t right_chunk = right & ((uint256_t(1) << 32) - 1); - - const field_pt a_chunk = witness_pt(ctx, left_chunk); - const field_pt b_chunk = witness_pt(ctx, right_chunk); + size_t chunk_size = (i != num_chunks - 1) ? 32 : num_bits - i * 32; + auto [left_chunk, right_chunk] = get_chunk(left, right, chunk_size); + field_pt a_chunk = witness_pt(ctx, left_chunk); + field_pt b_chunk = witness_pt(ctx, right_chunk); field_pt result_chunk = 0; if (is_xor_gate) { result_chunk = stdlib::plookup_read::read_from_2_to_1_table(plookup::MultiTableId::UINT32_XOR, a_chunk, b_chunk); + } else { result_chunk = stdlib::plookup_read::read_from_2_to_1_table(plookup::MultiTableId::UINT32_AND, a_chunk, b_chunk); } - uint256_t scaling_factor = uint256_t(1) << (32 * i); - res += result_chunk * scaling_factor; + auto scaling_factor = uint256_t(1) << (32 * i); + a_accumulator += a_chunk * scaling_factor; + b_accumulator += b_chunk * scaling_factor; - if (i == num_chunks - 1) { - const size_t final_num_bits = num_bits - (i * 32); - if (final_num_bits != 32) { - ctx->create_range_constraint(a_chunk.witness_index, final_num_bits, "bad range on a"); - ctx->create_range_constraint(b_chunk.witness_index, final_num_bits, "bad range on b"); - } + if (chunk_size != 32) { + ctx->create_range_constraint( + a_chunk.witness_index, chunk_size, "stdlib logic: bad range on final chunk of left operand"); + ctx->create_range_constraint( + b_chunk.witness_index, chunk_size, "stdlib logic: bad range on final chunk of right operand"); } + res += result_chunk * scaling_factor; + left = left >> 32; right = right >> 32; } + field_pt a_slice = a.slice(static_cast(num_bits - 1), 0)[1]; + field_pt b_slice = b.slice(static_cast(num_bits - 1), 0)[1]; + a_slice.assert_equal(a_accumulator, "stdlib logic: failed to reconstruct left operand"); + b_slice.assert_equal(b_accumulator, "stdlib logic: failed to reconstruct right operand"); return res; } else { + // If the composer doesn't have lookups we call the expensive logic constraint gate + // which creates constraints for each bit. We only create constraints up to num_bits. Composer* ctx = a.get_context(); + uint256_t left = a.get_value(); + uint256_t right = b.get_value(); + left = left & ((uint256_t(1) << num_bits) - 1); + right = right & ((uint256_t(1) << num_bits) - 1); + field_pt a_chunk = witness_pt(ctx, left); + field_pt b_chunk = witness_pt(ctx, right); auto accumulator_triple = ctx->create_logic_constraint( - a.normalize().get_witness_index(), b.normalize().get_witness_index(), num_bits, is_xor_gate); + a_chunk.normalize().get_witness_index(), b_chunk.normalize().get_witness_index(), num_bits, is_xor_gate); auto out_idx = accumulator_triple.out[accumulator_triple.out.size() - 1]; return field_t::from_witness_index(ctx, out_idx); } diff --git a/cpp/src/barretenberg/stdlib/primitives/logic/logic.hpp b/cpp/src/barretenberg/stdlib/primitives/logic/logic.hpp index 2b8b99d560..1ec450de00 100644 --- a/cpp/src/barretenberg/stdlib/primitives/logic/logic.hpp +++ b/cpp/src/barretenberg/stdlib/primitives/logic/logic.hpp @@ -1,17 +1,31 @@ #pragma once +#include "barretenberg/numeric/uint256/uint256.hpp" #include "barretenberg/stdlib/primitives/composers/composers_fwd.hpp" #include "barretenberg/stdlib/primitives/field/field.hpp" #include "barretenberg/stdlib/primitives/witness/witness.hpp" +#include +#include +#include namespace proof_system::plonk::stdlib { template class logic { - private: + public: using field_pt = field_t; using witness_pt = witness_t; public: - static field_pt create_logic_constraint(field_pt& a, field_pt& b, size_t num_bits, bool is_xor_gate); + static field_pt create_logic_constraint( + field_pt& a, + field_pt& b, + size_t num_bits, + bool is_xor_gate, + const std::function(uint256_t, uint256_t, size_t)>& get_chunk = + [](uint256_t left, uint256_t right, size_t chunk_size) { + uint256_t left_chunk = left & ((uint256_t(1) << chunk_size) - 1); + uint256_t right_chunk = right & ((uint256_t(1) << chunk_size) - 1); + return std::make_pair(left_chunk, right_chunk); + }); }; EXTERN_STDLIB_TYPE(logic); diff --git a/cpp/src/barretenberg/stdlib/primitives/logic/logic.test.cpp b/cpp/src/barretenberg/stdlib/primitives/logic/logic.test.cpp index bf1dd73e5f..188a32980c 100644 --- a/cpp/src/barretenberg/stdlib/primitives/logic/logic.test.cpp +++ b/cpp/src/barretenberg/stdlib/primitives/logic/logic.test.cpp @@ -1,6 +1,7 @@ #include "../bool/bool.hpp" +#include "barretenberg/numeric/uint256/uint256.hpp" +#include "barretenberg/proof_system/types/composer_type.hpp" #include "logic.hpp" -#include "barretenberg/plonk/proof_system/constants.hpp" #include #include "barretenberg/honk/composer/standard_honk_composer.hpp" #include "barretenberg/plonk/composer/standard_composer.hpp" @@ -8,6 +9,15 @@ #include "barretenberg/plonk/composer/turbo_composer.hpp" #include "barretenberg/numeric/random/engine.hpp" +#pragma GCC diagnostic ignored "-Wunused-local-typedefs" + +#define STDLIB_TYPE_ALIASES \ + using Composer = TypeParam; \ + using witness_ct = stdlib::witness_t; \ + using field_ct = stdlib::field_t; \ + using bool_ct = stdlib::bool_t; \ + using public_witness_ct = stdlib::public_witness_t; + namespace test_stdlib_logic { namespace { @@ -19,81 +29,137 @@ template void ignore_unused(T&) {} // use to ignore unused variables i using namespace barretenberg; using namespace proof_system::plonk; -template class stdlib_logic : public testing::Test { - typedef stdlib::bool_t bool_ct; - typedef stdlib::field_t field_ct; - typedef stdlib::witness_t witness_ct; - typedef stdlib::public_witness_t public_witness_ct; - - public: - /** - * @brief Test logic - */ - static void test_logic() - { - Composer composer; - auto run_test = [&](size_t num_bits) { - uint256_t mask = (uint256_t(1) << num_bits) - 1; - - uint256_t a = engine.get_random_uint256() & mask; - uint256_t b = engine.get_random_uint256() & mask; - - uint256_t and_expected = a & b; - uint256_t xor_expected = a ^ b; - - field_ct x = witness_ct(&composer, a); - field_ct y = witness_ct(&composer, b); - - field_ct x_const(&composer, a); - field_ct y_const(&composer, b); - field_ct and_result = stdlib::logic::create_logic_constraint(x, y, num_bits, false); - field_ct xor_result = stdlib::logic::create_logic_constraint(x, y, num_bits, true); - - field_ct and_result_left_constant = - stdlib::logic::create_logic_constraint(x_const, y, num_bits, false); - field_ct xor_result_left_constant = - stdlib::logic::create_logic_constraint(x_const, y, num_bits, true); - - field_ct and_result_right_constant = - stdlib::logic::create_logic_constraint(x, y_const, num_bits, false); - field_ct xor_result_right_constant = - stdlib::logic::create_logic_constraint(x, y_const, num_bits, true); - - field_ct and_result_both_constant = - stdlib::logic::create_logic_constraint(x_const, y_const, num_bits, false); - field_ct xor_result_both_constant = - stdlib::logic::create_logic_constraint(x_const, y_const, num_bits, true); - - EXPECT_EQ(uint256_t(and_result.get_value()), and_expected); - EXPECT_EQ(uint256_t(and_result_left_constant.get_value()), and_expected); - EXPECT_EQ(uint256_t(and_result_right_constant.get_value()), and_expected); - EXPECT_EQ(uint256_t(and_result_both_constant.get_value()), and_expected); - - EXPECT_EQ(uint256_t(xor_result.get_value()), xor_expected); - EXPECT_EQ(uint256_t(xor_result_left_constant.get_value()), xor_expected); - EXPECT_EQ(uint256_t(xor_result_right_constant.get_value()), xor_expected); - EXPECT_EQ(uint256_t(xor_result_both_constant.get_value()), xor_expected); - }; - - for (size_t i = 8; i < 248; i += 8) { - run_test(i); - } - auto prover = composer.create_prover(); - plonk::proof proof = prover.construct_proof(); - auto verifier = composer.create_verifier(); - bool result = verifier.verify_proof(proof); +template class LogicTest : public testing::Test {}; - EXPECT_EQ(result, true); - } -}; +using ComposerTypes = + ::testing::Types; + +TYPED_TEST_SUITE(LogicTest, ComposerTypes); + +TYPED_TEST(LogicTest, TestCorrectLogic) +{ + STDLIB_TYPE_ALIASES + + auto run_test = [](size_t num_bits, Composer& composer) { + uint256_t mask = (uint256_t(1) << num_bits) - 1; + + uint256_t a = engine.get_random_uint256() & mask; + uint256_t b = engine.get_random_uint256() & mask; + + uint256_t and_expected = a & b; + uint256_t xor_expected = a ^ b; + + field_ct x = witness_ct(&composer, a); + field_ct y = witness_ct(&composer, b); + + field_ct x_const(&composer, a); + field_ct y_const(&composer, b); + + field_ct and_result = stdlib::logic::create_logic_constraint(x, y, num_bits, false); + field_ct xor_result = stdlib::logic::create_logic_constraint(x, y, num_bits, true); + + field_ct and_result_left_constant = + stdlib::logic::create_logic_constraint(x_const, y, num_bits, false); + field_ct xor_result_left_constant = + stdlib::logic::create_logic_constraint(x_const, y, num_bits, true); -typedef testing::Types - ComposerTypes; + field_ct and_result_right_constant = + stdlib::logic::create_logic_constraint(x, y_const, num_bits, false); + field_ct xor_result_right_constant = + stdlib::logic::create_logic_constraint(x, y_const, num_bits, true); -TYPED_TEST_SUITE(stdlib_logic, ComposerTypes); + field_ct and_result_both_constant = + stdlib::logic::create_logic_constraint(x_const, y_const, num_bits, false); + field_ct xor_result_both_constant = + stdlib::logic::create_logic_constraint(x_const, y_const, num_bits, true); -TYPED_TEST(stdlib_logic, test_logic) + EXPECT_EQ(uint256_t(and_result.get_value()), and_expected); + EXPECT_EQ(uint256_t(and_result_left_constant.get_value()), and_expected); + EXPECT_EQ(uint256_t(and_result_right_constant.get_value()), and_expected); + EXPECT_EQ(uint256_t(and_result_both_constant.get_value()), and_expected); + + EXPECT_EQ(uint256_t(xor_result.get_value()), xor_expected); + EXPECT_EQ(uint256_t(xor_result_left_constant.get_value()), xor_expected); + EXPECT_EQ(uint256_t(xor_result_right_constant.get_value()), xor_expected); + EXPECT_EQ(uint256_t(xor_result_both_constant.get_value()), xor_expected); + }; + + auto composer = Composer(); + for (size_t i = 8; i < 248; i += 8) { + run_test(8, composer); + } + auto prover = composer.create_prover(); + plonk::proof proof = prover.construct_proof(); + auto verifier = composer.create_verifier(); + bool result = verifier.verify_proof(proof); + EXPECT_EQ(result, true); +} + +// Tests the constraints will fail if the operands are larger than expected even though the result contains the correct +// number of bits when using the UltraComposer This is because the range constraints on the right and left operand are +// not being satisfied. +TYPED_TEST(LogicTest, LargeOperands) +{ + STDLIB_TYPE_ALIASES + auto composer = Composer(); + + uint256_t mask = (uint256_t(1) << 48) - 1; + uint256_t a = engine.get_random_uint256() & mask; + uint256_t b = engine.get_random_uint256() & mask; + + uint256_t expected_mask = (uint256_t(1) << 40) - 1; + uint256_t and_expected = (a & b) & expected_mask; + uint256_t xor_expected = (a ^ b) & expected_mask; + + field_ct x = witness_ct(&composer, a); + field_ct y = witness_ct(&composer, b); + + field_ct xor_result = stdlib::logic::create_logic_constraint(x, y, 40, true); + field_ct and_result = stdlib::logic::create_logic_constraint(x, y, 40, false); + EXPECT_EQ(uint256_t(and_result.get_value()), and_expected); + EXPECT_EQ(uint256_t(xor_result.get_value()), xor_expected); + + auto prover = composer.create_prover(); + plonk::proof proof = prover.construct_proof(); + auto verifier = composer.create_verifier(); + bool result = verifier.verify_proof(proof); + EXPECT_EQ(result, true); +} + +// Ensures that malicious witnesses which produce the same result are detected. This potential security issue cannot +// happen if the composer doesn't support lookup gates because constraints will be created for each bit of the left and +// right operand. +TYPED_TEST(LogicTest, DifferentWitnessSameResult) { - TestFixture::test_logic(); + + STDLIB_TYPE_ALIASES + auto composer = Composer(); + if (Composer::type == ComposerType::PLOOKUP) { + uint256_t a = 3758096391; + uint256_t b = 2147483649; + field_ct x = witness_ct(&composer, uint256_t(a)); + field_ct y = witness_ct(&composer, uint256_t(b)); + + uint256_t xor_expected = a ^ b; + const std::function(uint256_t, uint256_t, size_t)>& get_bad_chunk = + [](uint256_t left, uint256_t right, size_t chunk_size) { + (void)left; + (void)right; + (void)chunk_size; + auto left_chunk = uint256_t(2684354565); + auto right_chunk = uint256_t(3221225475); + return std::make_pair(left_chunk, right_chunk); + }; + + field_ct xor_result = stdlib::logic::create_logic_constraint(x, y, 32, true, get_bad_chunk); + EXPECT_EQ(uint256_t(xor_result.get_value()), xor_expected); + + auto prover = composer.create_prover(); + plonk::proof proof = prover.construct_proof(); + auto verifier = composer.create_verifier(); + bool result = verifier.verify_proof(proof); + EXPECT_EQ(result, false); + } } + } // namespace test_stdlib_logic \ No newline at end of file From a6b79878471127c1566d1388c4efef0ca0016e46 Mon Sep 17 00:00:00 2001 From: zac-williamson Date: Mon, 24 Apr 2023 11:39:39 +0200 Subject: [PATCH 2/3] fixed error where applying copy constraints to range-constrained indices would create an unbalanced set membership check --- .../barretenberg/plonk/composer/ultra_composer.cpp | 14 ++++++++++++-- .../barretenberg/plonk/composer/ultra_composer.hpp | 2 +- .../plonk/composer/ultra_composer.test.cpp | 2 +- 3 files changed, 14 insertions(+), 4 deletions(-) diff --git a/cpp/src/barretenberg/plonk/composer/ultra_composer.cpp b/cpp/src/barretenberg/plonk/composer/ultra_composer.cpp index b3846ce36d..d565401db8 100644 --- a/cpp/src/barretenberg/plonk/composer/ultra_composer.cpp +++ b/cpp/src/barretenberg/plonk/composer/ultra_composer.cpp @@ -1256,11 +1256,21 @@ void UltraComposer::create_new_range_constraint(const uint32_t variable_index, } } -void UltraComposer::process_range_list(const RangeList& list) +void UltraComposer::process_range_list(RangeList& list) { assert_valid_variables(list.variable_indices); ASSERT(list.variable_indices.size() > 0); + + // replace witness index in variable_indices with the real variable index (i.e. not an index that is a copied) + for (uint32_t& x : list.variable_indices) { + x = real_variable_index[x]; + } + // remove duplicate witness indices to prevent the sorted list set size being wrong! + std::sort(list.variable_indices.begin(), list.variable_indices.end()); + auto back_iterator = std::unique(list.variable_indices.begin(), list.variable_indices.end()); + list.variable_indices.erase(back_iterator, list.variable_indices.end()); + // go over variables // for each variable, create mirror variable with same value - with tau tag // need to make sure that, in original list, increments of at most 3 @@ -1297,7 +1307,7 @@ void UltraComposer::process_range_list(const RangeList& list) void UltraComposer::process_range_lists() { - for (const auto& i : range_lists) + for (auto& i : range_lists) process_range_list(i.second); } diff --git a/cpp/src/barretenberg/plonk/composer/ultra_composer.hpp b/cpp/src/barretenberg/plonk/composer/ultra_composer.hpp index 1ce6813d40..9543be93c3 100644 --- a/cpp/src/barretenberg/plonk/composer/ultra_composer.hpp +++ b/cpp/src/barretenberg/plonk/composer/ultra_composer.hpp @@ -447,7 +447,7 @@ class UltraComposer : public ComposerBase { } RangeList create_range_list(const uint64_t target_range); - void process_range_list(const RangeList& list); + void process_range_list(RangeList& list); void process_range_lists(); /** diff --git a/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp b/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp index 6207ec399b..1bbd8b265d 100644 --- a/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp +++ b/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp @@ -820,7 +820,7 @@ TEST(ultra_composer, range_constraint_fails) auto proof = prover.construct_proof(); auto verifier = composer.create_verifier(); bool result = verifier.verify_proof(proof); - EXPECT_EQ(result, false); + EXPECT_EQ(result, true); } } // namespace proof_system::plonk::test_ultra_composer From 8a1f3b2aa7166d6f2f4c320fb00fd7aa32354f6d Mon Sep 17 00:00:00 2001 From: maramihali Date: Mon, 24 Apr 2023 10:59:11 +0000 Subject: [PATCH 3/3] improve documentation and fix issue in non-ultra variants of the composer --- .../splitting_tmp/ultra_plonk_composer.test.cpp | 2 +- .../barretenberg/plonk/composer/ultra_composer.cpp | 6 ++++-- .../barretenberg/plonk/composer/ultra_composer.hpp | 2 ++ .../plonk/composer/ultra_composer.test.cpp | 14 +++++--------- .../barretenberg/stdlib/primitives/logic/logic.cpp | 13 +++++-------- 5 files changed, 17 insertions(+), 20 deletions(-) diff --git a/cpp/src/barretenberg/plonk/composer/splitting_tmp/ultra_plonk_composer.test.cpp b/cpp/src/barretenberg/plonk/composer/splitting_tmp/ultra_plonk_composer.test.cpp index cc90c8de1e..e7bc91da4e 100644 --- a/cpp/src/barretenberg/plonk/composer/splitting_tmp/ultra_plonk_composer.test.cpp +++ b/cpp/src/barretenberg/plonk/composer/splitting_tmp/ultra_plonk_composer.test.cpp @@ -914,4 +914,4 @@ TEST(ultra_plonk_composer_splitting_tmp, ram) EXPECT_EQ(result, true); } -} // namespace proof_system::plonk +} // namespace proof_system::plonk \ No newline at end of file diff --git a/cpp/src/barretenberg/plonk/composer/ultra_composer.cpp b/cpp/src/barretenberg/plonk/composer/ultra_composer.cpp index d565401db8..e0a1e3d803 100644 --- a/cpp/src/barretenberg/plonk/composer/ultra_composer.cpp +++ b/cpp/src/barretenberg/plonk/composer/ultra_composer.cpp @@ -1262,7 +1262,9 @@ void UltraComposer::process_range_list(RangeList& list) ASSERT(list.variable_indices.size() > 0); - // replace witness index in variable_indices with the real variable index (i.e. not an index that is a copied) + // replace witness index in variable_indices with the real variable index i.e. if a copy constraint has been + // applied on a variable after it was range constrained, this makes sure the indices in list point to the updated + // index in the range list so the set equivalence does not fail for (uint32_t& x : list.variable_indices) { x = real_variable_index[x]; } @@ -1272,7 +1274,7 @@ void UltraComposer::process_range_list(RangeList& list) list.variable_indices.erase(back_iterator, list.variable_indices.end()); // go over variables - // for each variable, create mirror variable with same value - with tau tag + // iterate over each variable and create mirror variable with same value - with tau tag // need to make sure that, in original list, increments of at most 3 std::vector sorted_list; sorted_list.reserve(list.variable_indices.size()); diff --git a/cpp/src/barretenberg/plonk/composer/ultra_composer.hpp b/cpp/src/barretenberg/plonk/composer/ultra_composer.hpp index 9543be93c3..852184272d 100644 --- a/cpp/src/barretenberg/plonk/composer/ultra_composer.hpp +++ b/cpp/src/barretenberg/plonk/composer/ultra_composer.hpp @@ -58,6 +58,8 @@ class UltraComposer : public ComposerBase { uint64_t target_range; uint32_t range_tag; uint32_t tau_tag; + // contains the list of variable indices that are range constrained to target_range + // as ordered in the vector of variable indoces from the composer std::vector variable_indices; }; diff --git a/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp b/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp index 1bbd8b265d..18cdc2f44d 100644 --- a/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp +++ b/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp @@ -793,15 +793,11 @@ TYPED_TEST(ultra_composer, range_checks_on_duplicates) TestFixture::prove_and_verify(composer, /*expected_result=*/true); } -// Issue appearing in under-constrained circuit i.e. in which variables: -// - do not appear in any gate of the circuit -// - are smaller than 2^14 (hence not sliced and reconstructed through addition gates) -// - BUT have range constraints applied on them (which, by default, do not add gates to the circuit -// but check sets are correctly sorted) -// The test further shows that the witnesses have different indices hence the problem is not caused by -// the range constraint being applied to the same witness twice. -// TODO: look further into the problem and exemplify it better -TEST(ultra_composer, range_constraint_fails) +// Ensure copy constraints added on variables smaller than 2^14, which have been previously +// range constrained, do not break the set equivalence checks because of indices mismatch. +// 2^14 is DEFAULT_PLOOKUP_RANGE_BITNUM i.e. the maximum size before a variable gets sliced +// before range constraints are applied to it. +TEST(ultra_composer, range_constraint_small_variable) { auto composer = UltraComposer(); uint16_t mask = (1 << 8) - 1; diff --git a/cpp/src/barretenberg/stdlib/primitives/logic/logic.cpp b/cpp/src/barretenberg/stdlib/primitives/logic/logic.cpp index c1822638d7..033ec3b9d2 100644 --- a/cpp/src/barretenberg/stdlib/primitives/logic/logic.cpp +++ b/cpp/src/barretenberg/stdlib/primitives/logic/logic.cpp @@ -13,7 +13,8 @@ namespace proof_system::plonk::stdlib { * @brief A logical AND or XOR over a variable number of bits. * * @details Defaults to basic Composer method if not using plookup-compatible composer. If the left and right operands - * are larger than num_bit, the result will be truncated to num_bits. + * are larger than num_bit, the result will be truncated to num_bits. However, the two operands could be + * range-constrained to num_bits before the call which would remove the need to range constrain inside this function. * * @tparam Composer * @param a @@ -105,14 +106,10 @@ field_t logic::create_logic_constraint( // If the composer doesn't have lookups we call the expensive logic constraint gate // which creates constraints for each bit. We only create constraints up to num_bits. Composer* ctx = a.get_context(); - uint256_t left = a.get_value(); - uint256_t right = b.get_value(); - left = left & ((uint256_t(1) << num_bits) - 1); - right = right & ((uint256_t(1) << num_bits) - 1); - field_pt a_chunk = witness_pt(ctx, left); - field_pt b_chunk = witness_pt(ctx, right); + field_pt a_slice = a.slice(static_cast(num_bits - 1), 0)[1]; + field_pt b_slice = b.slice(static_cast(num_bits - 1), 0)[1]; auto accumulator_triple = ctx->create_logic_constraint( - a_chunk.normalize().get_witness_index(), b_chunk.normalize().get_witness_index(), num_bits, is_xor_gate); + a_slice.normalize().get_witness_index(), b_slice.normalize().get_witness_index(), num_bits, is_xor_gate); auto out_idx = accumulator_triple.out[accumulator_triple.out.size() - 1]; return field_t::from_witness_index(ctx, out_idx); }