diff --git a/barretenberg/README.md b/barretenberg/README.md index 57cc64e37c00..3907614d0eeb 100644 --- a/barretenberg/README.md +++ b/barretenberg/README.md @@ -287,3 +287,68 @@ python3 -m http.server ``` and tunnel the port through ssh. + +### Debugging Verifification Failures + +The CicuitChecker::check_circuit function is used to get the gate index and block information about a failing circuit constraint. +If you are in a scenario where you have a failing call to check_circuit and wish to get more information out of it than just the gate index, you can use this feature to get a stack trace, see example below. + +Usage instructions: +- On ubuntu (or our mainframe accounts) use `sudo apt-get install libdw-dev` to support trace printing +- Use `cmake --preset clang16-dbg-fast-circuit-check-traces` and `cmake --build --preset clang16-dbg-fast-circuit-check-traces` to enable the backward-cpp dependency through the CHECK_CIRCUIT_STACKTRACES CMake variable. +- Run any case where you have a failing check_circuit call, you will now have a stack trace illuminating where this constraint was added in code. + +Caveats: +- This works best for code that is not overly generic, i.e. where just the sequence of function calls carries a lot of information. It is possible to tag extra data along with the stack trace, this can be done as a followup, please leave feedback if desired. +- There are certain functions like `assert_equals` that can cause gates that occur _before_ them to fail. If this would be useful to automatically report, please leave feedback. + +Example: +``` +[ RUN ] standard_circuit_constructor.test_check_circuit_broken +Stack trace (most recent call last): +#4 Source "_deps/gtest-src/googletest/src/gtest.cc", line 2845, in Run + 2842: if (!Test::HasFatalFailure() && !Test::IsSkipped()) { + 2843: // This doesn't throw as all user code that can throw are wrapped into + 2844: // exception handling code. + >2845: test->Run(); + 2846: } + 2847: + 2848: if (test != nullptr) { +#3 Source "_deps/gtest-src/googletest/src/gtest.cc", line 2696, in Run + 2693: // GTEST_SKIP(). + 2694: if (!HasFatalFailure() && !IsSkipped()) { + 2695: impl->os_stack_trace_getter()->UponLeavingGTest(); + >2696: internal::HandleExceptionsInMethodIfSupported(this, &Test::TestBody, + 2697: "the test body"); + 2698: } +#2 | Source "_deps/gtest-src/googletest/src/gtest.cc", line 2657, in HandleSehExceptionsInMethodIfSupported + | 2655: #if GTEST_HAS_EXCEPTIONS + | 2656: try { + | >2657: return HandleSehExceptionsInMethodIfSupported(object, method, location); + | 2658: } catch (const AssertionException&) { // NOLINT + | 2659: // This failure was reported already. + Source "_deps/gtest-src/googletest/src/gtest.cc", line 2621, in HandleExceptionsInMethodIfSupported + 2618: } + 2619: #else + 2620: (void)location; + >2621: return (object->*method)(); + 2622: #endif // GTEST_HAS_SEH + 2623: } +#1 Source "/mnt/user-data/adam/aztec-packages/barretenberg/cpp/src/barretenberg/circuit_checker/standard_circuit_builder.test.cpp", line 464, in TestBody + 461: uint32_t d_idx = circuit_constructor.add_variable(d); + 462: circuit_constructor.create_add_gate({ a_idx, b_idx, c_idx, fr::one(), fr::one(), fr::neg_one(), fr::zero() }); + 463: + > 464: circuit_constructor.create_add_gate({ d_idx, c_idx, a_idx, fr::one(), fr::neg_one(), fr::neg_one(), fr::zero() }); + 465: + 466: bool result = CircuitChecker::check(circuit_constructor); + 467: EXPECT_EQ(result, false); +#0 Source "/mnt/user-data/adam/aztec-packages/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/standard_circuit_builder.cpp", line 22, in create_add_gate + 19: { + 20: this->assert_valid_variables({ in.a, in.b, in.c }); + 21: + > 22: blocks.arithmetic.populate_wires(in.a, in.b, in.c); + 23: blocks.arithmetic.q_m().emplace_back(FF::zero()); + 24: blocks.arithmetic.q_1().emplace_back(in.a_scaling); + 25: blocks.arithmetic.q_2().emplace_back(in.b_scaling); +gate number4 +``` \ No newline at end of file diff --git a/barretenberg/cpp/CMakeLists.txt b/barretenberg/cpp/CMakeLists.txt index f277ebfa089f..9e1ad5d4ced8 100644 --- a/barretenberg/cpp/CMakeLists.txt +++ b/barretenberg/cpp/CMakeLists.txt @@ -19,7 +19,7 @@ configure_file( # Add doxygen build command find_package(Doxygen) if (DOXYGEN_FOUND) -add_custom_target(build_docs +add_custom_target(build_docs COMMAND ${DOXYGEN_EXECUTABLE} ${PROJECT_SOURCE_DIR}/docs/Doxyfile WORKING_DIRECTORY ${PROJECT_SOURCE_DIR} COMMENT "Generate documentation with Doxygen") @@ -34,6 +34,8 @@ option(DISABLE_TBB "Intel Thread Building Blocks" ON) option(COVERAGE "Enable collecting coverage from tests" OFF) option(ENABLE_ASAN "Address sanitizer for debugging tricky memory corruption" OFF) option(ENABLE_HEAVY_TESTS "Enable heavy tests when collecting coverage" OFF) +# Note: Must do 'sudo apt-get install libdw-dev' or equivalent +option(CHECK_CIRCUIT_STACKTRACES "Enable (slow) stack traces for check circuit" OFF) if(CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64" OR CMAKE_SYSTEM_PROCESSOR MATCHES "arm64") message(STATUS "Compiling for ARM.") @@ -45,6 +47,10 @@ if(CMAKE_SYSTEM_PROCESSOR MATCHES "aarch64" OR CMAKE_SYSTEM_PROCESSOR MATCHES "a set(DISABLE_TBB 0) endif() +if(CHECK_CIRCUIT_STACKTRACES) + add_compile_options(-DCHECK_CIRCUIT_STACKTRACES) +endif() + if(ENABLE_ASAN) add_compile_options(-fsanitize=address) add_link_options(-fsanitize=address) @@ -136,6 +142,8 @@ include(cmake/gtest.cmake) include(cmake/benchmark.cmake) include(cmake/module.cmake) include(cmake/msgpack.cmake) +include(cmake/backward-cpp.cmake) + add_subdirectory(src) if (ENABLE_ASAN AND NOT(FUZZING)) find_program(LLVM_SYMBOLIZER_PATH NAMES llvm-symbolizer-16) diff --git a/barretenberg/cpp/CMakePresets.json b/barretenberg/cpp/CMakePresets.json index 4b4639a17691..b18d0604fd6d 100644 --- a/barretenberg/cpp/CMakePresets.json +++ b/barretenberg/cpp/CMakePresets.json @@ -96,6 +96,16 @@ "LDFLAGS": "-O2 -gdwarf-4" } }, + { + "name": "clang16-dbg-fast-circuit-check-traces", + "displayName": "Optimized debug build with Clang-16 with stack traces for failing circuit checks", + "description": "Build with globally installed Clang-16 in optimized debug mode with stack traces for failing circuit checks", + "inherits": "clang16-dbg-fast", + "binaryDir": "build-debug-fast-circuit-check-traces", + "cacheVariables": { + "CHECK_CIRCUIT_STACKTRACES": "ON" + } + }, { "name": "clang16-assert", "binaryDir": "build-assert", @@ -405,6 +415,11 @@ "inherits": "default", "configurePreset": "clang16-dbg-fast" }, + { + "name": "clang16-dbg-fast-circuit-check-traces", + "inherits": "clang16-dbg-fast", + "configurePreset": "clang16-dbg-fast-circuit-check-traces" + }, { "name": "clang16-assert", "inherits": "default", @@ -480,12 +495,7 @@ "configurePreset": "wasm", "inheritConfigureEnvironment": true, "jobs": 0, - "targets": [ - "barretenberg.wasm", - "barretenberg", - "wasi", - "env" - ] + "targets": ["barretenberg.wasm", "barretenberg", "wasi", "env"] }, { "name": "wasm-dbg", diff --git a/barretenberg/cpp/cmake/backward-cpp.cmake b/barretenberg/cpp/cmake/backward-cpp.cmake new file mode 100644 index 000000000000..b0d70b5e2cba --- /dev/null +++ b/barretenberg/cpp/cmake/backward-cpp.cmake @@ -0,0 +1,11 @@ +if(CHECK_CIRCUIT_STACKTRACES) + include(FetchContent) + + # Also requires one of: libbfd (gnu binutils), libdwarf, libdw (elfutils) + FetchContent_Declare(backward + GIT_REPOSITORY https://github.com/bombela/backward-cpp + GIT_TAG 51f0700452cf71c57d43c2d028277b24cde32502 + SYSTEM # optional, the Backward include directory will be treated as system directory + ) + FetchContent_MakeAvailable(backward) +endif() \ No newline at end of file diff --git a/barretenberg/cpp/cmake/module.cmake b/barretenberg/cpp/cmake/module.cmake index 71fa5cd7dbed..8365188dc2fe 100644 --- a/barretenberg/cpp/cmake/module.cmake +++ b/barretenberg/cpp/cmake/module.cmake @@ -56,6 +56,19 @@ function(barretenberg_module MODULE_NAME) ${TBB_IMPORTED_TARGETS} ) + if(CHECK_CIRCUIT_STACKTRACES) + target_link_libraries( + ${MODULE_NAME}_objects + PUBLIC + Backward::Interface + ) + target_link_options( + ${MODULE_NAME} + PRIVATE + -ldw -lelf + ) + endif() + # enable msgpack downloading via dependency (solves race condition) add_dependencies(${MODULE_NAME} msgpack-c) add_dependencies(${MODULE_NAME}_objects msgpack-c) @@ -88,6 +101,19 @@ function(barretenberg_module MODULE_NAME) ) list(APPEND exe_targets ${MODULE_NAME}_tests) + if(CHECK_CIRCUIT_STACKTRACES) + target_link_libraries( + ${MODULE_NAME}_test_objects + PUBLIC + Backward::Interface + ) + target_link_options( + ${MODULE_NAME}_tests + PRIVATE + -ldw -lelf + ) + endif() + if(WASM) target_link_options( ${MODULE_NAME}_tests @@ -229,6 +255,18 @@ function(barretenberg_module MODULE_NAME) benchmark::benchmark ${TBB_IMPORTED_TARGETS} ) + if(CHECK_CIRCUIT_STACKTRACES) + target_link_libraries( + ${BENCHMARK_NAME}_bench_objects + PUBLIC + Backward::Interface + ) + target_link_options( + ${BENCHMARK_NAME}_bench + PRIVATE + -ldw -lelf + ) + endif() # enable msgpack downloading via dependency (solves race condition) add_dependencies(${BENCHMARK_NAME}_bench_objects msgpack-c) diff --git a/barretenberg/cpp/src/CMakeLists.txt b/barretenberg/cpp/src/CMakeLists.txt index 4d0468e3962f..96832e6a8421 100644 --- a/barretenberg/cpp/src/CMakeLists.txt +++ b/barretenberg/cpp/src/CMakeLists.txt @@ -188,4 +188,16 @@ if(WASM) -nostartfiles -Wl,--no-entry,--export-dynamic ) + if(CHECK_CIRCUIT_STACKTRACES) + target_link_libraries( + barretenberg.wasm + PUBLIC + Backward::Interface + ) + target_link_options( + barretenberg.wasm + PRIVATE + -ldw -lelf + ) + endif() endif() diff --git a/barretenberg/cpp/src/barretenberg/bb/CMakeLists.txt b/barretenberg/cpp/src/barretenberg/bb/CMakeLists.txt index dc17b35d0012..031861aa4c22 100644 --- a/barretenberg/cpp/src/barretenberg/bb/CMakeLists.txt +++ b/barretenberg/cpp/src/barretenberg/bb/CMakeLists.txt @@ -12,4 +12,16 @@ if (NOT(FUZZING)) barretenberg env ) + if(CHECK_CIRCUIT_STACKTRACES) + target_link_libraries( + bb + PUBLIC + Backward::Interface + ) + target_link_options( + bb + PRIVATE + -ldw -lelf + ) + endif() endif() \ No newline at end of file diff --git a/barretenberg/cpp/src/barretenberg/circuit_checker/standard_circuit_checker.hpp b/barretenberg/cpp/src/barretenberg/circuit_checker/standard_circuit_checker.hpp index 545f49967285..f3ad6e648e88 100644 --- a/barretenberg/cpp/src/barretenberg/circuit_checker/standard_circuit_checker.hpp +++ b/barretenberg/cpp/src/barretenberg/circuit_checker/standard_circuit_checker.hpp @@ -25,6 +25,9 @@ class StandardCircuitChecker { FF gate_sum = block.q_m()[i] * left * right + block.q_1()[i] * left + block.q_2()[i] * right + block.q_3()[i] * output + block.q_c()[i]; if (!gate_sum.is_zero()) { +#ifdef CHECK_CIRCUIT_STACKTRACES + block.stack_traces.print(i); +#endif info("gate number", i); return false; } diff --git a/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_builder.test.cpp b/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_builder.test.cpp index 483f1c52966c..6f814a95844d 100644 --- a/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_builder.test.cpp +++ b/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_builder.test.cpp @@ -323,7 +323,7 @@ TEST(ultra_circuit_constructor, sort_with_edges_gate) fr h = fr(8); { - UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder(); + UltraCircuitBuilder circuit_constructor; auto a_idx = circuit_constructor.add_variable(a); auto b_idx = circuit_constructor.add_variable(b); auto c_idx = circuit_constructor.add_variable(c); @@ -339,7 +339,7 @@ TEST(ultra_circuit_constructor, sort_with_edges_gate) } { - UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder(); + UltraCircuitBuilder circuit_constructor; auto a_idx = circuit_constructor.add_variable(a); auto b_idx = circuit_constructor.add_variable(b); auto c_idx = circuit_constructor.add_variable(c); @@ -355,7 +355,7 @@ TEST(ultra_circuit_constructor, sort_with_edges_gate) EXPECT_EQ(result, false); } { - UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder(); + UltraCircuitBuilder circuit_constructor; auto a_idx = circuit_constructor.add_variable(a); auto b_idx = circuit_constructor.add_variable(b); auto c_idx = circuit_constructor.add_variable(c); @@ -371,7 +371,7 @@ TEST(ultra_circuit_constructor, sort_with_edges_gate) EXPECT_EQ(result, false); } { - UltraCircuitBuilder circuit_constructor = UltraCircuitBuilder(); + UltraCircuitBuilder circuit_constructor; auto a_idx = circuit_constructor.add_variable(a); auto c_idx = circuit_constructor.add_variable(c); auto d_idx = circuit_constructor.add_variable(d); diff --git a/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_checker.cpp b/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_checker.cpp index e213801cb229..55e7d3f44a17 100644 --- a/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_checker.cpp +++ b/barretenberg/cpp/src/barretenberg/circuit_checker/ultra_circuit_checker.cpp @@ -40,7 +40,7 @@ template bool UltraCircuitChecker::check(const Builder& build size_t block_idx = 0; for (auto& block : builder.blocks.get()) { result = result && check_block(builder, block, tag_data, memory_data, lookup_hash_table); - if (result == false) { + if (!result) { info("Failed at block idx = ", block_idx); return false; } @@ -49,7 +49,7 @@ template bool UltraCircuitChecker::check(const Builder& build // Tag check is only expected to pass after entire execution trace (all blocks) have been processed result = result && check_tag_data(tag_data); - if (result == false) { + if (!result) { info("Failed tag check."); return false; } @@ -71,6 +71,14 @@ bool UltraCircuitChecker::check_block(Builder& builder, params.eta_two = memory_data.eta_two; params.eta_three = memory_data.eta_three; + auto report_fail = [&](const char* message, size_t row_idx) { + info(message, row_idx); +#ifdef CHECK_CIRCUIT_STACKTRACES + block.stack_traces.print(row_idx); +#endif + return false; + }; + // Perform checks on each gate defined in the builder bool result = true; for (size_t idx = 0; idx < block.size(); ++idx) { @@ -78,50 +86,41 @@ bool UltraCircuitChecker::check_block(Builder& builder, populate_values(builder, block, values, tag_data, memory_data, idx); result = result && check_relation(values, params); - if (result == false) { - info("Failed Arithmetic relation at row idx = ", idx); - return false; + if (!result) { + return report_fail("Failed Arithmetic relation at row idx = ", idx); } result = result && check_relation(values, params); - if (result == false) { - info("Failed Elliptic relation at row idx = ", idx); - return false; + if (!result) { + return report_fail("Failed Elliptic relation at row idx = ", idx); } result = result && check_relation(values, params); - if (result == false) { - info("Failed Auxiliary relation at row idx = ", idx); - return false; + if (!result) { + return report_fail("Failed Auxiliary relation at row idx = ", idx); } result = result && check_relation(values, params); - if (result == false) { - info("Failed DeltaRangeConstraint relation at row idx = ", idx); - return false; + if (!result) { + return report_fail("Failed DeltaRangeConstraint relation at row idx = ", idx); } result = result && check_lookup(values, lookup_hash_table); - if (result == false) { - info("Failed Lookup check relation at row idx = ", idx); - return false; + if (!result) { + return report_fail("Failed Lookup check relation at row idx = ", idx); } if constexpr (IsMegaBuilder) { result = result && check_relation(values, params); - if (result == false) { - info("Failed PoseidonInternal relation at row idx = ", idx); - return false; + if (!result) { + return report_fail("Failed PoseidonInternal relation at row idx = ", idx); } result = result && check_relation(values, params); - if (result == false) { - info("Failed PoseidonExternal relation at row idx = ", idx); - return false; + if (!result) { + return report_fail("Failed PoseidonExternal relation at row idx = ", idx); } result = result && check_databus_read(values, builder); - if (result == false) { - info("Failed databus read at row idx = ", idx); - return false; + if (!result) { + return report_fail("Failed databus read at row idx = ", idx); } } - if (result == false) { - info("Failed at row idx = ", idx); - return false; + if (!result) { + return report_fail("Failed at row idx = ", idx); } } @@ -223,8 +222,8 @@ void UltraCircuitChecker::populate_values( values.w_l = builder.get_variable(block.w_l()[idx]); values.w_r = builder.get_variable(block.w_r()[idx]); values.w_o = builder.get_variable(block.w_o()[idx]); - // Note: memory_data contains indices into the block to which RAM/ROM gates were added so we need to check that we - // are indexing into the correct block before updating the w_4 value. + // Note: memory_data contains indices into the block to which RAM/ROM gates were added so we need to check that + // we are indexing into the correct block before updating the w_4 value. if (block.has_ram_rom && memory_data.read_record_gates.contains(idx)) { values.w_4 = compute_memory_record_term( values.w_l, values.w_r, values.w_o, memory_data.eta, memory_data.eta_two, memory_data.eta_three); diff --git a/barretenberg/cpp/src/barretenberg/eccvm/eccvm_prover.cpp b/barretenberg/cpp/src/barretenberg/eccvm/eccvm_prover.cpp index 0633561940bc..43cd7248f110 100644 --- a/barretenberg/cpp/src/barretenberg/eccvm/eccvm_prover.cpp +++ b/barretenberg/cpp/src/barretenberg/eccvm/eccvm_prover.cpp @@ -33,7 +33,6 @@ ECCVMProver::ECCVMProver(CircuitBuilder& builder, const std::shared_ptr(key->circuit_size); - info("circuit_size"); transcript->send_to_verifier("circuit_size", circuit_size); } diff --git a/barretenberg/cpp/src/barretenberg/plonk_honk_shared/arithmetization/arithmetization.hpp b/barretenberg/cpp/src/barretenberg/plonk_honk_shared/arithmetization/arithmetization.hpp index fa1808adf9c0..4d3514f20cfc 100644 --- a/barretenberg/cpp/src/barretenberg/plonk_honk_shared/arithmetization/arithmetization.hpp +++ b/barretenberg/cpp/src/barretenberg/plonk_honk_shared/arithmetization/arithmetization.hpp @@ -6,6 +6,9 @@ #include #include #include +#ifdef CHECK_CIRCUIT_STACKTRACES +#include +#endif namespace bb { @@ -32,6 +35,23 @@ namespace bb { * We should only do this if it becomes necessary or convenient. */ +#ifdef CHECK_CIRCUIT_STACKTRACES +struct BbStackTrace : backward::StackTrace { + BbStackTrace() { load_here(32); } +}; +struct StackTraces { + std::vector stack_traces; + void populate() { stack_traces.emplace_back(); } + void print(size_t gate_idx) const { backward::Printer{}.print(stack_traces.at(gate_idx)); } + // Don't interfere with equality semantics of structs that include this in debug builds + bool operator==(const StackTraces& other) const + { + static_cast(other); + return true; + } +}; +#endif + /** * @brief Basic structure for storing gate data in a builder * @@ -46,6 +66,11 @@ template class ExecutionTr using Selectors = std::array; using Wires = std::array; +#ifdef CHECK_CIRCUIT_STACKTRACES + // If enabled, we keep slow stack traces to be able to correlate gates with code locations where they were added + StackTraces stack_traces; +#endif + Wires wires; // vectors of indices into a witness variables array Selectors selectors; bool has_ram_rom = false; // does the block contain RAM/ROM gates @@ -65,6 +90,9 @@ template class ExecutionTr for (auto& p : selectors) { p.reserve(size_hint); } +#ifdef CHECK_CIRCUIT_STACKTRACES + stack_traces.stack_traces.reserve(size_hint); +#endif } uint32_t get_fixed_size() const { return fixed_size; } @@ -83,6 +111,9 @@ template class StandardArith { public: void populate_wires(const uint32_t& idx_1, const uint32_t& idx_2, const uint32_t& idx_3) { +#ifdef CHECK_CIRCUIT_STACKTRACES + this->stack_traces.populate(); +#endif this->wires[0].emplace_back(idx_1); this->wires[1].emplace_back(idx_2); this->wires[2].emplace_back(idx_3); @@ -130,6 +161,9 @@ template class UltraArith { public: void populate_wires(const uint32_t& idx_1, const uint32_t& idx_2, const uint32_t& idx_3, const uint32_t& idx_4) { +#ifdef CHECK_CIRCUIT_STACKTRACES + this->stack_traces.populate(); +#endif this->wires[0].emplace_back(idx_1); this->wires[1].emplace_back(idx_2); this->wires[2].emplace_back(idx_3); @@ -246,6 +280,9 @@ template class UltraHonkArith { public: void populate_wires(const uint32_t& idx_1, const uint32_t& idx_2, const uint32_t& idx_3, const uint32_t& idx_4) { +#ifdef CHECK_CIRCUIT_STACKTRACES + this->stack_traces.populate(); +#endif this->wires[0].emplace_back(idx_1); this->wires[1].emplace_back(idx_2); this->wires[2].emplace_back(idx_3); diff --git a/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/standard_circuit_builder.cpp b/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/standard_circuit_builder.cpp index 351c3ed0eb47..ad0938731d54 100644 --- a/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/standard_circuit_builder.cpp +++ b/barretenberg/cpp/src/barretenberg/stdlib_circuit_builders/standard_circuit_builder.cpp @@ -279,7 +279,6 @@ std::vector StandardCircuitBuilder_::decompose_into_base4_accumula accumulator_idx = new_accumulator_idx; } } - this->assert_equal(witness_index, accumulator_idx, msg); return accumulators; }