From 25394601da863f7afcb52633a03278c4778b255f Mon Sep 17 00:00:00 2001 From: jeanmon Date: Fri, 2 Feb 2024 13:48:53 +0000 Subject: [PATCH 1/5] 4304 - replace some addition with bitwise or for efficiency --- .../cpp/src/barretenberg/vm/avm_trace/AvmMini_execution.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_execution.cpp b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_execution.cpp index 53019fb77ce9..8b0afbe16479 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_execution.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_execution.cpp @@ -212,12 +212,12 @@ std::vector Execution::gen_trace(std::vector const& instructio case AvmMemoryTag::U64: // value represented as 2 uint32_t operands val = inst.operands.at(0); val <<= 32; - val += inst.operands.at(1); + val |= inst.operands.at(1); dst_offset = inst.operands.at(2); break; case AvmMemoryTag::U128: // value represented as 4 uint32_t operands for (size_t i = 0; i < 4; i++) { - val += inst.operands.at(i); + val |= inst.operands.at(i); val <<= 32; } dst_offset = inst.operands.at(4); From 483e2d293009fd11a00bad7d33abc201133356a3 Mon Sep 17 00:00:00 2001 From: jeanmon Date: Mon, 5 Feb 2024 14:59:40 +0000 Subject: [PATCH 2/5] 4304 - implement more generic deserialization function for bytecode --- .../vm/avm_trace/AvmMini_deserialization.cpp | 176 ++++++++++++++++ .../vm/avm_trace/AvmMini_deserialization.hpp | 31 +++ .../vm/avm_trace/AvmMini_execution.cpp | 191 +++++------------- .../vm/avm_trace/AvmMini_execution.hpp | 7 - .../vm/avm_trace/AvmMini_instructions.hpp | 11 +- .../vm/tests/AvmMini_execution.test.cpp | 132 +++++++----- .../instruction_serialization.ts | 5 +- 7 files changed, 348 insertions(+), 205 deletions(-) create mode 100644 barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_deserialization.cpp create mode 100644 barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_deserialization.hpp diff --git a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_deserialization.cpp b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_deserialization.cpp new file mode 100644 index 000000000000..17ece6955499 --- /dev/null +++ b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_deserialization.cpp @@ -0,0 +1,176 @@ +#include "AvmMini_deserialization.hpp" +#include "barretenberg/vm/avm_trace/AvmMini_common.hpp" +#include "barretenberg/vm/avm_trace/AvmMini_instructions.hpp" +#include "barretenberg/vm/avm_trace/AvmMini_opcode.hpp" +#include +#include +#include +#include + +namespace { +using namespace avm_trace; + +const std::vector three_operand_format = { + OperandType::TAG, + OperandType::UINT32, + OperandType::UINT32, + OperandType::UINT32, +}; + +} // Anonymous namespace + +namespace avm_trace { + +// Contrary to TS, the format does not contain the opcode byte which prefixes any instruction. +// The format for OpCode::SET has to be handled separately as it is variable based on the tag. +const std::unordered_map> Deserialization::OPCODE_WIRE_FORMAT = { + // Compute + // Compute - Arithmetic + { OpCode::ADD, three_operand_format }, + { OpCode::SUB, three_operand_format }, + { OpCode::MUL, three_operand_format }, + { OpCode::DIV, three_operand_format }, + // Execution Environment - Calldata + { OpCode::CALLDATACOPY, { OperandType::UINT32, OperandType::UINT32, OperandType::UINT32 } }, + // Machine State - Internal Control Flow + { OpCode::JUMP, { OperandType::UINT32 } }, + { OpCode::INTERNALCALL, { OperandType::UINT32 } }, + { OpCode::INTERNALRETURN, {} }, + // Machine State - Memory + // OpCode::SET is handled differently + // Control Flow - Contract Calls + { OpCode::RETURN, { OperandType::UINT32, OperandType::UINT32 } }, +}; + +const std::unordered_map Deserialization::OPERAND_TYPE_SIZE = { + { OperandType::TAG, 1 }, { OperandType::UINT8, 1 }, { OperandType::UINT16, 2 }, + { OperandType::UINT32, 4 }, { OperandType::UINT64, 8 }, { OperandType::UINT128, 16 }, +}; + +/** + * @brief Parsing of the supplied bytecode into a vector of instructions. It essentially + * checks that each opcode value is in the defined range and extracts the operands + * for each opcode based on the specification from OPCODE_WIRE_FORMAT. + * + * @param bytecode The bytecode to be parsed as a vector of bytes/uint8_t + * @throws runtime_error exception when the bytecode is invalid. + * @return Vector of instructions + */ +std::vector Deserialization::parse(std::vector const& bytecode) +{ + std::vector instructions; + size_t pos = 0; + const auto length = bytecode.size(); + + while (pos < length) { + const uint8_t opcode_byte = bytecode.at(pos); + + if (!Bytecode::is_valid(opcode_byte)) { + throw_or_abort("Invalid opcode byte: " + std::to_string(opcode_byte) + + " at position: " + std::to_string(pos)); + } + pos++; + + auto const opcode = static_cast(opcode_byte); + std::vector inst_format; + + if (opcode == OpCode::SET) { + if (pos == length) { + throw_or_abort("Operand for SET opcode is missing at position " + std::to_string(pos)); + } + uint8_t set_tag_u8 = bytecode.at(pos); + if (!std::set{ static_cast(AvmMemoryTag::U8), + static_cast(AvmMemoryTag::U16), + static_cast(AvmMemoryTag::U32), + static_cast(AvmMemoryTag::U64), + static_cast(AvmMemoryTag::U128) } + .contains(set_tag_u8)) { + throw_or_abort("Instruction tag for SET opcode is invalid at position " + std::to_string(pos) + + " value: " + std::to_string(set_tag_u8)); + } + + auto in_tag = static_cast(set_tag_u8); + switch (in_tag) { + case AvmMemoryTag::U8: + inst_format = { OperandType::TAG, OperandType::UINT8, OperandType::UINT32 }; + break; + case AvmMemoryTag::U16: + inst_format = { OperandType::TAG, OperandType::UINT16, OperandType::UINT32 }; + break; + case AvmMemoryTag::U32: + inst_format = { OperandType::TAG, OperandType::UINT32, OperandType::UINT32 }; + break; + case AvmMemoryTag::U64: + inst_format = { OperandType::TAG, OperandType::UINT64, OperandType::UINT32 }; + break; + case AvmMemoryTag::U128: + inst_format = { OperandType::TAG, OperandType::UINT128, OperandType::UINT32 }; + break; + default: // This branch is guarded above. + inst_format = {}; + } + } else { + inst_format = OPCODE_WIRE_FORMAT.at(opcode); + } + + std::vector operands{}; + + for (auto const& opType : inst_format) { + // No underflow as while condition guarantees pos <= length (after pos++) + if (length - pos < OPERAND_TYPE_SIZE.at(opType)) { + throw_or_abort("Operand is missing at position " + std::to_string(pos)); + } + Operand operand; + uint8_t const* pos_ptr = &bytecode.at(pos); + + switch (opType) { + case OperandType::TAG: { + uint8_t tag_u8 = bytecode.at(pos); + if (tag_u8 == static_cast(AvmMemoryTag::U0) || tag_u8 > MAX_MEM_TAG) { + throw_or_abort("Instruction tag is invalid at position " + std::to_string(pos) + + " value: " + std::to_string(tag_u8)); + } + operand = static_cast(tag_u8); + pos++; + break; + } + case OperandType::UINT8: + operand = bytecode.at(pos); + pos++; + break; + case OperandType::UINT16: { + uint16_t operand_u16{}; + serialize::read(pos_ptr, operand_u16); + operand = operand_u16; + pos += 2; + break; + } + case OperandType::UINT32: { + uint32_t operand_u32{}; + serialize::read(pos_ptr, operand_u32); + operand = operand_u32; + pos += 4; + break; + } + case OperandType::UINT64: { + uint64_t operand_u64{}; + serialize::read(pos_ptr, operand_u64); + operand = operand_u64; + pos += 8; + break; + } + case OperandType::UINT128: { + uint128_t operand_u128{}; + serialize::read(pos_ptr, operand_u128); + operand = operand_u128; + pos += 16; + break; + } + } + operands.push_back(operand); + } + instructions.emplace_back(opcode, operands); + } + return instructions; +}; +} // namespace avm_trace \ No newline at end of file diff --git a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_deserialization.hpp b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_deserialization.hpp new file mode 100644 index 000000000000..93493b8564c0 --- /dev/null +++ b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_deserialization.hpp @@ -0,0 +1,31 @@ +#pragma once + +#include "barretenberg/numeric/uint128/uint128.hpp" +#include "barretenberg/vm/avm_trace/AvmMini_common.hpp" +#include "barretenberg/vm/avm_trace/AvmMini_instructions.hpp" +#include "barretenberg/vm/avm_trace/AvmMini_opcode.hpp" +#include +#include +#include +#include +#include + +namespace avm_trace { + +// Possible types for an instruction's operand in its wire format. (Keep in sync with TS code. +// See avm/serialization/instruction_serialization.ts). +// Note that the TAG enum value is not supported in TS and is parsed as UINT8. +enum class OperandType : uint8_t { TAG, UINT8, UINT16, UINT32, UINT64, UINT128 }; + +class Deserialization { + public: + Deserialization() = default; + + static std::vector parse(std::vector const& bytecode); + + private: + static const std::unordered_map> OPCODE_WIRE_FORMAT; + static const std::unordered_map OPERAND_TYPE_SIZE; +}; + +} // namespace avm_trace \ No newline at end of file diff --git a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_execution.cpp b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_execution.cpp index 8b0afbe16479..79f0fc0d4434 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_execution.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_execution.cpp @@ -1,8 +1,8 @@ #include "AvmMini_execution.hpp" #include "barretenberg/common/serialize.hpp" -#include "barretenberg/common/throw_or_abort.hpp" #include "barretenberg/proof_system/circuit_builder/generated/AvmMini_circuit_builder.hpp" #include "barretenberg/vm/avm_trace/AvmMini_common.hpp" +#include "barretenberg/vm/avm_trace/AvmMini_deserialization.hpp" #include "barretenberg/vm/avm_trace/AvmMini_instructions.hpp" #include "barretenberg/vm/avm_trace/AvmMini_opcode.hpp" #include "barretenberg/vm/avm_trace/AvmMini_trace.hpp" @@ -10,6 +10,7 @@ #include #include #include +#include #include using namespace bb; @@ -27,7 +28,7 @@ namespace avm_trace { */ HonkProof Execution::run_and_prove(std::vector const& bytecode, std::vector const& calldata) { - auto instructions = parse(bytecode); + auto instructions = Deserialization::parse(bytecode); auto trace = gen_trace(instructions, calldata); auto circuit_builder = bb::AvmMiniCircuitBuilder(); circuit_builder.set_trace(std::move(trace)); @@ -37,122 +38,6 @@ HonkProof Execution::run_and_prove(std::vector const& bytecode, std::ve return prover.construct_proof(); } -/** - * @brief Parsing of the supplied bytecode into a vector of instructions. It essentially - * checks that each opcode value is in the defined range and extracts the operands - * for each opcode. - * - * @param bytecode The bytecode to be parsed as a vector of bytes/uint8_t - * @throws runtime_error exception when the bytecode is invalid. - * @return Vector of instructions - */ -std::vector Execution::parse(std::vector const& bytecode) -{ - std::vector instructions; - size_t pos = 0; - const auto length = bytecode.size(); - - while (pos < length) { - const uint8_t opcode_byte = bytecode.at(pos); - pos += AVM_OPCODE_BYTE_LENGTH; - - if (!Bytecode::is_valid(opcode_byte)) { - throw_or_abort("Invalid opcode byte: " + std::to_string(opcode_byte)); - } - - const auto opcode = static_cast(opcode_byte); - auto in_tag_u8 = static_cast(AvmMemoryTag::U0); - - if (Bytecode::has_in_tag(opcode)) { - if (pos + AVM_IN_TAG_BYTE_LENGTH > length) { - throw_or_abort("Instruction tag missing at position " + std::to_string(pos)); - } - in_tag_u8 = bytecode.at(pos); - if (in_tag_u8 == static_cast(AvmMemoryTag::U0) || in_tag_u8 > MAX_MEM_TAG) { - throw_or_abort("Instruction tag is invalid at position " + std::to_string(pos) + - " value: " + std::to_string(in_tag_u8)); - } - pos += AVM_IN_TAG_BYTE_LENGTH; - } - - auto const in_tag = static_cast(in_tag_u8); - std::vector operands{}; - size_t num_of_operands{}; - size_t operands_size{}; - - // SET opcode particularity about the number of operands depending on the - // instruction tag. Namely, a constant of type instruction tag and not a - // memory address is passed in the operands. - // The bytecode of the operands is of the form CONSTANT || dst_offset - // CONSTANT is of size k bits for type Uk, k=8,16,32,64,128 - // dst_offset is of size 32 bits - // CONSTANT has to be decomposed into 32-bit chunks - if (opcode == OpCode::SET) { - switch (in_tag) { - case AvmMemoryTag::U8: - num_of_operands = 2; - operands_size = 5; - break; - case AvmMemoryTag::U16: - num_of_operands = 2; - operands_size = 6; - break; - case AvmMemoryTag::U32: - num_of_operands = 2; - operands_size = 8; - break; - case AvmMemoryTag::U64: - num_of_operands = 3; - operands_size = 12; - break; - case AvmMemoryTag::U128: - num_of_operands = 5; - operands_size = 20; - break; - default: - throw_or_abort("Instruction tag for SET opcode is invalid at position " + std::to_string(pos) + - " value: " + std::to_string(in_tag_u8)); - break; - } - } else { - num_of_operands = Bytecode::OPERANDS_NUM.at(opcode); - operands_size = AVM_OPERAND_BYTE_LENGTH * num_of_operands; - } - - if (pos + operands_size > length) { - throw_or_abort("Operand is missing at position " + std::to_string(pos)); - } - - // We handle operands which are encoded with less than 4 bytes. - // This occurs for opcode SET and tag U8 and U16. - if (opcode == OpCode::SET && in_tag == AvmMemoryTag::U8) { - operands.push_back(static_cast(bytecode.at(pos))); - pos++; - num_of_operands--; - } else if (opcode == OpCode::SET && in_tag == AvmMemoryTag::U16) { - uint8_t const* ptr = &bytecode.at(pos); - uint16_t operand{}; - serialize::read(ptr, operand); - operands.push_back(static_cast(operand)); - pos += 2; - num_of_operands--; - } - - // Operands of size of 32 bits. - for (size_t i = 0; i < num_of_operands; i++) { - uint8_t const* ptr = &bytecode.at(pos); - uint32_t operand{}; - serialize::read(ptr, operand); - operands.push_back(operand); - pos += AVM_OPERAND_BYTE_LENGTH; - } - - instructions.emplace_back(opcode, operands, static_cast(in_tag)); - } - - return instructions; -} - /** * @brief Generate the execution trace pertaining to the supplied instructions. * @@ -164,7 +49,7 @@ std::vector Execution::gen_trace(std::vector const& instructio { AvmMiniTraceBuilder trace_builder{}; - // copied version of pc maintained in trace builder. The value of pc is evolving based + // Copied version of pc maintained in trace builder. The value of pc is evolving based // on opcode logic and therefore is not maintained here. However, the next opcode in the execution // is determined by this value which require read access to the code below. uint32_t pc = 0; @@ -174,62 +59,82 @@ std::vector Execution::gen_trace(std::vector const& instructio auto inst = instructions.at(pc); switch (inst.op_code) { + // Compute + // Compute - Arithmetic case OpCode::ADD: - trace_builder.add(inst.operands.at(0), inst.operands.at(1), inst.operands.at(2), inst.in_tag); + trace_builder.add(std::get(inst.operands.at(1)), + std::get(inst.operands.at(2)), + std::get(inst.operands.at(3)), + std::get(inst.operands.at(0))); break; case OpCode::SUB: - trace_builder.sub(inst.operands.at(0), inst.operands.at(1), inst.operands.at(2), inst.in_tag); + trace_builder.sub(std::get(inst.operands.at(1)), + std::get(inst.operands.at(2)), + std::get(inst.operands.at(3)), + std::get(inst.operands.at(0))); break; case OpCode::MUL: - trace_builder.mul(inst.operands.at(0), inst.operands.at(1), inst.operands.at(2), inst.in_tag); + trace_builder.mul(std::get(inst.operands.at(1)), + std::get(inst.operands.at(2)), + std::get(inst.operands.at(3)), + std::get(inst.operands.at(0))); break; case OpCode::DIV: - trace_builder.div(inst.operands.at(0), inst.operands.at(1), inst.operands.at(2), inst.in_tag); + trace_builder.div(std::get(inst.operands.at(1)), + std::get(inst.operands.at(2)), + std::get(inst.operands.at(3)), + std::get(inst.operands.at(0))); break; + // Execution Environment - Calldata case OpCode::CALLDATACOPY: - trace_builder.calldata_copy(inst.operands.at(0), inst.operands.at(1), inst.operands.at(2), calldata); + trace_builder.calldata_copy(std::get(inst.operands.at(0)), + std::get(inst.operands.at(1)), + std::get(inst.operands.at(2)), + calldata); break; + // Machine State - Internal Control Flow case OpCode::JUMP: - trace_builder.jump(inst.operands.at(0)); + trace_builder.jump(std::get(inst.operands.at(0))); break; case OpCode::INTERNALCALL: - trace_builder.internal_call(inst.operands.at(0)); + trace_builder.internal_call(std::get(inst.operands.at(0))); break; case OpCode::INTERNALRETURN: trace_builder.internal_return(); break; + // Machine State - Memory case OpCode::SET: { uint32_t dst_offset{}; uint128_t val{}; - switch (inst.in_tag) { + AvmMemoryTag in_tag = std::get(inst.operands.at(0)); + dst_offset = std::get(inst.operands.at(2)); + + switch (in_tag) { case AvmMemoryTag::U8: + val = std::get(inst.operands.at(1)); + break; case AvmMemoryTag::U16: + val = std::get(inst.operands.at(1)); + break; case AvmMemoryTag::U32: - // U8, U16, U32 value represented in a single uint32_t operand - val = inst.operands.at(0); - dst_offset = inst.operands.at(1); + val = std::get(inst.operands.at(1)); break; - case AvmMemoryTag::U64: // value represented as 2 uint32_t operands - val = inst.operands.at(0); - val <<= 32; - val |= inst.operands.at(1); - dst_offset = inst.operands.at(2); + case AvmMemoryTag::U64: + val = std::get(inst.operands.at(1)); break; - case AvmMemoryTag::U128: // value represented as 4 uint32_t operands - for (size_t i = 0; i < 4; i++) { - val |= inst.operands.at(i); - val <<= 32; - } - dst_offset = inst.operands.at(4); + case AvmMemoryTag::U128: + val = std::get(inst.operands.at(1)); break; default: break; } - trace_builder.set(val, dst_offset, inst.in_tag); + + trace_builder.set(val, dst_offset, in_tag); break; } + // Control Flow - Contract Calls case OpCode::RETURN: - trace_builder.return_op(inst.operands.at(0), inst.operands.at(1)); + trace_builder.return_op(std::get(inst.operands.at(0)), std::get(inst.operands.at(1))); break; default: break; diff --git a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_execution.hpp b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_execution.hpp index 6c31f52d0404..337e54df8795 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_execution.hpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_execution.hpp @@ -14,13 +14,6 @@ class Execution { public: Execution() = default; - static size_t const AVM_OPERAND_BYTE_LENGTH = 4; // Keep in sync with TS code - static_assert(sizeof(uint32_t) / sizeof(uint8_t) == AVM_OPERAND_BYTE_LENGTH); - - static size_t const AVM_OPCODE_BYTE_LENGTH = 1; // Keep in sync with TS code - static size_t const AVM_IN_TAG_BYTE_LENGTH = 1; // Keep in sync with TS code - - static std::vector parse(std::vector const& bytecode); static std::vector gen_trace(std::vector const& instructions, std::vector const& calldata); static bb::HonkProof run_and_prove(std::vector const& bytecode, std::vector const& calldata = std::vector{}); diff --git a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_instructions.hpp b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_instructions.hpp index 0cc18e560875..497961685eae 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_instructions.hpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_instructions.hpp @@ -1,5 +1,6 @@ #pragma once +#include "barretenberg/numeric/uint128/uint128.hpp" #include "barretenberg/vm/avm_trace/AvmMini_common.hpp" #include "barretenberg/vm/avm_trace/AvmMini_opcode.hpp" #include @@ -7,17 +8,17 @@ namespace avm_trace { +using Operand = std::variant; + class Instruction { public: OpCode op_code; - std::vector operands; - AvmMemoryTag in_tag; + std::vector operands; Instruction() = delete; - explicit Instruction(OpCode op_code, std::vector operands, AvmMemoryTag in_tag) + explicit Instruction(OpCode op_code, std::vector operands) : op_code(op_code) - , operands(std::move(operands)) - , in_tag(in_tag){}; + , operands(std::move(operands)){}; }; } // namespace avm_trace \ No newline at end of file diff --git a/barretenberg/cpp/src/barretenberg/vm/tests/AvmMini_execution.test.cpp b/barretenberg/cpp/src/barretenberg/vm/tests/AvmMini_execution.test.cpp index ce2fcf868b12..6ac3867618e0 100644 --- a/barretenberg/cpp/src/barretenberg/vm/tests/AvmMini_execution.test.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/tests/AvmMini_execution.test.cpp @@ -2,6 +2,7 @@ #include "AvmMini_common.test.hpp" #include "barretenberg/common/utils.hpp" #include "barretenberg/vm/avm_trace/AvmMini_common.hpp" +#include "barretenberg/vm/avm_trace/AvmMini_deserialization.hpp" #include "barretenberg/vm/avm_trace/AvmMini_helper.hpp" #include "barretenberg/vm/avm_trace/AvmMini_opcode.hpp" #include "barretenberg/vm/tests/helpers.test.hpp" @@ -60,24 +61,28 @@ TEST_F(AvmMiniExecutionTests, basicAddReturn) "00000000"; // ret size 0 auto bytecode = hex_to_bytes(bytecode_hex); - auto instructions = Execution::parse(bytecode); + auto instructions = Deserialization::parse(bytecode); // 2 instructions EXPECT_EQ(instructions.size(), 2); // ADD EXPECT_EQ(instructions.at(0).op_code, OpCode::ADD); - EXPECT_EQ(instructions.at(0).operands.size(), 3); - EXPECT_EQ(instructions.at(0).operands.at(0), 7); - EXPECT_EQ(instructions.at(0).operands.at(1), 9); - EXPECT_EQ(instructions.at(0).operands.at(2), 1); - EXPECT_EQ(instructions.at(0).in_tag, AvmMemoryTag::U8); + + auto operands = instructions.at(0).operands; + EXPECT_EQ(operands.size(), 4); + EXPECT_EQ(std::get(operands.at(0)), AvmMemoryTag::U8); + EXPECT_EQ(std::get(operands.at(1)), 7); + EXPECT_EQ(std::get(operands.at(2)), 9); + EXPECT_EQ(std::get(operands.at(3)), 1); // RETURN EXPECT_EQ(instructions.at(1).op_code, OpCode::RETURN); - EXPECT_EQ(instructions.at(1).operands.size(), 2); - EXPECT_EQ(instructions.at(1).operands.at(0), 0); - EXPECT_EQ(instructions.at(1).operands.at(0), 0); + + operands = instructions.at(1).operands; + EXPECT_EQ(operands.size(), 2); + EXPECT_EQ(std::get(operands.at(0)), 0); + EXPECT_EQ(std::get(operands.at(1)), 0); auto trace = Execution::gen_trace(instructions, std::vector{}); @@ -105,30 +110,37 @@ TEST_F(AvmMiniExecutionTests, setAndSubOpcodes) "00000000"; // ret size 0 auto bytecode = hex_to_bytes(bytecode_hex); - auto instructions = Execution::parse(bytecode); + auto instructions = Deserialization::parse(bytecode); EXPECT_EQ(instructions.size(), 4); // SET EXPECT_EQ(instructions.at(0).op_code, OpCode::SET); - EXPECT_EQ(instructions.at(0).operands.size(), 2); - EXPECT_EQ(instructions.at(0).operands.at(0), 47123); - EXPECT_EQ(instructions.at(0).operands.at(1), 170); - EXPECT_EQ(instructions.at(0).in_tag, AvmMemoryTag::U16); + + auto operands = instructions.at(0).operands; + EXPECT_EQ(operands.size(), 3); + EXPECT_EQ(std::get(operands.at(0)), AvmMemoryTag::U16); + EXPECT_EQ(std::get(operands.at(1)), 47123); + EXPECT_EQ(std::get(operands.at(2)), 170); // SET EXPECT_EQ(instructions.at(1).op_code, OpCode::SET); - EXPECT_EQ(instructions.at(1).operands.size(), 2); - EXPECT_EQ(instructions.at(1).operands.at(0), 37123); - EXPECT_EQ(instructions.at(1).operands.at(1), 51); - EXPECT_EQ(instructions.at(1).in_tag, AvmMemoryTag::U16); + + operands = instructions.at(1).operands; + EXPECT_EQ(operands.size(), 3); + EXPECT_EQ(std::get(operands.at(0)), AvmMemoryTag::U16); + EXPECT_EQ(std::get(operands.at(1)), 37123); + EXPECT_EQ(std::get(operands.at(2)), 51); // SUB EXPECT_EQ(instructions.at(2).op_code, OpCode::SUB); - EXPECT_EQ(instructions.at(2).operands.size(), 3); - EXPECT_EQ(instructions.at(2).operands.at(0), 170); - EXPECT_EQ(instructions.at(2).operands.at(1), 51); - EXPECT_EQ(instructions.at(2).in_tag, AvmMemoryTag::U16); + + operands = instructions.at(2).operands; + EXPECT_EQ(operands.size(), 4); + EXPECT_EQ(std::get(operands.at(0)), AvmMemoryTag::U16); + EXPECT_EQ(std::get(operands.at(1)), 170); + EXPECT_EQ(std::get(operands.at(2)), 51); + EXPECT_EQ(std::get(operands.at(3)), 1); auto trace = Execution::gen_trace(instructions, std::vector{}); @@ -176,31 +188,37 @@ TEST_F(AvmMiniExecutionTests, powerWithMulOpcodes) bytecode_hex.append(ret_hex); auto bytecode = hex_to_bytes(bytecode_hex); - auto instructions = Execution::parse(bytecode); + auto instructions = Deserialization::parse(bytecode); EXPECT_EQ(instructions.size(), 15); // MUL first pos EXPECT_EQ(instructions.at(2).op_code, OpCode::MUL); - EXPECT_EQ(instructions.at(2).operands.size(), 3); - EXPECT_EQ(instructions.at(2).operands.at(0), 0); - EXPECT_EQ(instructions.at(2).operands.at(1), 1); - EXPECT_EQ(instructions.at(2).operands.at(2), 1); - EXPECT_EQ(instructions.at(2).in_tag, AvmMemoryTag::U64); + + auto operands = instructions.at(2).operands; + EXPECT_EQ(operands.size(), 4); + EXPECT_EQ(std::get(operands.at(0)), AvmMemoryTag::U64); + EXPECT_EQ(std::get(operands.at(1)), 0); + EXPECT_EQ(std::get(operands.at(2)), 1); + EXPECT_EQ(std::get(operands.at(3)), 1); // MUL last pos EXPECT_EQ(instructions.at(13).op_code, OpCode::MUL); - EXPECT_EQ(instructions.at(13).operands.size(), 3); - EXPECT_EQ(instructions.at(13).operands.at(0), 0); - EXPECT_EQ(instructions.at(13).operands.at(1), 1); - EXPECT_EQ(instructions.at(13).operands.at(2), 1); - EXPECT_EQ(instructions.at(13).in_tag, AvmMemoryTag::U64); + + operands = instructions.at(13).operands; + EXPECT_EQ(operands.size(), 4); + EXPECT_EQ(std::get(operands.at(0)), AvmMemoryTag::U64); + EXPECT_EQ(std::get(operands.at(1)), 0); + EXPECT_EQ(std::get(operands.at(2)), 1); + EXPECT_EQ(std::get(operands.at(3)), 1); // RETURN EXPECT_EQ(instructions.at(14).op_code, OpCode::RETURN); - EXPECT_EQ(instructions.at(14).operands.size(), 2); - EXPECT_EQ(instructions.at(14).operands.at(0), 0); - EXPECT_EQ(instructions.at(14).operands.at(0), 0); + operands = instructions.at(14).operands; + + EXPECT_EQ(operands.size(), 2); + EXPECT_EQ(std::get(operands.at(0)), 0); + EXPECT_EQ(std::get(operands.at(1)), 0); auto trace = Execution::gen_trace(instructions, std::vector{}); @@ -245,7 +263,7 @@ TEST_F(AvmMiniExecutionTests, simpleInternalCall) ; auto bytecode = hex_to_bytes(bytecode_hex); - auto instructions = Execution::parse(bytecode); + auto instructions = Deserialization::parse(bytecode); EXPECT_EQ(instructions.size(), 6); @@ -254,7 +272,7 @@ TEST_F(AvmMiniExecutionTests, simpleInternalCall) // INTERNALCALL EXPECT_EQ(instructions.at(1).op_code, OpCode::INTERNALCALL); EXPECT_EQ(instructions.at(1).operands.size(), 1); - EXPECT_EQ(instructions.at(1).operands.at(0), 4); + EXPECT_EQ(std::get(instructions.at(1).operands.at(0)), 4); // INTERNALRETURN EXPECT_EQ(instructions.at(5).op_code, OpCode::INTERNALRETURN); @@ -322,7 +340,7 @@ TEST_F(AvmMiniExecutionTests, nestedInternalCalls) bytecode_f2 + bytecode_f1 + bytecode_g; auto bytecode = hex_to_bytes(bytecode_hex); - auto instructions = Execution::parse(bytecode); + auto instructions = Deserialization::parse(bytecode); EXPECT_EQ(instructions.size(), 12); @@ -385,7 +403,7 @@ TEST_F(AvmMiniExecutionTests, jumpAndCalldatacopy) ; auto bytecode = hex_to_bytes(bytecode_hex); - auto instructions = Execution::parse(bytecode); + auto instructions = Deserialization::parse(bytecode); EXPECT_EQ(instructions.size(), 5); @@ -394,14 +412,16 @@ TEST_F(AvmMiniExecutionTests, jumpAndCalldatacopy) // CALLDATACOPY EXPECT_EQ(instructions.at(0).op_code, OpCode::CALLDATACOPY); EXPECT_EQ(instructions.at(0).operands.size(), 3); - EXPECT_EQ(instructions.at(0).operands.at(0), 0); - EXPECT_EQ(instructions.at(0).operands.at(1), 2); - EXPECT_EQ(instructions.at(0).operands.at(2), 10); + + auto operands = instructions.at(0).operands; + EXPECT_EQ(std::get(operands.at(0)), 0); + EXPECT_EQ(std::get(operands.at(1)), 2); + EXPECT_EQ(std::get(operands.at(2)), 10); // JUMP EXPECT_EQ(instructions.at(1).op_code, OpCode::JUMP); EXPECT_EQ(instructions.at(1).operands.size(), 1); - EXPECT_EQ(instructions.at(1).operands.at(0), 3); + EXPECT_EQ(std::get(instructions.at(1).operands.at(0)), 3); auto trace = Execution::gen_trace(instructions, std::vector{ 13, 156 }); @@ -437,7 +457,7 @@ TEST_F(AvmMiniExecutionTests, invalidOpcode) "00000000"; // ret size 0 auto bytecode = hex_to_bytes(bytecode_hex); - EXPECT_THROW_WITH_MESSAGE(Execution::parse(bytecode), "opcode"); + EXPECT_THROW_WITH_MESSAGE(Deserialization::parse(bytecode), "Invalid opcode"); } // Negative test detecting an invalid memmory instruction tag. @@ -453,7 +473,7 @@ TEST_F(AvmMiniExecutionTests, invalidInstructionTag) "00000000"; // ret size 0 auto bytecode = hex_to_bytes(bytecode_hex); - EXPECT_THROW_WITH_MESSAGE(Execution::parse(bytecode), "Instruction tag is invalid"); + EXPECT_THROW_WITH_MESSAGE(Deserialization::parse(bytecode), "Instruction tag is invalid"); } // Negative test detecting SET opcode with instruction memory tag set to FF. @@ -469,7 +489,21 @@ TEST_F(AvmMiniExecutionTests, ffInstructionTagSetOpcode) "00002344"; // auto bytecode = hex_to_bytes(bytecode_hex); - EXPECT_THROW_WITH_MESSAGE(Execution::parse(bytecode), "Instruction tag for SET opcode is invalid"); + EXPECT_THROW_WITH_MESSAGE(Deserialization::parse(bytecode), "Instruction tag for SET opcode is invalid"); +} + +// Negative test detecting SET opcode without any operand. +TEST_F(AvmMiniExecutionTests, SetOpcodeNoOperand) +{ + std::string bytecode_hex = "00" // ADD + "05" // U128 + "00000007" // addr a 7 + "00000009" // addr b 9 + "00000001" // addr c 1 + "27"; // SET 39 = 0x27 + + auto bytecode = hex_to_bytes(bytecode_hex); + EXPECT_THROW_WITH_MESSAGE(Deserialization::parse(bytecode), "Operand for SET opcode is missing"); } // Negative test detecting an incomplete instruction: missing instruction tag @@ -483,7 +517,7 @@ TEST_F(AvmMiniExecutionTests, truncatedInstructionNoTag) "01"; // SUB auto bytecode = hex_to_bytes(bytecode_hex); - EXPECT_THROW_WITH_MESSAGE(Execution::parse(bytecode), "Instruction tag missing"); + EXPECT_THROW_WITH_MESSAGE(Deserialization::parse(bytecode), "Operand is missing"); } // Negative test detecting an incomplete instruction: instruction tag present but an operand is missing @@ -500,7 +534,7 @@ TEST_F(AvmMiniExecutionTests, truncatedInstructionNoOperand) "FFFFFFBB"; // addr b and missing address for c = a-b auto bytecode = hex_to_bytes(bytecode_hex); - EXPECT_THROW_WITH_MESSAGE(Execution::parse(bytecode), "Operand is missing"); + EXPECT_THROW_WITH_MESSAGE(Deserialization::parse(bytecode), "Operand is missing"); } } // namespace tests_avm \ No newline at end of file diff --git a/yarn-project/simulator/src/avm/serialization/instruction_serialization.ts b/yarn-project/simulator/src/avm/serialization/instruction_serialization.ts index 0cba3caef370..38acdf9bfdde 100644 --- a/yarn-project/simulator/src/avm/serialization/instruction_serialization.ts +++ b/yarn-project/simulator/src/avm/serialization/instruction_serialization.ts @@ -67,7 +67,10 @@ export enum Opcode { TOTAL_OPCODES_NUMBER, } -// Possible types for an instruction's operand in its wire format. +// Possible types for an instruction's operand in its wire format. (Keep in sync with CPP code. +// See vm/avm_trace/AvmMini_deserialization.cpp) +// Note that cpp code introduced an additional enum value TAG to express the instruction tag. In TS, +// this one is parsed as UINT8. export enum OperandType { UINT8, UINT16, From b5a9393b9746c2747f3de8c866d2f402724687bd Mon Sep 17 00:00:00 2001 From: jeanmon Date: Tue, 6 Feb 2024 16:49:20 +0000 Subject: [PATCH 3/5] 4304 - address review comments --- .../vm/avm_trace/AvmMini_deserialization.cpp | 61 ++++++++++--------- .../vm/avm_trace/AvmMini_deserialization.hpp | 4 -- .../vm/avm_trace/AvmMini_execution.hpp | 6 +- .../vm/tests/AvmMini_execution.test.cpp | 8 +-- 4 files changed, 38 insertions(+), 41 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_deserialization.cpp b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_deserialization.cpp index 17ece6955499..a2ab8a882bd9 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_deserialization.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_deserialization.cpp @@ -2,13 +2,16 @@ #include "barretenberg/vm/avm_trace/AvmMini_common.hpp" #include "barretenberg/vm/avm_trace/AvmMini_instructions.hpp" #include "barretenberg/vm/avm_trace/AvmMini_opcode.hpp" +#include #include #include +#include #include #include +namespace avm_trace { + namespace { -using namespace avm_trace; const std::vector three_operand_format = { OperandType::TAG, @@ -17,13 +20,9 @@ const std::vector three_operand_format = { OperandType::UINT32, }; -} // Anonymous namespace - -namespace avm_trace { - // Contrary to TS, the format does not contain the opcode byte which prefixes any instruction. // The format for OpCode::SET has to be handled separately as it is variable based on the tag. -const std::unordered_map> Deserialization::OPCODE_WIRE_FORMAT = { +const std::unordered_map> OPCODE_WIRE_FORMAT = { // Compute // Compute - Arithmetic { OpCode::ADD, three_operand_format }, @@ -42,11 +41,13 @@ const std::unordered_map> Deserialization::OPCO { OpCode::RETURN, { OperandType::UINT32, OperandType::UINT32 } }, }; -const std::unordered_map Deserialization::OPERAND_TYPE_SIZE = { +const std::unordered_map OPERAND_TYPE_SIZE = { { OperandType::TAG, 1 }, { OperandType::UINT8, 1 }, { OperandType::UINT16, 2 }, { OperandType::UINT32, 4 }, { OperandType::UINT64, 8 }, { OperandType::UINT128, 16 }, }; +} // Anonymous namespace + /** * @brief Parsing of the supplied bytecode into a vector of instructions. It essentially * checks that each opcode value is in the defined range and extracts the operands @@ -78,13 +79,15 @@ std::vector Deserialization::parse(std::vector const& byte if (pos == length) { throw_or_abort("Operand for SET opcode is missing at position " + std::to_string(pos)); } + + std::set const valid_tags = { static_cast(AvmMemoryTag::U8), + static_cast(AvmMemoryTag::U16), + static_cast(AvmMemoryTag::U32), + static_cast(AvmMemoryTag::U64), + static_cast(AvmMemoryTag::U128) }; uint8_t set_tag_u8 = bytecode.at(pos); - if (!std::set{ static_cast(AvmMemoryTag::U8), - static_cast(AvmMemoryTag::U16), - static_cast(AvmMemoryTag::U32), - static_cast(AvmMemoryTag::U64), - static_cast(AvmMemoryTag::U128) } - .contains(set_tag_u8)) { + + if (!valid_tags.contains(set_tag_u8)) { throw_or_abort("Instruction tag for SET opcode is invalid at position " + std::to_string(pos) + " value: " + std::to_string(set_tag_u8)); } @@ -108,20 +111,20 @@ std::vector Deserialization::parse(std::vector const& byte break; default: // This branch is guarded above. inst_format = {}; + std::cerr << "This code branch must have been guarded by the tag validation. \n"; + assert(false); } } else { inst_format = OPCODE_WIRE_FORMAT.at(opcode); } - std::vector operands{}; + std::vector operands; - for (auto const& opType : inst_format) { + for (OperandType const& opType : inst_format) { // No underflow as while condition guarantees pos <= length (after pos++) if (length - pos < OPERAND_TYPE_SIZE.at(opType)) { throw_or_abort("Operand is missing at position " + std::to_string(pos)); } - Operand operand; - uint8_t const* pos_ptr = &bytecode.at(pos); switch (opType) { case OperandType::TAG: { @@ -130,44 +133,42 @@ std::vector Deserialization::parse(std::vector const& byte throw_or_abort("Instruction tag is invalid at position " + std::to_string(pos) + " value: " + std::to_string(tag_u8)); } - operand = static_cast(tag_u8); - pos++; + operands.emplace_back(static_cast(tag_u8)); break; } case OperandType::UINT8: - operand = bytecode.at(pos); - pos++; + operands.emplace_back(bytecode.at(pos)); break; case OperandType::UINT16: { uint16_t operand_u16{}; + uint8_t const* pos_ptr = &bytecode.at(pos); serialize::read(pos_ptr, operand_u16); - operand = operand_u16; - pos += 2; + operands.emplace_back(operand_u16); break; } case OperandType::UINT32: { uint32_t operand_u32{}; + uint8_t const* pos_ptr = &bytecode.at(pos); serialize::read(pos_ptr, operand_u32); - operand = operand_u32; - pos += 4; + operands.emplace_back(operand_u32); break; } case OperandType::UINT64: { uint64_t operand_u64{}; + uint8_t const* pos_ptr = &bytecode.at(pos); serialize::read(pos_ptr, operand_u64); - operand = operand_u64; - pos += 8; + operands.emplace_back(operand_u64); break; } case OperandType::UINT128: { uint128_t operand_u128{}; + uint8_t const* pos_ptr = &bytecode.at(pos); serialize::read(pos_ptr, operand_u128); - operand = operand_u128; - pos += 16; + operands.emplace_back(operand_u128); break; } } - operands.push_back(operand); + pos += OPERAND_TYPE_SIZE.at(opType); } instructions.emplace_back(opcode, operands); } diff --git a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_deserialization.hpp b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_deserialization.hpp index 93493b8564c0..91a52eaf41b9 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_deserialization.hpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_deserialization.hpp @@ -22,10 +22,6 @@ class Deserialization { Deserialization() = default; static std::vector parse(std::vector const& bytecode); - - private: - static const std::unordered_map> OPCODE_WIRE_FORMAT; - static const std::unordered_map OPERAND_TYPE_SIZE; }; } // namespace avm_trace \ No newline at end of file diff --git a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_execution.hpp b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_execution.hpp index 337e54df8795..40c75d186e20 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_execution.hpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_execution.hpp @@ -14,9 +14,9 @@ class Execution { public: Execution() = default; - static std::vector gen_trace(std::vector const& instructions, std::vector const& calldata); - static bb::HonkProof run_and_prove(std::vector const& bytecode, - std::vector const& calldata = std::vector{}); + static std::vector gen_trace(std::vector const& instructions, + std::vector const& calldata = {}); + static bb::HonkProof run_and_prove(std::vector const& bytecode, std::vector const& calldata = {}); }; } // namespace avm_trace \ No newline at end of file diff --git a/barretenberg/cpp/src/barretenberg/vm/tests/AvmMini_execution.test.cpp b/barretenberg/cpp/src/barretenberg/vm/tests/AvmMini_execution.test.cpp index 6ac3867618e0..e02cddcb81ef 100644 --- a/barretenberg/cpp/src/barretenberg/vm/tests/AvmMini_execution.test.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/tests/AvmMini_execution.test.cpp @@ -142,7 +142,7 @@ TEST_F(AvmMiniExecutionTests, setAndSubOpcodes) EXPECT_EQ(std::get(operands.at(2)), 51); EXPECT_EQ(std::get(operands.at(3)), 1); - auto trace = Execution::gen_trace(instructions, std::vector{}); + auto trace = Execution::gen_trace(instructions); // Find the first row enabling the subtraction selector auto row = std::ranges::find_if(trace.begin(), trace.end(), [](Row r) { return r.avmMini_sel_op_sub == 1; }); @@ -220,7 +220,7 @@ TEST_F(AvmMiniExecutionTests, powerWithMulOpcodes) EXPECT_EQ(std::get(operands.at(0)), 0); EXPECT_EQ(std::get(operands.at(1)), 0); - auto trace = Execution::gen_trace(instructions, std::vector{}); + auto trace = Execution::gen_trace(instructions); // Find the first row enabling the multiplication selector and pc = 13 auto row = std::ranges::find_if( @@ -277,7 +277,7 @@ TEST_F(AvmMiniExecutionTests, simpleInternalCall) // INTERNALRETURN EXPECT_EQ(instructions.at(5).op_code, OpCode::INTERNALRETURN); - auto trace = Execution::gen_trace(instructions, std::vector{}); + auto trace = Execution::gen_trace(instructions); // Expected sequence of PCs during execution std::vector pc_sequence{ 0, 1, 4, 5, 2, 3 }; @@ -356,7 +356,7 @@ TEST_F(AvmMiniExecutionTests, nestedInternalCalls) EXPECT_EQ(instructions.at(i).op_code, opcode_sequence.at(i)); } - auto trace = Execution::gen_trace(instructions, std::vector{}); + auto trace = Execution::gen_trace(instructions); // Expected sequence of PCs during execution std::vector pc_sequence{ 0, 1, 2, 8, 6, 7, 9, 10, 4, 5, 11, 3 }; From 20d7770fe4409685548852664fc607bc0b0ee14d Mon Sep 17 00:00:00 2001 From: jeanmon Date: Tue, 6 Feb 2024 17:40:42 +0000 Subject: [PATCH 4/5] 4304 - additional review comments addressed --- .../vm/avm_trace/AvmMini_alu_trace.cpp | 28 +++++++++---------- .../vm/avm_trace/AvmMini_deserialization.cpp | 9 +++--- .../vm/avm_trace/AvmMini_execution.cpp | 6 ++-- .../vm/avm_trace/AvmMini_trace.cpp | 4 +-- .../vm/tests/AvmMini_execution.test.cpp | 12 ++++---- 5 files changed, 29 insertions(+), 30 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_alu_trace.cpp b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_alu_trace.cpp index 7da44d916bd4..ed35f3f0720f 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_alu_trace.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_alu_trace.cpp @@ -47,11 +47,11 @@ std::vector AvmMiniAluTraceBuilder::final */ FF AvmMiniAluTraceBuilder::add(FF const& a, FF const& b, AvmMemoryTag in_tag, uint32_t const clk) { - FF c{}; + FF c = 0; bool carry = false; - uint8_t alu_u8_r0{}; - uint8_t alu_u8_r1{}; - std::array alu_u16_reg{}; + uint8_t alu_u8_r0 = 0; + uint8_t alu_u8_r1 = 0; + std::array alu_u16_reg; uint128_t a_u128{ a }; uint128_t b_u128{ b }; @@ -136,11 +136,11 @@ FF AvmMiniAluTraceBuilder::add(FF const& a, FF const& b, AvmMemoryTag in_tag, ui */ FF AvmMiniAluTraceBuilder::sub(FF const& a, FF const& b, AvmMemoryTag in_tag, uint32_t const clk) { - FF c{}; + FF c = 0; bool carry = false; - uint8_t alu_u8_r0{}; - uint8_t alu_u8_r1{}; - std::array alu_u16_reg{}; + uint8_t alu_u8_r0 = 0; + uint8_t alu_u8_r1 = 0; + std::array alu_u16_reg; uint128_t a_u128{ a }; uint128_t b_u128{ b }; uint128_t c_u128 = a_u128 - b_u128; @@ -220,12 +220,12 @@ FF AvmMiniAluTraceBuilder::sub(FF const& a, FF const& b, AvmMemoryTag in_tag, ui */ FF AvmMiniAluTraceBuilder::mul(FF const& a, FF const& b, AvmMemoryTag in_tag, uint32_t const clk) { - FF c{}; + FF c = 0; bool carry = false; - uint8_t alu_u8_r0{}; - uint8_t alu_u8_r1{}; + uint8_t alu_u8_r0 = 0; + uint8_t alu_u8_r1 = 0; - std::array alu_u16_reg{}; + std::array alu_u16_reg; uint128_t a_u128{ a }; uint128_t b_u128{ b }; @@ -258,8 +258,8 @@ FF AvmMiniAluTraceBuilder::mul(FF const& a, FF const& b, AvmMemoryTag in_tag, ui uint128_t c_u128 = a_u128 * b_u128; // Decompose a_u128 and b_u128 over 8 16-bit registers. - std::array alu_u16_reg_a{}; - std::array alu_u16_reg_b{}; + std::array alu_u16_reg_a; + std::array alu_u16_reg_b; uint128_t a_trunc_128 = a_u128; uint128_t b_trunc_128 = b_u128; diff --git a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_deserialization.cpp b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_deserialization.cpp index a2ab8a882bd9..dace5225ecff 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_deserialization.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_deserialization.cpp @@ -110,7 +110,6 @@ std::vector Deserialization::parse(std::vector const& byte inst_format = { OperandType::TAG, OperandType::UINT128, OperandType::UINT32 }; break; default: // This branch is guarded above. - inst_format = {}; std::cerr << "This code branch must have been guarded by the tag validation. \n"; assert(false); } @@ -140,28 +139,28 @@ std::vector Deserialization::parse(std::vector const& byte operands.emplace_back(bytecode.at(pos)); break; case OperandType::UINT16: { - uint16_t operand_u16{}; + uint16_t operand_u16 = 0; uint8_t const* pos_ptr = &bytecode.at(pos); serialize::read(pos_ptr, operand_u16); operands.emplace_back(operand_u16); break; } case OperandType::UINT32: { - uint32_t operand_u32{}; + uint32_t operand_u32 = 0; uint8_t const* pos_ptr = &bytecode.at(pos); serialize::read(pos_ptr, operand_u32); operands.emplace_back(operand_u32); break; } case OperandType::UINT64: { - uint64_t operand_u64{}; + uint64_t operand_u64 = 0; uint8_t const* pos_ptr = &bytecode.at(pos); serialize::read(pos_ptr, operand_u64); operands.emplace_back(operand_u64); break; } case OperandType::UINT128: { - uint128_t operand_u128{}; + uint128_t operand_u128 = 0; uint8_t const* pos_ptr = &bytecode.at(pos); serialize::read(pos_ptr, operand_u128); operands.emplace_back(operand_u128); diff --git a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_execution.cpp b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_execution.cpp index 79f0fc0d4434..0aae89481002 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_execution.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_execution.cpp @@ -47,7 +47,7 @@ HonkProof Execution::run_and_prove(std::vector const& bytecode, std::ve */ std::vector Execution::gen_trace(std::vector const& instructions, std::vector const& calldata) { - AvmMiniTraceBuilder trace_builder{}; + AvmMiniTraceBuilder trace_builder; // Copied version of pc maintained in trace builder. The value of pc is evolving based // on opcode logic and therefore is not maintained here. However, the next opcode in the execution @@ -104,8 +104,8 @@ std::vector Execution::gen_trace(std::vector const& instructio break; // Machine State - Memory case OpCode::SET: { - uint32_t dst_offset{}; - uint128_t val{}; + uint32_t dst_offset = 0; + uint128_t val = 0; AvmMemoryTag in_tag = std::get(inst.operands.at(0)); dst_offset = std::get(inst.operands.at(2)); diff --git a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_trace.cpp b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_trace.cpp index 88aac4a09e69..85f9906533ff 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_trace.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_trace.cpp @@ -377,7 +377,7 @@ std::vector AvmMiniTraceBuilder::return_op(uint32_t ret_offset, uint32_t ret { if (ret_size == 0) { halt(); - return std::vector{}; + return {}; } // We parallelize loading memory operations in chunk of 3, i.e., 1 per intermediate register. @@ -604,7 +604,7 @@ std::vector AvmMiniTraceBuilder::finalize() // Fill the rest with zeros. size_t zero_rows_num = AVM_TRACE_SIZE - main_trace_size - 1; while (zero_rows_num-- > 0) { - main_trace.push_back(Row{}); + main_trace.push_back({}); } main_trace.at(main_trace_size - 1).avmMini_last = FF(1); diff --git a/barretenberg/cpp/src/barretenberg/vm/tests/AvmMini_execution.test.cpp b/barretenberg/cpp/src/barretenberg/vm/tests/AvmMini_execution.test.cpp index e02cddcb81ef..89cc0b78e58b 100644 --- a/barretenberg/cpp/src/barretenberg/vm/tests/AvmMini_execution.test.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/tests/AvmMini_execution.test.cpp @@ -84,9 +84,9 @@ TEST_F(AvmMiniExecutionTests, basicAddReturn) EXPECT_EQ(std::get(operands.at(0)), 0); EXPECT_EQ(std::get(operands.at(1)), 0); - auto trace = Execution::gen_trace(instructions, std::vector{}); + auto trace = Execution::gen_trace(instructions); - gen_proof_and_validate(bytecode, std::move(trace), std::vector{}); + gen_proof_and_validate(bytecode, std::move(trace), {}); } // Positive test for SET and SUB opcodes @@ -148,7 +148,7 @@ TEST_F(AvmMiniExecutionTests, setAndSubOpcodes) auto row = std::ranges::find_if(trace.begin(), trace.end(), [](Row r) { return r.avmMini_sel_op_sub == 1; }); EXPECT_EQ(row->avmMini_ic, 10000); // 47123 - 37123 = 10000 - gen_proof_and_validate(bytecode, std::move(trace), std::vector{}); + gen_proof_and_validate(bytecode, std::move(trace), {}); } // Positive test for multiple MUL opcodes @@ -227,7 +227,7 @@ TEST_F(AvmMiniExecutionTests, powerWithMulOpcodes) trace.begin(), trace.end(), [](Row r) { return r.avmMini_sel_op_mul == 1 && r.avmMini_pc == 13; }); EXPECT_EQ(row->avmMini_ic, 244140625); // 5^12 = 244140625 - gen_proof_and_validate(bytecode, std::move(trace), std::vector{}); + gen_proof_and_validate(bytecode, std::move(trace), {}); } // Positive test about a single internal_call and internal_return @@ -290,7 +290,7 @@ TEST_F(AvmMiniExecutionTests, simpleInternalCall) auto row = std::ranges::find_if(trace.begin(), trace.end(), [](Row r) { return r.avmMini_sel_op_add == 1; }); EXPECT_EQ(row->avmMini_ic, 345567789); - gen_proof_and_validate(bytecode, std::move(trace), std::vector{}); + gen_proof_and_validate(bytecode, std::move(trace), {}); } // Positive test with some nested internall calls @@ -370,7 +370,7 @@ TEST_F(AvmMiniExecutionTests, nestedInternalCalls) EXPECT_EQ(row->avmMini_ic, 187); EXPECT_EQ(row->avmMini_pc, 4); - gen_proof_and_validate(bytecode, std::move(trace), std::vector{}); + gen_proof_and_validate(bytecode, std::move(trace), {}); } // Positive test with JUMP and CALLDATACOPY From 13ed96a6d8d227a834397520516832f8f4045b10 Mon Sep 17 00:00:00 2001 From: jeanmon Date: Tue, 6 Feb 2024 20:04:39 +0000 Subject: [PATCH 5/5] 4304 - fix uninitialization of some std::array's --- .../barretenberg/vm/avm_trace/AvmMini_alu_trace.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_alu_trace.cpp b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_alu_trace.cpp index ed35f3f0720f..1425fb6bf78d 100644 --- a/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_alu_trace.cpp +++ b/barretenberg/cpp/src/barretenberg/vm/avm_trace/AvmMini_alu_trace.cpp @@ -51,7 +51,7 @@ FF AvmMiniAluTraceBuilder::add(FF const& a, FF const& b, AvmMemoryTag in_tag, ui bool carry = false; uint8_t alu_u8_r0 = 0; uint8_t alu_u8_r1 = 0; - std::array alu_u16_reg; + std::array alu_u16_reg{}; // Must be zero-initialized (FF tag case) uint128_t a_u128{ a }; uint128_t b_u128{ b }; @@ -140,7 +140,7 @@ FF AvmMiniAluTraceBuilder::sub(FF const& a, FF const& b, AvmMemoryTag in_tag, ui bool carry = false; uint8_t alu_u8_r0 = 0; uint8_t alu_u8_r1 = 0; - std::array alu_u16_reg; + std::array alu_u16_reg{}; // Must be zero-initialized (FF tag case) uint128_t a_u128{ a }; uint128_t b_u128{ b }; uint128_t c_u128 = a_u128 - b_u128; @@ -225,7 +225,7 @@ FF AvmMiniAluTraceBuilder::mul(FF const& a, FF const& b, AvmMemoryTag in_tag, ui uint8_t alu_u8_r0 = 0; uint8_t alu_u8_r1 = 0; - std::array alu_u16_reg; + std::array alu_u16_reg{}; // Must be zero-initialized (FF tag case) uint128_t a_u128{ a }; uint128_t b_u128{ b }; @@ -258,8 +258,8 @@ FF AvmMiniAluTraceBuilder::mul(FF const& a, FF const& b, AvmMemoryTag in_tag, ui uint128_t c_u128 = a_u128 * b_u128; // Decompose a_u128 and b_u128 over 8 16-bit registers. - std::array alu_u16_reg_a; - std::array alu_u16_reg_b; + std::array alu_u16_reg_a; // Will be initialized in for loop below. + std::array alu_u16_reg_b; // Will be initialized in for loop below. uint128_t a_trunc_128 = a_u128; uint128_t b_trunc_128 = b_u128;