From cee6505004af826a64bf766a3df84b83993590bf Mon Sep 17 00:00:00 2001 From: zac-williamson Date: Thu, 20 Apr 2023 17:50:00 +0200 Subject: [PATCH 1/4] fixed bug where range constraining connected witnesses threw an error --- .../plonk/composer/ultra_composer.cpp | 6 ++-- .../plonk/composer/ultra_composer.hpp | 4 +++ .../plonk/composer/ultra_composer.test.cpp | 35 +++++++++++++++++++ 3 files changed, 43 insertions(+), 2 deletions(-) diff --git a/cpp/src/barretenberg/plonk/composer/ultra_composer.cpp b/cpp/src/barretenberg/plonk/composer/ultra_composer.cpp index 8782f6ab61..d17939fba1 100644 --- a/cpp/src/barretenberg/plonk/composer/ultra_composer.cpp +++ b/cpp/src/barretenberg/plonk/composer/ultra_composer.cpp @@ -1217,8 +1217,10 @@ void UltraComposer::create_new_range_constraint(const uint32_t variable_index, } auto& list = range_lists[target_range]; - assign_tag(variable_index, list.range_tag); - list.variable_indices.emplace_back(variable_index); + if (real_variable_tags[real_variable_index[variable_index]] != list.range_tag) { + assign_tag(variable_index, list.range_tag); + list.variable_indices.emplace_back(variable_index); + } } void UltraComposer::process_range_list(const RangeList& list) diff --git a/cpp/src/barretenberg/plonk/composer/ultra_composer.hpp b/cpp/src/barretenberg/plonk/composer/ultra_composer.hpp index b3157639fd..1ce6813d40 100644 --- a/cpp/src/barretenberg/plonk/composer/ultra_composer.hpp +++ b/cpp/src/barretenberg/plonk/composer/ultra_composer.hpp @@ -425,6 +425,10 @@ class UltraComposer : public ComposerBase { void assign_tag(const uint32_t variable_index, const uint32_t tag) { ASSERT(tag <= current_tag); + // If we've already assigned this tag to this variable, return (can happen due to copy constraints) + if (real_variable_tags[real_variable_index[variable_index]] == tag) { + return; + } ASSERT(real_variable_tags[real_variable_index[variable_index]] == DUMMY_TAG); real_variable_tags[real_variable_index[variable_index]] = tag; } diff --git a/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp b/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp index eef80fa56c..f3f87047af 100644 --- a/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp +++ b/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp @@ -758,4 +758,39 @@ TYPED_TEST(ultra_composer, ram) TestFixture::prove_and_verify(composer, /*expected_result=*/true); } +TYPED_TEST(ultra_composer, range_checks_on_duplicates) +{ + UltraComposer composer = UltraComposer(); + + uint32_t a = composer.add_variable(100); + uint32_t b = composer.add_variable(100); + uint32_t c = composer.add_variable(100); + uint32_t d = composer.add_variable(100); + + composer.assert_equal(a, b); + composer.assert_equal(a, c); + composer.assert_equal(a, d); + + composer.create_new_range_constraint(a, 1000); + composer.create_new_range_constraint(b, 1000); + composer.create_new_range_constraint(c, 1000); + composer.create_new_range_constraint(d, 1000); + + composer.create_big_add_gate( + { + a, + b, + c, + d, + 0, + 0, + 0, + 0, + 0, + }, + true); + + TestFixture::prove_and_verify(composer, /*expected_result=*/true); +} + } // namespace proof_system::plonk::test_ultra_composer From 4e86e8f4ec48fc1b608f483bfd69a342a4928b57 Mon Sep 17 00:00:00 2001 From: zac-williamson Date: Thu, 20 Apr 2023 18:03:51 +0200 Subject: [PATCH 2/4] fix --- cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp b/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp index f3f87047af..8a96d54652 100644 --- a/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp +++ b/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp @@ -788,7 +788,7 @@ TYPED_TEST(ultra_composer, range_checks_on_duplicates) 0, 0, }, - true); + false); TestFixture::prove_and_verify(composer, /*expected_result=*/true); } From a42e10447b40ac17b433caa04887ae189890cbc9 Mon Sep 17 00:00:00 2001 From: zac-williamson Date: Thu, 20 Apr 2023 18:21:09 +0200 Subject: [PATCH 3/4] bugfix on create_new_range_constraint can now apply multiple overlapping ranges to the same witness (or copies of the witness) --- .../plonk/composer/ultra_composer.cpp | 30 +++++++++++++++++++ .../plonk/composer/ultra_composer.test.cpp | 4 +-- 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/cpp/src/barretenberg/plonk/composer/ultra_composer.cpp b/cpp/src/barretenberg/plonk/composer/ultra_composer.cpp index d17939fba1..2b1cd9ec2c 100644 --- a/cpp/src/barretenberg/plonk/composer/ultra_composer.cpp +++ b/cpp/src/barretenberg/plonk/composer/ultra_composer.cpp @@ -1218,6 +1218,36 @@ void UltraComposer::create_new_range_constraint(const uint32_t variable_index, auto& list = range_lists[target_range]; if (real_variable_tags[real_variable_index[variable_index]] != list.range_tag) { + + if (real_variable_tags[real_variable_index[variable_index]] != DUMMY_TAG) { + const auto existing_tag = real_variable_tags[real_variable_index[variable_index]]; + bool found_tag = false; + // existing range? + for (const auto& r : range_lists) { + if (r.second.range_tag == existing_tag) { + found_tag = true; + if (r.first < target_range) { + // already has a more restrictive range check, skip + return; + } else { + // difficult to remove an existing range check. + // Instead deep-copy the variable and apply a range check to new variable + const uint32_t copied_witness = add_variable(get_variable(variable_index)); + create_add_gate({ .a = variable_index, + .b = copied_witness, + .c = zero_idx, + .a_scaling = 1, + .b_scaling = -1, + .c_scaling = 0, + .const_scaling = 0 }); + // recursve w. new witness that has no tag attached + create_new_range_constraint(copied_witness, target_range, msg); + return; + } + } + } + ASSERT(found_tag == true); + } assign_tag(variable_index, list.range_tag); list.variable_indices.emplace_back(variable_index); } diff --git a/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp b/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp index 8a96d54652..75d30ce519 100644 --- a/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp +++ b/cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp @@ -772,8 +772,8 @@ TYPED_TEST(ultra_composer, range_checks_on_duplicates) composer.assert_equal(a, d); composer.create_new_range_constraint(a, 1000); - composer.create_new_range_constraint(b, 1000); - composer.create_new_range_constraint(c, 1000); + composer.create_new_range_constraint(b, 1001); + composer.create_new_range_constraint(c, 999); composer.create_new_range_constraint(d, 1000); composer.create_big_add_gate( From b7af09f75af85fa8adad354f4e5a1d8e0b0b8ad8 Mon Sep 17 00:00:00 2001 From: codygunton Date: Thu, 20 Apr 2023 18:43:04 +0000 Subject: [PATCH 4/4] Comments and change for clarity. --- .../plonk/composer/ultra_composer.cpp | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/cpp/src/barretenberg/plonk/composer/ultra_composer.cpp b/cpp/src/barretenberg/plonk/composer/ultra_composer.cpp index 2b1cd9ec2c..b3846ce36d 100644 --- a/cpp/src/barretenberg/plonk/composer/ultra_composer.cpp +++ b/cpp/src/barretenberg/plonk/composer/ultra_composer.cpp @@ -1216,22 +1216,25 @@ void UltraComposer::create_new_range_constraint(const uint32_t variable_index, range_lists.insert({ target_range, create_range_list(target_range) }); } + const auto existing_tag = real_variable_tags[real_variable_index[variable_index]]; auto& list = range_lists[target_range]; - if (real_variable_tags[real_variable_index[variable_index]] != list.range_tag) { - if (real_variable_tags[real_variable_index[variable_index]] != DUMMY_TAG) { - const auto existing_tag = real_variable_tags[real_variable_index[variable_index]]; + // If the variable's tag matches the target range list's tag, do nothing. + if (existing_tag != list.range_tag) { + // If the variable is 'untagged' (i.e., it has the dummy tag), assign it the appropriate tag. + // Otherwise, find the range for which the variable has already been tagged. + if (existing_tag != DUMMY_TAG) { bool found_tag = false; - // existing range? for (const auto& r : range_lists) { if (r.second.range_tag == existing_tag) { found_tag = true; if (r.first < target_range) { - // already has a more restrictive range check, skip + // The variable already has a more restrictive range check, so do nothing. return; } else { - // difficult to remove an existing range check. - // Instead deep-copy the variable and apply a range check to new variable + // The range constraint we are trying to impose is more restrictive than the existing range + // constraint. It would be difficult to remove an existing range check. Instead deep-copy the + // variable and apply a range check to new variable const uint32_t copied_witness = add_variable(get_variable(variable_index)); create_add_gate({ .a = variable_index, .b = copied_witness, @@ -1240,7 +1243,7 @@ void UltraComposer::create_new_range_constraint(const uint32_t variable_index, .b_scaling = -1, .c_scaling = 0, .const_scaling = 0 }); - // recursve w. new witness that has no tag attached + // Recurse with new witness that has no tag attached. create_new_range_constraint(copied_witness, target_range, msg); return; }