fix: nullifier tests (#249)

* fixes * fix: more possible regression tests * fix: remove logs * fix: remove more logs * clean: reuse test code * clean * rm done todo --------- Co-authored-by: cheethas <[email protected]>
AztecProtocol · Apr 17, 2023 · f7c09de · f7c09de
1 parent d76b0f3
commit f7c09de
Show file tree

Hide file tree

Showing 5 changed files with 87 additions and 12 deletions.
diff --git a/circuits/cpp/src/aztec3/circuits/rollup/base/.test.cpp b/circuits/cpp/src/aztec3/circuits/rollup/base/.test.cpp
@@ -507,6 +507,65 @@ TEST_F(base_rollup_tests, nullifier_tree_regression)
     ASSERT_EQ(outputs.end_nullifier_tree_snapshot, nullifier_tree_end_snapshot);
 }
 
+void perform_standard_nullifier_test(std::array<fr, KERNEL_NEW_NULLIFIERS_LENGTH * 2> new_nullifiers)
+{
+    // Regression test caught when testing the typescript nullifier tree implementation
+    DummyComposer composer = DummyComposer();
+    BaseRollupInputs empty_inputs = dummy_base_rollup_inputs_with_vk_proof();
+
+    std::vector<fr> initial_values(7, 0);
+    for (size_t i = 0; i < 7; i++) {
+        initial_values[i] = i + 1;
+    }
+
+    std::tuple<BaseRollupInputs, AppendOnlyTreeSnapshot<NT>, AppendOnlyTreeSnapshot<NT>> inputs_and_snapshots =
+        utils::generate_nullifier_tree_testing_values(empty_inputs, new_nullifiers, initial_values);
+    BaseRollupInputs testing_inputs = std::get<0>(inputs_and_snapshots);
+    AppendOnlyTreeSnapshot<NT> nullifier_tree_start_snapshot = std::get<1>(inputs_and_snapshots);
+    AppendOnlyTreeSnapshot<NT> nullifier_tree_end_snapshot = std::get<2>(inputs_and_snapshots);
+
+    /**
+     * RUN
+     */
+
+    // Run the circuit
+    BaseOrMergeRollupPublicInputs outputs =
+        aztec3::circuits::rollup::native_base_rollup::base_rollup_circuit(composer, testing_inputs);
+
+    /**
+     * ASSERT
+     */
+    // Start state
+    ASSERT_EQ(outputs.start_nullifier_tree_snapshot, nullifier_tree_start_snapshot);
+
+    // End state
+    ASSERT_EQ(outputs.end_nullifier_tree_snapshot, nullifier_tree_end_snapshot);
+}
+
+// Another regression test with values from a failing packages test
+TEST_F(base_rollup_tests, nullifier_tree_regression_2)
+{
+    // Regression test caught when testing the typescript nullifier tree implementation
+    std::array<fr, KERNEL_NEW_NULLIFIERS_LENGTH* 2> new_nullifiers = { 0 };
+    new_nullifiers[0] = uint256_t("2a7d956c1365d259646d2d85babe1abb793bb8789e98df7e2336a29a0c91fd01");
+    new_nullifiers[1] = uint256_t("236bf2d113f9ffee89df1a7a04890c9ad3583c6773eb9cdec484184f66abd4c6");
+    new_nullifiers[4] = uint256_t("2f5c8a1ee33c7104b244e22a3e481637cd501c9eae868cfab6b16e3b4ef3d635");
+    new_nullifiers[5] = uint256_t("0c484a20780e31747cf9f4f6803986525ed98ef587f5155a1c50689c2cad10ae");
+
+    perform_standard_nullifier_test(new_nullifiers);
+}
+
+TEST_F(base_rollup_tests, nullifier_tree_regression_3)
+{
+    std::array<fr, KERNEL_NEW_NULLIFIERS_LENGTH* 2> new_nullifiers = { 0 };
+    new_nullifiers[0] = uint256_t("0740a17aa6437e71836d2adcdcb3f52879bb869cdd9c8fb8dc39a12846cd17f2");
+    new_nullifiers[1] = uint256_t("282e0e2f38310a7c7c98b636830b66f3276294560e26ef2499da10892f00af8f");
+    new_nullifiers[4] = uint256_t("0f117936e888bd3befb4435f4d65300d25609e95a3d1563f62ef7e58c294f578");
+    new_nullifiers[5] = uint256_t("0fcb3908cb15ebf8bab276f5df17524d3b676c8655234e4350953c387fffcdd7");
+
+    perform_standard_nullifier_test(new_nullifiers);
+}
+
 // Note leaving this test here as there are no negative tests, even though it no longer passes
 TEST_F(base_rollup_tests, new_nullifier_tree_sparse_attack)
 {

diff --git a/circuits/cpp/src/aztec3/circuits/rollup/base/native_base_rollup_circuit.cpp b/circuits/cpp/src/aztec3/circuits/rollup/base/native_base_rollup_circuit.cpp
@@ -313,11 +313,15 @@ AppendOnlySnapshot check_nullifier_tree_non_membership_and_insert_to_tree(DummyC
                     // check previous nullifier leaves
                     // TODO: this is a hack, and insecure, we need to fix this
                     bool matched = false;
-                    for (size_t k = 0; k < nullifier_index; k++) {
-                        if ((uint256_t(nullifier_insertion_subtree[k].nextValue) > uint256_t(nullifier) &&
-                             uint256_t(nullifier_insertion_subtree[k].value) < uint256_t(nullifier)) ||
-                            (nullifier_insertion_subtree[k].nextValue == 0 &&
-                             nullifier_insertion_subtree[k].nextIndex == 0)) {
+
+                    for (size_t k = 0; k < nullifier_index && !matched; k++) {
+                        if (nullifier_insertion_subtree[k].value == 0) {
+                            continue;
+                        }
+
+                        if ((uint256_t(nullifier_insertion_subtree[k].value) < uint256_t(nullifier)) &&
+                            (uint256_t(nullifier_insertion_subtree[k].nextValue) > uint256_t(nullifier) ||
+                             nullifier_insertion_subtree[k].nextValue == 0)) {
 
                             matched = true;
                             // Update pointers
@@ -329,6 +333,7 @@ AppendOnlySnapshot check_nullifier_tree_non_membership_and_insert_to_tree(DummyC
                             nullifier_insertion_subtree[k].nextValue = nullifier;
                         }
                     }
+
                     // if not matched, our subtree will misformed - we must reject
                     composer.do_assert(matched, "Nullifier subtree is malformed");
 

diff --git a/circuits/cpp/src/aztec3/circuits/rollup/base/nullifier_tree_testing_harness.cpp b/circuits/cpp/src/aztec3/circuits/rollup/base/nullifier_tree_testing_harness.cpp
@@ -12,11 +12,13 @@ NullifierMemoryTreeTestingHarness::NullifierMemoryTreeTestingHarness(size_t dept
 {}
 
 // Check for a larger value in an array
-bool check_has_lesser_value(std::vector<fr> const& values, fr const& value)
+bool check_has_less_than(std::vector<fr> const& values, fr const& value)
 {
+
     // Must perform comparisons on integers
     uint256_t value_as_uint = uint256_t(value);
     for (auto const& v : values) {
+        // info(v, " ", uint256_t(v) < value_as_uint);
         if (uint256_t(v) < value_as_uint) {
             return true;
         }
@@ -108,20 +110,20 @@ NullifierMemoryTreeTestingHarness::circuit_prep_batch_insert(std::vector<fr> con
             sibling_paths.push_back(empty_sp);
             low_nullifier_indexes.push_back(empty_index);
             low_nullifiers.push_back(empty_leaf);
+            continue;
         }
 
         // If the low_nullifier node has been touched this sub tree insertion, we provide a dummy sibling path
         // It will be up to the circuit to check if the included node is valid vs the other nodes that have been
         // inserted before it If it has not been touched, we provide a sibling path then update the nodes pointers
         auto prev_nodes = touched_nodes.find(current);
 
-        bool has_lesser_value = false;
+        bool has_less_than = false;
         if (prev_nodes != touched_nodes.end()) {
-            has_lesser_value = check_has_lesser_value(prev_nodes->second, new_value);
+            has_less_than = check_has_less_than(prev_nodes->second, new_value);
         }
         // If there is a lower value in the tree, we need to check the current low nullifiers for one that can be used
-        // if (current == 0 || has_lesser_value) {
-        if (has_lesser_value) {
+        if (has_less_than) {
 
             for (size_t j = 0; j < pending_insertion_tree.size(); ++j) {
                 // Skip checking empty values
@@ -171,14 +173,22 @@ NullifierMemoryTreeTestingHarness::circuit_prep_batch_insert(std::vector<fr> con
                                         .nextValue = new_value };
 
             // Update the old leaf in the tree
-            update_element(current, new_leaf.hash());
+            // update old value in tree
+            update_element_in_place(current, new_leaf);
         }
     }
 
     // Return tuple of low nullifiers and sibling paths
     return std::make_tuple(low_nullifiers, sibling_paths, low_nullifier_indexes);
 }
 
+void NullifierMemoryTreeTestingHarness::update_element_in_place(size_t index, nullifier_leaf leaf)
+{
+    // Find the leaf with the value closest and less than `value`
+    this->leaves_[index] = leaf;
+    update_element(index, leaf.hash());
+}
+
 std::pair<nullifier_leaf, size_t> NullifierMemoryTreeTestingHarness::find_lower(fr const& value)
 {
     size_t current;

diff --git a/circuits/cpp/src/aztec3/circuits/rollup/base/nullifier_tree_testing_harness.hpp b/circuits/cpp/src/aztec3/circuits/rollup/base/nullifier_tree_testing_harness.hpp
@@ -38,6 +38,8 @@ class NullifierMemoryTreeTestingHarness : public proof_system::plonk::stdlib::me
     // Current size of the tree
     fr size() { return leaves_.size(); }
 
+    void update_element_in_place(size_t index, nullifier_leaf leaf);
+
     // Get all of the sibling paths and low nullifier values required to craft an non membership / inclusion proofs
     std::tuple<std::vector<nullifier_leaf>, std::vector<std::vector<fr>>, std::vector<uint32_t>>
     circuit_prep_batch_insert(std::vector<fr> const& values);

diff --git a/circuits/cpp/src/aztec3/circuits/rollup/base/utils.cpp b/circuits/cpp/src/aztec3/circuits/rollup/base/utils.cpp
@@ -159,7 +159,6 @@ generate_nullifier_tree_testing_values(BaseRollupInputs<NT> rollupInputs,
         }
         insertion_values.push_back(insertion_val);
         reference_tree.append_value(insertion_val);
-        auto hashes = reference_tree.get_hashes();
     }
 
     // Get the hash paths etc from the insertion values