From 4343bb16ea186ec10d264ae065098e7f7684fcc1 Mon Sep 17 00:00:00 2001 From: jeanmon Date: Thu, 15 Feb 2024 08:33:55 +0000 Subject: [PATCH 1/3] 4598 - remove superfluous trace builder constructor --- .../cpp/src/barretenberg/vm/tests/avm_arithmetic.test.cpp | 7 +------ .../cpp/src/barretenberg/vm/tests/avm_bitwise.test.cpp | 6 +----- .../src/barretenberg/vm/tests/avm_control_flow.test.cpp | 6 +----- .../cpp/src/barretenberg/vm/tests/avm_execution.test.cpp | 6 +----- .../cpp/src/barretenberg/vm/tests/avm_memory.test.cpp | 6 +----- 5 files changed, 5 insertions(+), 26 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/vm/tests/avm_arithmetic.test.cpp b/barretenberg/cpp/src/barretenberg/vm/tests/avm_arithmetic.test.cpp index e9335512e5d..f2e1e272aca 100644 --- a/barretenberg/cpp/src/barretenberg/vm/tests/avm_arithmetic.test.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/tests/avm_arithmetic.test.cpp @@ -251,11 +251,7 @@ class AvmArithmeticTests : public ::testing::Test { protected: // TODO(640): The Standard Honk on Grumpkin test suite fails unless the SRS is initialised for every test. - void SetUp() override - { - srs::init_crs_factory("../srs_db/ignition"); - trace_builder = AvmTraceBuilder(); // Clean instance for every run. - }; + void SetUp() override { srs::init_crs_factory("../srs_db/ignition"); }; }; class AvmArithmeticTestsFF : public AvmArithmeticTests {}; @@ -303,7 +299,6 @@ class AvmArithmeticNegativeTestsU128 : public AvmArithmeticTests {}; // Test on basic addition over finite field type. TEST_F(AvmArithmeticTestsFF, addition) { - // trace_builder trace_builder.calldata_copy(0, 3, 0, std::vector{ 37, 4, 11 }); // Memory layout: [37,4,11,0,0,0,....] diff --git a/barretenberg/cpp/src/barretenberg/vm/tests/avm_bitwise.test.cpp b/barretenberg/cpp/src/barretenberg/vm/tests/avm_bitwise.test.cpp index 3abc6645219..b657e199322 100644 --- a/barretenberg/cpp/src/barretenberg/vm/tests/avm_bitwise.test.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/tests/avm_bitwise.test.cpp @@ -81,11 +81,7 @@ class AvmBitwiseTests : public ::testing::Test { protected: // TODO(640): The Standard Honk on Grumpkin test suite fails unless the SRS is initialised for every test. - void SetUp() override - { - srs::init_crs_factory("../srs_db/ignition"); - trace_builder = AvmTraceBuilder(); // Clean instance for every run. - }; + void SetUp() override { srs::init_crs_factory("../srs_db/ignition"); }; }; class AvmBitwiseTestsU8 : public AvmBitwiseTests {}; diff --git a/barretenberg/cpp/src/barretenberg/vm/tests/avm_control_flow.test.cpp b/barretenberg/cpp/src/barretenberg/vm/tests/avm_control_flow.test.cpp index 99a47bc1332..73d175e3232 100644 --- a/barretenberg/cpp/src/barretenberg/vm/tests/avm_control_flow.test.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/tests/avm_control_flow.test.cpp @@ -11,11 +11,7 @@ class AvmControlFlowTests : public ::testing::Test { protected: // TODO(640): The Standard Honk on Grumpkin test suite fails unless the SRS is initialised for every test. - void SetUp() override - { - srs::init_crs_factory("../srs_db/ignition"); - trace_builder = AvmTraceBuilder(); // Clean instance for every run. - }; + void SetUp() override { srs::init_crs_factory("../srs_db/ignition"); }; }; /****************************************************************************** diff --git a/barretenberg/cpp/src/barretenberg/vm/tests/avm_execution.test.cpp b/barretenberg/cpp/src/barretenberg/vm/tests/avm_execution.test.cpp index eb17c7b69ac..2760088ed83 100644 --- a/barretenberg/cpp/src/barretenberg/vm/tests/avm_execution.test.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/tests/avm_execution.test.cpp @@ -45,11 +45,7 @@ class AvmExecutionTests : public ::testing::Test { protected: // TODO(640): The Standard Honk on Grumpkin test suite fails unless the SRS is initialised for every test. - void SetUp() override - { - srs::init_crs_factory("../srs_db/ignition"); - trace_builder = AvmTraceBuilder(); // Clean instance for every run. - }; + void SetUp() override { srs::init_crs_factory("../srs_db/ignition"); }; }; // Basic positive test with an ADD and RETURN opcode. diff --git a/barretenberg/cpp/src/barretenberg/vm/tests/avm_memory.test.cpp b/barretenberg/cpp/src/barretenberg/vm/tests/avm_memory.test.cpp index 5c357d1bad7..a14b1478d0a 100644 --- a/barretenberg/cpp/src/barretenberg/vm/tests/avm_memory.test.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/tests/avm_memory.test.cpp @@ -11,11 +11,7 @@ class AvmMemoryTests : public ::testing::Test { protected: // TODO(640): The Standard Honk on Grumpkin test suite fails unless the SRS is initialised for every test. - void SetUp() override - { - srs::init_crs_factory("../srs_db/ignition"); - trace_builder = AvmTraceBuilder(); // Clean instance for every run. - }; + void SetUp() override { srs::init_crs_factory("../srs_db/ignition"); }; }; /****************************************************************************** From 28b1f784c3bfad1eebfcf39a30a71de45bf0b26e Mon Sep 17 00:00:00 2001 From: jeanmon Date: Thu, 15 Feb 2024 10:19:58 +0000 Subject: [PATCH 2/3] 4598 - propagate tag err to the main trace for return_op and internal_return --- .../barretenberg/vm/avm_trace/avm_trace.cpp | 23 +++++++++++++++---- .../vm/tests/avm_arithmetic.test.cpp | 4 ++++ 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/vm/avm_trace/avm_trace.cpp b/barretenberg/cpp/src/barretenberg/vm/avm_trace/avm_trace.cpp index 80d92bc98f8..fc3458b8996 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm_trace/avm_trace.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm_trace/avm_trace.cpp @@ -404,6 +404,8 @@ std::vector AvmTraceBuilder::return_op(uint32_t ret_offset, uint32_t ret_siz // Reading and loading to Ia auto read_a = mem_trace_builder.read_and_load_from_memory(clk, IntermRegister::IA, mem_idx_a, AvmMemoryTag::FF); + bool tag_match = read_a.tag_match; + FF ia = read_a.val; returnMem.push_back(ia); @@ -414,6 +416,7 @@ std::vector AvmTraceBuilder::return_op(uint32_t ret_offset, uint32_t ret_siz // Reading and loading to Ib auto read_b = mem_trace_builder.read_and_load_from_memory(clk, IntermRegister::IB, mem_idx_b, AvmMemoryTag::FF); + tag_match = tag_match && read_b.tag_match; FF ib = read_b.val; returnMem.push_back(ib); } @@ -425,6 +428,7 @@ std::vector AvmTraceBuilder::return_op(uint32_t ret_offset, uint32_t ret_siz // Reading and loading to Ic auto read_c = mem_trace_builder.read_and_load_from_memory(clk, IntermRegister::IC, mem_idx_c, AvmMemoryTag::FF); + tag_match = tag_match && read_c.tag_match; FF ic = read_c.val; returnMem.push_back(ic); } @@ -435,9 +439,10 @@ std::vector AvmTraceBuilder::return_op(uint32_t ret_offset, uint32_t ret_siz .avm_main_internal_return_ptr = FF(internal_return_ptr), .avm_main_sel_halt = FF(1), .avm_main_in_tag = FF(static_cast(AvmMemoryTag::FF)), - .avm_main_ia = ia, - .avm_main_ib = ib, - .avm_main_ic = ic, + .avm_main_tag_err = FF(static_cast(!tag_match)), + .avm_main_ia = tag_match ? ia : FF(0), + .avm_main_ib = tag_match ? ib : FF(0), + .avm_main_ic = tag_match ? ic : FF(0), .avm_main_mem_op_a = FF(mem_op_a), .avm_main_mem_op_b = FF(mem_op_b), .avm_main_mem_op_c = FF(mem_op_c), @@ -565,7 +570,8 @@ void AvmTraceBuilder::internal_return() .avm_main_pc = pc, .avm_main_internal_return_ptr = FF(internal_return_ptr), .avm_main_sel_internal_return = FF(1), - .avm_main_ia = read_a.val, + .avm_main_tag_err = FF(static_cast(!read_a.tag_match)), + .avm_main_ia = read_a.tag_match ? read_a.val : FF(0), .avm_main_mem_op_a = FF(1), .avm_main_rwa = FF(0), .avm_main_mem_idx_a = FF(internal_return_ptr - 1), @@ -718,6 +724,10 @@ void AvmTraceBuilder::op_not(uint32_t a_offset, uint32_t dst_offset, AvmMemoryTa // ~a = c FF a = read_a.tag_match ? read_a.val : FF(0); + // TODO(4613): If tag_match == false, then the value of c + // will not be zero which would not satisfy the constraint that + // ic == 0 whenever tag_err == 1. This constraint might be removed + // as part of #4613. FF c = alu_trace_builder.op_not(a, in_tag, clk); // Write into memory value c from intermediate register ic. @@ -760,6 +770,11 @@ void AvmTraceBuilder::op_eq(uint32_t a_offset, uint32_t b_offset, uint32_t dst_o // c = a == b ? 1 : 0 FF a = tag_match ? read_a.val : FF(0); FF b = tag_match ? read_b.val : FF(0); + + // TODO(4613): If tag_match == false, then the value of c + // will not be zero which would not satisfy the constraint that + // ic == 0 whenever tag_err == 1. This constraint might be removed + // as part of #4613. FF c = alu_trace_builder.op_eq(a, b, in_tag, clk); // Write into memory value c from intermediate register ic. diff --git a/barretenberg/cpp/src/barretenberg/vm/tests/avm_arithmetic.test.cpp b/barretenberg/cpp/src/barretenberg/vm/tests/avm_arithmetic.test.cpp index f2e1e272aca..2e749b41536 100644 --- a/barretenberg/cpp/src/barretenberg/vm/tests/avm_arithmetic.test.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/tests/avm_arithmetic.test.cpp @@ -41,6 +41,10 @@ void common_validate_arithmetic_op(Row const& main_row, EXPECT_EQ(alu_row.avm_alu_alu_ia, a); EXPECT_EQ(alu_row.avm_alu_alu_ib, b); EXPECT_EQ(alu_row.avm_alu_alu_ic, c); + + // Check that no error is raised + EXPECT_EQ(main_row.avm_main_tag_err, FF(0)); + EXPECT_EQ(main_row.avm_main_op_err, FF(0)); } Row common_validate_add(std::vector const& trace, From cb08f8c67be16cd2c1ba46cd1a8f1e2e63ae13b4 Mon Sep 17 00:00:00 2001 From: jeanmon Date: Thu, 15 Feb 2024 10:24:26 +0000 Subject: [PATCH 3/3] 4598 - refactor: move op_not and op_eq routines --- .../barretenberg/vm/avm_trace/avm_trace.cpp | 183 +++++++++--------- 1 file changed, 92 insertions(+), 91 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/vm/avm_trace/avm_trace.cpp b/barretenberg/cpp/src/barretenberg/vm/avm_trace/avm_trace.cpp index fc3458b8996..35639ae7276 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm_trace/avm_trace.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm_trace/avm_trace.cpp @@ -227,6 +227,98 @@ void AvmTraceBuilder::op_div(uint32_t a_offset, uint32_t b_offset, uint32_t dst_ }); } +/** + * @brief Bitwise not with direct memory access. + * + * @param a_offset An index in memory pointing to the only operand of Not. + * @param dst_offset An index in memory pointing to the output of Not. + * @param in_tag The instruction memory tag of the operands. + */ +void AvmTraceBuilder::op_not(uint32_t a_offset, uint32_t dst_offset, AvmMemoryTag in_tag) +{ + auto clk = static_cast(main_trace.size()); + + // Reading from memory and loading into ia. + auto read_a = mem_trace_builder.read_and_load_from_memory(clk, IntermRegister::IA, a_offset, in_tag); + + // ~a = c + FF a = read_a.tag_match ? read_a.val : FF(0); + // TODO(4613): If tag_match == false, then the value of c + // will not be zero which would not satisfy the constraint that + // ic == 0 whenever tag_err == 1. This constraint might be removed + // as part of #4613. + FF c = alu_trace_builder.op_not(a, in_tag, clk); + + // Write into memory value c from intermediate register ic. + mem_trace_builder.write_into_memory(clk, IntermRegister::IC, dst_offset, c, in_tag); + + main_trace.push_back(Row{ + .avm_main_clk = clk, + .avm_main_pc = FF(pc++), + .avm_main_internal_return_ptr = FF(internal_return_ptr), + .avm_main_sel_op_not = FF(1), + .avm_main_in_tag = FF(static_cast(in_tag)), + .avm_main_tag_err = FF(static_cast(!read_a.tag_match)), + .avm_main_ia = a, + .avm_main_ic = c, + .avm_main_mem_op_a = FF(1), + .avm_main_mem_op_c = FF(1), + .avm_main_rwc = FF(1), + .avm_main_mem_idx_a = FF(a_offset), + .avm_main_mem_idx_c = FF(dst_offset), + }); +}; + +/** + * @brief Equality with direct memory access. + * + * @param a_offset An index in memory pointing to the first operand of the equality. + * @param b_offset An index in memory pointing to the second operand of the equality. + * @param dst_offset An index in memory pointing to the output of the equality. + * @param in_tag The instruction memory tag of the operands. + */ +void AvmTraceBuilder::op_eq(uint32_t a_offset, uint32_t b_offset, uint32_t dst_offset, AvmMemoryTag in_tag) +{ + auto clk = static_cast(main_trace.size()); + + // Reading from memory and loading into ia resp. ib. + auto read_a = mem_trace_builder.read_and_load_from_memory(clk, IntermRegister::IA, a_offset, in_tag); + auto read_b = mem_trace_builder.read_and_load_from_memory(clk, IntermRegister::IB, b_offset, in_tag); + bool tag_match = read_a.tag_match && read_b.tag_match; + + // c = a == b ? 1 : 0 + FF a = tag_match ? read_a.val : FF(0); + FF b = tag_match ? read_b.val : FF(0); + + // TODO(4613): If tag_match == false, then the value of c + // will not be zero which would not satisfy the constraint that + // ic == 0 whenever tag_err == 1. This constraint might be removed + // as part of #4613. + FF c = alu_trace_builder.op_eq(a, b, in_tag, clk); + + // Write into memory value c from intermediate register ic. + mem_trace_builder.write_into_memory(clk, IntermRegister::IC, dst_offset, c, in_tag); + + main_trace.push_back(Row{ + .avm_main_clk = clk, + .avm_main_pc = FF(pc++), + .avm_main_internal_return_ptr = FF(internal_return_ptr), + .avm_main_sel_op_eq = FF(1), + .avm_main_in_tag = FF(static_cast(in_tag)), + .avm_main_tag_err = FF(static_cast(!tag_match)), + .avm_main_ia = a, + .avm_main_ib = b, + .avm_main_ic = c, + .avm_main_mem_op_a = FF(1), + .avm_main_mem_op_b = FF(1), + .avm_main_mem_op_c = FF(1), + .avm_main_rwc = FF(1), + .avm_main_mem_idx_a = FF(a_offset), + .avm_main_mem_idx_b = FF(b_offset), + .avm_main_mem_idx_c = FF(dst_offset), + }); +} + // TODO: Finish SET opcode implementation. This is a partial implementation // facilitating testing of arithmetic operations over non finite field types. // We add an entry in the memory trace and a simplified one in the main trace @@ -708,95 +800,4 @@ std::vector AvmTraceBuilder::finalize() return trace; } -/** - * @brief Bitwise not with direct memory access. - * - * @param a_offset An index in memory pointing to the only operand of Not. - * @param dst_offset An index in memory pointing to the output of Not. - * @param in_tag The instruction memory tag of the operands. - */ -void AvmTraceBuilder::op_not(uint32_t a_offset, uint32_t dst_offset, AvmMemoryTag in_tag) -{ - auto clk = static_cast(main_trace.size()); - - // Reading from memory and loading into ia. - auto read_a = mem_trace_builder.read_and_load_from_memory(clk, IntermRegister::IA, a_offset, in_tag); - - // ~a = c - FF a = read_a.tag_match ? read_a.val : FF(0); - // TODO(4613): If tag_match == false, then the value of c - // will not be zero which would not satisfy the constraint that - // ic == 0 whenever tag_err == 1. This constraint might be removed - // as part of #4613. - FF c = alu_trace_builder.op_not(a, in_tag, clk); - - // Write into memory value c from intermediate register ic. - mem_trace_builder.write_into_memory(clk, IntermRegister::IC, dst_offset, c, in_tag); - - main_trace.push_back(Row{ - .avm_main_clk = clk, - .avm_main_pc = FF(pc++), - .avm_main_internal_return_ptr = FF(internal_return_ptr), - .avm_main_sel_op_not = FF(1), - .avm_main_in_tag = FF(static_cast(in_tag)), - .avm_main_tag_err = FF(static_cast(!read_a.tag_match)), - .avm_main_ia = a, - .avm_main_ic = c, - .avm_main_mem_op_a = FF(1), - .avm_main_mem_op_c = FF(1), - .avm_main_rwc = FF(1), - .avm_main_mem_idx_a = FF(a_offset), - .avm_main_mem_idx_c = FF(dst_offset), - }); -}; - -/** - * @brief Equality with direct memory access. - * - * @param a_offset An index in memory pointing to the first operand of the equality. - * @param b_offset An index in memory pointing to the second operand of the equality. - * @param dst_offset An index in memory pointing to the output of the equality. - * @param in_tag The instruction memory tag of the operands. - */ -void AvmTraceBuilder::op_eq(uint32_t a_offset, uint32_t b_offset, uint32_t dst_offset, AvmMemoryTag in_tag) -{ - auto clk = static_cast(main_trace.size()); - - // Reading from memory and loading into ia resp. ib. - auto read_a = mem_trace_builder.read_and_load_from_memory(clk, IntermRegister::IA, a_offset, in_tag); - auto read_b = mem_trace_builder.read_and_load_from_memory(clk, IntermRegister::IB, b_offset, in_tag); - bool tag_match = read_a.tag_match && read_b.tag_match; - - // c = a == b ? 1 : 0 - FF a = tag_match ? read_a.val : FF(0); - FF b = tag_match ? read_b.val : FF(0); - - // TODO(4613): If tag_match == false, then the value of c - // will not be zero which would not satisfy the constraint that - // ic == 0 whenever tag_err == 1. This constraint might be removed - // as part of #4613. - FF c = alu_trace_builder.op_eq(a, b, in_tag, clk); - - // Write into memory value c from intermediate register ic. - mem_trace_builder.write_into_memory(clk, IntermRegister::IC, dst_offset, c, in_tag); - - main_trace.push_back(Row{ - .avm_main_clk = clk, - .avm_main_pc = FF(pc++), - .avm_main_internal_return_ptr = FF(internal_return_ptr), - .avm_main_sel_op_eq = FF(1), - .avm_main_in_tag = FF(static_cast(in_tag)), - .avm_main_tag_err = FF(static_cast(!tag_match)), - .avm_main_ia = a, - .avm_main_ib = b, - .avm_main_ic = c, - .avm_main_mem_op_a = FF(1), - .avm_main_mem_op_b = FF(1), - .avm_main_mem_op_c = FF(1), - .avm_main_rwc = FF(1), - .avm_main_mem_idx_a = FF(a_offset), - .avm_main_mem_idx_b = FF(b_offset), - .avm_main_mem_idx_c = FF(dst_offset), - }); -} } // namespace avm_trace