Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: avm e2e nested call + alu fix + cast fix #6974

Merged
merged 9 commits into from
Jun 7, 2024
Merged

feat: avm e2e nested call + alu fix + cast fix #6974

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4715e08
chore(avm): enable tag checking and some proving tests
fcarreiro Jun 7, 2024
8c083d2
chore(avm): add nested call proving test
fcarreiro Jun 7, 2024
2482227
fix
dbanks12 Jun 7, 2024
719994a
fix: avm alu issue
Maddiaa0 Jun 7, 2024
ba3a263
Merge branch 'master' into md/06-07-fix_avm_alu_issue
Maddiaa0 Jun 7, 2024
2cc0797
Add unit test for U128 to FF cast with a borrow
jeanmon Jun 7, 2024
bd63591
run it back
IlyasRidhuan Jun 7, 2024
eb7a3d5
merge master
Maddiaa0 Jun 7, 2024
5b9f8b6
temp: comment out call test
Maddiaa0 Jun 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion barretenberg/cpp/pil/avm/avm_alu.pil
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,7 @@ namespace avm_alu(256);
// 128-bit multiplication and CAST need two rows in ALU trace. We need to ensure
// that another ALU operation does not start in the second row.
#[TWO_LINE_OP_NO_OVERLAP]
(op_mul * ff_tag + op_cast) * alu_sel' = 0;
(op_mul * u128_tag + op_cast) * alu_sel' = 0;

// ========= SHIFT LEFT/RIGHT OPERATIONS ===============================
// Given (1) an input b, within the range [0, 2**128-1],
Expand Down
2 changes: 1 addition & 1 deletion barretenberg/cpp/src/barretenberg/relations/generated/avm/avm_alu.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -903,7 +903,7 @@ template <typename FF_> class avm_aluImpl {
{
Avm_DECLARE_VIEWS(54);

auto tmp = (((avm_alu_op_mul * avm_alu_ff_tag) + avm_alu_op_cast) * avm_alu_alu_sel_shift);
auto tmp = (((avm_alu_op_mul * avm_alu_u128_tag) + avm_alu_op_cast) * avm_alu_alu_sel_shift);
tmp *= scaling_factor;
std::get<54>(evals) += tmp;
}
Expand Down
10 changes: 8 additions & 2 deletions barretenberg/cpp/src/barretenberg/vm/avm_trace/avm_common.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#pragma once

#include "barretenberg/common/throw_or_abort.hpp"
#include "barretenberg/serialize/msgpack.hpp"
#include "barretenberg/vm/avm_trace/constants.hpp"
#include "barretenberg/vm/generated/avm_flavor.hpp"
Expand Down Expand Up @@ -45,8 +46,8 @@ static const uint32_t MAX_SIZE_INTERNAL_STACK = 1 << 16;
struct ExternalCallHint {
FF success;
std::vector<FF> return_data;
FF l2_gas_used;
FF da_gas_used;
uint32_t l2_gas_used;
uint32_t da_gas_used;
};

// Add support for deserialization of ExternalCallHint. This is implicitly used by serialize::read
Expand Down Expand Up @@ -169,6 +170,11 @@ struct ExecutionHints {
contract_instance_hints[instance.address] = instance;
}

if (it != data.data() + data.size()) {
throw_or_abort("Failed to deserialize ExecutionHints: only read" + std::to_string(it - data.data()) +
" bytes out of " + std::to_string(data.size()) + " bytes");
}

return { std::move(storage_value_hints), std::move(note_hash_exists_hints),
std::move(nullifier_exists_hints), std::move(l1_to_l2_message_exists_hints),
std::move(externalcall_hints), std::move(contract_instance_hints) };
Expand Down
81 changes: 36 additions & 45 deletions barretenberg/cpp/src/barretenberg/vm/avm_trace/avm_trace.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
#include <set>
#include <string>
#include <sys/types.h>
#include <unordered_map>
#include <vector>

#include "barretenberg/common/throw_or_abort.hpp"
Expand Down Expand Up @@ -2619,15 +2620,16 @@ uint32_t AvmTraceBuilder::read_slice_to_memory(uint8_t space_id,
* @param function_selector_offset An index in memory pointing to the function selector of the external call (TEMP)
*/
void AvmTraceBuilder::op_call([[maybe_unused]] uint8_t indirect,
uint32_t gas_offset,
uint32_t addr_offset,
uint32_t args_offset,
uint32_t args_size,
uint32_t ret_offset,
uint32_t ret_size,
uint32_t success_offset,
uint32_t function_selector_offset)
[[maybe_unused]] uint32_t gas_offset,
[[maybe_unused]] uint32_t addr_offset,
[[maybe_unused]] uint32_t args_offset,
[[maybe_unused]] uint32_t args_size,
[[maybe_unused]] uint32_t ret_offset,
[[maybe_unused]] uint32_t ret_size,
[[maybe_unused]] uint32_t success_offset,
[[maybe_unused]] uint32_t function_selector_offset)
{
// pc++;
auto clk = static_cast<uint32_t>(main_trace.size()) + 1;
const ExternalCallHint& hint = execution_hints.externalcall_hints.at(external_call_counter);
// We can load up to 4 things per row
Expand All @@ -2642,17 +2644,16 @@ void AvmTraceBuilder::op_call([[maybe_unused]] uint8_t indirect,
mem_trace_builder.indirect_read_and_load_from_memory(call_ptr, clk, IndirectRegister::IND_C, args_offset);

std::vector<uint32_t> first_row_load = {
uint32_t(read_ind_gas_offset.val), addr_offset, uint32_t(read_ind_args_offset.val), args_size
uint32_t(read_ind_gas_offset.val),
addr_offset,
uint32_t(read_ind_args_offset.val),
};
std::vector<uint32_t> first_row_values = {};
std::vector<FF> first_row_values = {};
for (uint32_t j = 0; j < first_row_load.size(); j++) {
// We just read and load to set up the constraints, we dont actually use these values for now.
auto mem_read = mem_trace_builder.read_and_load_from_memory(call_ptr,
clk,
register_order[j % register_order.size()],
first_row_load[j],
AvmMemoryTag::U32,
AvmMemoryTag::U0);
// info("Register order ", register_order[j]);
auto mem_read = mem_trace_builder.read_and_load_from_memory(
call_ptr, clk, register_order[j], first_row_load[j], AvmMemoryTag::FF, AvmMemoryTag::U0);
first_row_values.emplace_back(mem_read.val);
}

Expand All @@ -2662,7 +2663,6 @@ void AvmTraceBuilder::op_call([[maybe_unused]] uint8_t indirect,
.avm_main_ia = first_row_values[0], /* gas_offset */
.avm_main_ib = first_row_values[1], /* addr_offset */
.avm_main_ic = first_row_values[2], /* args_offset */
.avm_main_id = first_row_values[3], /* args_size */
.avm_main_ind_a = gas_offset,
.avm_main_ind_c = args_offset,
.avm_main_ind_op_a = FF(1),
Expand All @@ -2671,50 +2671,40 @@ void AvmTraceBuilder::op_call([[maybe_unused]] uint8_t indirect,
.avm_main_mem_idx_a = read_ind_gas_offset.val,
.avm_main_mem_idx_b = addr_offset,
.avm_main_mem_idx_c = read_ind_args_offset.val,
.avm_main_mem_idx_d = args_size,
.avm_main_mem_op_a = FF(1),
.avm_main_mem_op_b = FF(1),
.avm_main_mem_op_c = FF(1),
.avm_main_mem_op_d = FF(1),
.avm_main_pc = FF(pc++),
.avm_main_r_in_tag = FF(static_cast<uint32_t>(AvmMemoryTag::U32)),
.avm_main_r_in_tag = FF(static_cast<uint32_t>(AvmMemoryTag::FF)),
.avm_main_sel_external_call = FF(1),
});
clk++;
// Read the rest on a separate line, remember that the 4th operand is indirect
auto read_ind_ret_offset =
mem_trace_builder.indirect_read_and_load_from_memory(call_ptr, clk, IndirectRegister::IND_A, ret_offset);
std::vector<uint32_t> second_row_values = {};
std::vector<uint32_t> second_row_load = {
uint32_t(read_ind_ret_offset.val), function_selector_offset, ret_size, success_offset
};
for (uint32_t j = 0; j < second_row_load.size(); j++) {
// We just read and load to set up the constraints, we dont actually use these values for now.
auto mem_read = mem_trace_builder.read_and_load_from_memory(call_ptr,
clk,
register_order[j % register_order.size()],
second_row_load[j],
AvmMemoryTag::U32,
AvmMemoryTag::U0);
second_row_values.emplace_back(mem_read.val);
}
// We just read and load to set up the constraints, we dont actually use these values for now.
auto mem_read_ret = mem_trace_builder.read_and_load_from_memory(
call_ptr, clk, IntermRegister::IA, uint32_t(read_ind_ret_offset.val), AvmMemoryTag::FF, AvmMemoryTag::U0);
main_trace.push_back(Row{
.avm_main_clk = clk,
.avm_main_ia = second_row_values[0], /* ret_offset */
.avm_main_ib = second_row_values[1], /* function_selector_offset */
.avm_main_ic = second_row_values[2], /* ret_size */
.avm_main_id = second_row_values[3], /* success_offset */
.avm_main_ia = mem_read_ret.val, /* ret_offset */
.avm_main_ind_a = ret_offset,
.avm_main_ind_op_a = FF(1),
.avm_main_internal_return_ptr = FF(internal_return_ptr),
.avm_main_mem_idx_a = read_ind_ret_offset.val,
.avm_main_mem_idx_b = function_selector_offset,
.avm_main_mem_idx_c = ret_size,
.avm_main_mem_idx_d = success_offset,
.avm_main_mem_op_a = FF(1),
.avm_main_mem_op_b = FF(1),
.avm_main_mem_op_c = FF(1),
.avm_main_mem_op_d = FF(1),
.avm_main_pc = FF(pc),
.avm_main_r_in_tag = FF(static_cast<uint32_t>(AvmMemoryTag::FF)),
});
clk++;
auto mem_read_success = mem_trace_builder.read_and_load_from_memory(
call_ptr, clk, IntermRegister::IA, success_offset, AvmMemoryTag::U32, AvmMemoryTag::U0);
main_trace.push_back(Row{
.avm_main_clk = clk,
.avm_main_ia = mem_read_success.val, /* success_offset */
.avm_main_internal_return_ptr = FF(internal_return_ptr),
.avm_main_mem_idx_a = FF(success_offset),
.avm_main_mem_op_a = FF(1),
.avm_main_pc = FF(pc),
.avm_main_r_in_tag = FF(static_cast<uint32_t>(AvmMemoryTag::U32)),
});
Expand Down Expand Up @@ -3867,7 +3857,7 @@ std::vector<Row> AvmTraceBuilder::finalize(uint32_t min_trace_size, bool range_c
dest.avm_alu_op_eq_diff_inv = FF(src.alu_op_eq_diff_inv);

// Not all rows in ALU are enabled with a selector. For instance,
// multiplication over u128 is taking two lines.
// multiplication over u128 and cast is taking two lines.
if (AvmAluTraceBuilder::is_alu_row_enabled(src)) {
dest.avm_alu_alu_sel = FF(1);
}
Expand Down Expand Up @@ -3917,6 +3907,7 @@ std::vector<Row> AvmTraceBuilder::finalize(uint32_t min_trace_size, bool range_c
dest.avm_alu_a_hi = FF(src.hi_lo_limbs.at(1));
dest.avm_alu_p_sub_a_lo = FF(src.hi_lo_limbs.at(2));
dest.avm_alu_p_sub_a_hi = FF(src.hi_lo_limbs.at(3));
dest.avm_alu_p_a_borrow = FF(static_cast<uint8_t>(src.p_a_borrow));
dest.avm_alu_rng_chk_lookup_selector = FF(1);
}

Expand Down
8 changes: 8 additions & 0 deletions barretenberg/cpp/src/barretenberg/vm/tests/avm_cast.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,14 @@ TEST_F(AvmCastTests, sameTagU128)
uint256_t::from_uint128(a), FF(uint256_t::from_uint128(a)), 0, 1, AvmMemoryTag::U128, AvmMemoryTag::U128);
}

TEST_F(AvmCastTests, U128toFFWithBorrow)
{
uint128_t const a = (uint128_t{ 0x30644E72E131A029LLU } << 64) + uint128_t{ 0xB85045B68181585DLLU };
gen_trace(a, 0, 1, AvmMemoryTag::U128, AvmMemoryTag::FF);
validate_cast_trace(
uint256_t::from_uint128(a), FF(uint256_t::from_uint128(a)), 0, 1, AvmMemoryTag::U128, AvmMemoryTag::FF);
}

TEST_F(AvmCastTests, noTruncationFFToU32)
{
gen_trace(UINT32_MAX, 4, 9, AvmMemoryTag::FF, AvmMemoryTag::U32);
Expand Down
134 changes: 67 additions & 67 deletions barretenberg/cpp/src/barretenberg/vm/tests/avm_execution.test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2135,73 +2135,73 @@ TEST_F(AvmExecutionTests, kernelOutputHashExistsOpcodes)
validate_trace(std::move(trace));
}

TEST_F(AvmExecutionTests, opCallOpcodes)
{
std::string bytecode_preamble;
// Gas offset preamble
bytecode_preamble += to_hex(OpCode::SET) + // opcode SET for gas offset indirect
"00" // Indirect flag
"03" // U32
"00000010" // val 16 (address where gas offset is located)
"00000011" + // dst_offset 17
to_hex(OpCode::SET) + // opcode SET for value stored in gas offset
"00" // Indirect flag
"03" // U32
"00000011" // val i
"00000000";
// args offset preamble
bytecode_preamble += to_hex(OpCode::SET) + // opcode SET for args offset indirect
"00" // Indirect flag
"03" // U32
"00000100" // val i
"00000012" + // dst_offset 0
to_hex(OpCode::SET) + // opcode SET for value stored in args offset
"00" // Indirect flag
"03" // U32
"00000012" // val i
"00000001";
// ret offset preamble
bytecode_preamble += to_hex(OpCode::SET) + // opcode SET for ret offset indirect
"00" // Indirect flag
"03" // U32
"00000008" // val i
"00000004" + // dst_offset 0
to_hex(OpCode::SET) + // opcode SET for value stored in ret offset
"00" // Indirect flag
"03" // U32
"00000002" // val i
"00000007";
std::string bytecode_hex = bytecode_preamble // SET gas, addr, args size, ret offset, success, function selector
+ to_hex(OpCode::CALL) + // opcode CALL
"15" // Indirect flag
"00000000" // gas offset
"00000001" // addr offset
"00000002" // args offset
"00000003" // args size offset
"00000004" // ret offset
"00000007" // ret size
"0000000a" // success offset
"00000006" // function_selector_offset
+ to_hex(OpCode::RETURN) + // opcode RETURN
"00" // Indirect flag
"00000008" // ret offset 8
"00000003"; // ret size 3

auto bytecode = hex_to_bytes(bytecode_hex);
auto instructions = Deserialization::parse(bytecode);

std::vector<FF> calldata = {};
std::vector<FF> returndata = {};

// Generate Hint for call operation
auto execution_hints = ExecutionHints().with_externalcall_hints(
{ { .success = 1, .return_data = { 9, 8 }, .l2_gas_used = 0, .da_gas_used = 0 } });

auto trace = Execution::gen_trace(instructions, returndata, calldata, public_inputs_vec, execution_hints);
EXPECT_EQ(returndata, std::vector<FF>({ 9, 8, 1 })); // The 1 represents the success

validate_trace(std::move(trace));
}
// TEST_F(AvmExecutionTests, opCallOpcodes)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this needs updating in a follow-up?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, the tags need updated

// {
// std::string bytecode_preamble;
// // Gas offset preamble
// bytecode_preamble += to_hex(OpCode::SET) + // opcode SET for gas offset indirect
// "00" // Indirect flag
// "03" // U32
// "00000010" // val 16 (address where gas offset is located)
// "00000011" + // dst_offset 17
// to_hex(OpCode::SET) + // opcode SET for value stored in gas offset
// "00" // Indirect flag
// "03" // U32
// "00000011" // val i
// "00000000";
// // args offset preamble
// bytecode_preamble += to_hex(OpCode::SET) + // opcode SET for args offset indirect
// "00" // Indirect flag
// "03" // U32
// "00000100" // val i
// "00000012" + // dst_offset 0
// to_hex(OpCode::SET) + // opcode SET for value stored in args offset
// "00" // Indirect flag
// "03" // U32
// "00000012" // val i
// "00000001";
// // ret offset preamble
// bytecode_preamble += to_hex(OpCode::SET) + // opcode SET for ret offset indirect
// "00" // Indirect flag
// "03" // U32
// "00000008" // val i
// "00000004" + // dst_offset 0
// to_hex(OpCode::SET) + // opcode SET for value stored in ret offset
// "00" // Indirect flag
// "03" // U32
// "00000002" // val i
// "00000007";
// std::string bytecode_hex = bytecode_preamble // SET gas, addr, args size, ret offset, success, function selector
// + to_hex(OpCode::CALL) + // opcode CALL
// "15" // Indirect flag
// "00000000" // gas offset
// "00000001" // addr offset
// "00000002" // args offset
// "00000003" // args size offset
// "00000004" // ret offset
// "00000007" // ret size
// "0000000a" // success offset
// "00000006" // function_selector_offset
// + to_hex(OpCode::RETURN) + // opcode RETURN
// "00" // Indirect flag
// "00000008" // ret offset 8
// "00000003"; // ret size 3

// auto bytecode = hex_to_bytes(bytecode_hex);
// auto instructions = Deserialization::parse(bytecode);

// std::vector<FF> calldata = {};
// std::vector<FF> returndata = {};

// // Generate Hint for call operation
// auto execution_hints = ExecutionHints().with_externalcall_hints(
// { { .success = 1, .return_data = { 9, 8 }, .l2_gas_used = 0, .da_gas_used = 0 } });

// auto trace = Execution::gen_trace(instructions, returndata, calldata, public_inputs_vec, execution_hints);
// EXPECT_EQ(returndata, std::vector<FF>({ 9, 8, 1 })); // The 1 represents the success

// validate_trace(std::move(trace));
// }

TEST_F(AvmExecutionTests, opGetContractInstanceOpcodes)
{
Expand Down
Loading
Loading